Comments by Andrew Rissing (Top 122 by date)

Perhaps it would make it completely obvious looking at the code for Dictionary's ContainsKey/Indexer vs. TryGetValue.

public bool ContainsKey(TKey key)
{
return (this.FindEntry(key) >= 0);
}

and

public TValue this[TKey key]
{
get
{
int index = this.FindEntry(key);
if (index >= 0)
{
return this.entries[index].value;
}
ThrowHelper.ThrowKeyNotFoundException();
return default(TValue);
}
set
{
this.Insert(key, value, false);
}
}

vs.

public bool TryGetValue(TKey key, out TValue value)
{
int index = this.FindEntry(key);
if (index >= 0)
{
value = this.entries[index].value;
return true;
}
value = default(TValue);
return false;
}

So, you can see it does the same work (minus one additional call to FindEntry), but all in one single call.

If you use TryGetValue, it uses an out parameter to set the value and returns true, otherwise it returns a false and sets the out parameter to default.

By doing this, you access the dictionary once to see if the key exists and get the value if its there, rather than checking if the key exists and then later accessing it again to get the value. Do some performance testing, you'll see it is much faster to do it this way.

Furthermore, its easier to read:

if (!dct.TryGetValue(key, out value))
{
dct[key] = value = ....;
}

// value will always be set to something valid here.

If you do update this, I'd recommend using "TryGetValue" rather than using "ContainsKey", as it would yield a better performance.

Based on a comment by nonexisto, I realized that the constraint doesn't allow private constructors. So, the constraint isn't useful if you're intending this to work for classes with a hidden default constructor. If you're using this for public constructors, then you definitely don't need all of this to initialize the class.

Corrected. I removed the type constraint. I didn't realize it wouldn't apply private constructors. As for the code, it is definitely still applicable, as the code was designed for accessing a private member (hence the reference to Activator.CreateInstance).

Ignore that, nm...that would only apply to a public constructor.

Btw, you might want to add a default constructor constraint (i.e. where T : new()).

The overhead in your method may likely be due to the dynamic keyword usage.

But either way, here is the code I used to test it:

-----------------

var sw = Stopwatch.StartNew();

for (int i = 0; i < 1000; ++i)
{
List<string> x = DynamicInitializer.New<List<string>>();
}

sw.Stop();
Console.WriteLine("New<t>: " + sw.ElapsedMilliseconds);

var di = new DynamicInitializer();
sw = Stopwatch.StartNew();

for (int i = 0; i < 1000; ++i)
{
List<string> y = di.NewInstance<List<string>>();
}

sw.Stop();
Console.WriteLine("NewInstance<t>: " + sw.ElapsedMilliseconds);

Console.ReadLine();

-----------------

Testing it again, it seems the values are much closer now - ~150ms vs ~220ms. But using Expressions, still wins out. It roughly equates to being 20% faster than the original code. It doesn't seem to vary much with different types or release/debug builds, so I'd be curious if you find where the difference you're speaking of shows up.

If I take your code and rewrite as follows:

public V NewInstance<V>() where V : class
{
var type = typeof(V);
var target = type.GetConstructor(Type.EmptyTypes);
var dynamic = new DynamicMethod(string.Empty,
type,
new Type[0],
target.DeclaringType);
var il = dynamic.GetILGenerator();
il.DeclareLocal(target.DeclaringType);
il.Emit(OpCodes.Newobj, target);
il.Emit(OpCodes.Stloc_0);
il.Emit(OpCodes.Ldloc_0);
il.Emit(OpCodes.Ret);

var method = (Func<V>)dynamic.CreateDelegate(typeof(Func<V>));
return method();
}

It improves the timing from ~220ms to ~190ms.

I would disagree with statics being considered a bad practice. If you're not planning on trying to 'derive' them and they are purely utility in nature, statics are fine. Best practices should always be considered but not dogmatically followed.

Btw, you can mark these as all static.

It is generally not a good practice to reference items off of a disposed object. Plus, you can return within a using clause, as the objects will properly be disposed. You need to correct your ZipFilesToByteArray method to be as such:

public static byte[] ZipFilesToByteArray(IEnumerable files, CompressionOption compressionLevel = CompressionOption.Normal)
{
using (MemoryStream memoryStream = new MemoryStream())
{
ZipHelper.ZipFilesToStream(memoryStream, files, compressionLevel);
return memoryStream.ToArray();
}
}

It isn't about feelings, so much as etiquette in general. I don't personally care about the vote. I understand that it is an edge case for someone to want to do this. You are absolutely correct about the risk involved in using such, but like any tool its usefulness and effectiveness should be judged in the context of its use. So, like I started with, to each his own.

I think "All Time Programming" states it correctly. There are times when you will 'tighten' the screws on something that needs to be performant. But if this is called 2 times in the entire lifetime of an application, such code would be over-engineering. Because the original intent of the author was to place this into a constructor, it would seem you're likely to construct the object more than a few times, so I sought to derive a solution that removes the overhead of reflection. Regardless, I don't feel a vote of one is warranted based on one aspect of the code, when it will easily outperform the reflection based solution.

I liken it to saying that all trucks are worthless because they have poor gas-mileage. But have you tried moving your couch in a Prius?

Bah, System.Drawing *shakes fist*. I should have attempted before posting.

Ignoring the issues with not using SQL parameters, why not just do the math prior to the string.Format and save the database from computing it?

I would gladly see an alternate of yours that would accomplish the same goals with as much performance and elegance. Granted, to each his own, but I feel such a vote is unwarranted.

I'll definitely consider it, just need to find the time to do it.

If you're going to save off the Regex and use it more than once, compiled should always be faster. Otherwise, uncompiled should be used for the best performance.

I've posted the code to what I was describing below for your perusal.

It would just rely on what you described above to acquire what the default value was. So, it isn't any different from the above aside from pushing the setting of those values into a cached expression that will read the DefaultValueAttribute only once on the first hit.

Reflection is always slow, furthermore TypeDescriptor has additional overhead over regular reflection as well, which is partly due to the caching it does internally.

If you really wanted to go hog wild, you could go this route...

1) Create a static class that maintains a static dictionary with a key of type and value of Action.
2) In the class create a generic static method called Initialize<t>(this T item) (or whatever you like).
3) In the static method Initialize, you'll create an Action<t> using Expressions that will do the above, but cache the compiled expressions in the dictionary from line 1. If it already exists in the dictionary, just retrieve it.
4) Cast the Action to Action<t> (thanks to the generic method).
5) Pass in the item parameter and viola.


Granted, this is probably an overkill. It would greatly improve performance, as this would be called upon each object's construction.

Code coverage. As in, the percentage of the code covered (or executed) through a unit test. See http://en.wikipedia.org/wiki/Code_coverage.

Indeed, I have. Oddly enough, your alternate was removed. *shrug*

Personally, if you were going to go that route. I'd still try to drop the 'last' parameter from being passed in. It is just extra baggage for someone to maintain in order to call your method.

One solution I can see would be to override a StringReader. The index would still be maintained internal to that object, but at least they don't have access to modifying it or incorrectly passing it back in. The parameter just strikes me as exterior to finding items in a string.

Easy enough to do. It is a minor code smell, but smelly it shall be. :)

Actually, because you treat String.Empty as the end. You probably had it like that on purpose. You'd have to use null or some such to overcome that limitation.

Just a heads up, you might want to review the code:
-GetBetween - You have an extra + 1 on the second argument for computing endIndex.
-GetBetweenExcludeTokens - You didn't subtract the start.Length on the return.

While Alternative 4 will only load the instance when it is called, it could have some concurrency issues as two threads could call it instantiating two separate instances. Alternative 3 would not have concurrency issues, but would instantiate the instance the first time the class type is referenced in code.

This is very limited to a specific flavor of CSV (ex. ones that don't include, comment rows, quoted columns, etc.) and relies on MessageBoxes and Application.Exit to control flow.

If you're going to go through the hassle of parsing it to validate it, why not get the parsed values as well using one of the many parsers available from Codeproject or the internet?

Did you just create your own data structure and perform the calculations using the "by hand" algorithm?

I knew it would. I just didn't want to change the context of the original solution. Either way, not a big deal.

Updated :)

It is essentially now what cechode put forth, but meh. :D

;-)

Sorry, initially read your comment as referring to Cechode's not Rodriguess'. But essentially the comments still stand as well. I did do a performance test and mine was twice as fast as Rodriguess' as well. Most likely due to the fact that he's updating a list to maintain which items have not been removed yet.

There is no need to run performance testing against the two. You can visually inspect them and see that it would be more performant.

1) It isn't allocating a new object for each member of source.
2) It is performing a subset of the operations in cechode's version.

Just for the fun of it, I went ahead and did some performance testing. In terms of speed, it is at least an order of two times as fast.

Corrected above. Thanks for the catch.

This only works though from within the class that defines the event.

Ah, but that assumes you're shipping release code... :-D

Agreed, it should have been supported in some way.

As for the recomputing of compositeValues, it would be easy to implement a cache for those values.

Just fyi, it will cause 64-bit Vista to crash running this...

You might want to do some comparisons in performance, because I believe "info.GetValue(obj, null)" will be orders of magnitude slower than expression-based alternatives.

This isn't a tip/trick, but a blog post. I'd delete it and have CP pull the info from your blog directly.

Definitely makes more sense if you are doing an operation that is more complex.

I'll keep an eye out for the update, because at this point I'm not seeing the benefit.

I agree with Jose. While interesting academically, what is the use of something like this when you could just "&&" the results together?

Thanks for pointing out the extra ToArray. I have removed it, since it is not needed.

While I haven't tested it for performance, it may very well be more performat than relying on a DataTable to do the sorting, since it has to parse the string passed into the "Select" method. I mainly was trying to get away from having to pass the sort column as an integer and then the sorting direction as a string. Furthermore, using the above style is very much inline and natural for someone who is use to Linq's OrderBy extension methods.

Btw, I've updated my alternate to provide a more complete picture of what the code would be and an example of how to use it.

The Timer class you've listed is the Stopwatch class. The only reason you would need this if you're working on a .NET version that doesn't provide that class already.

Assigning null to a variable is very inexpensive. In every case I've seen it used, it reduces readability and lacks any real benefit. The ultimate point I'm trying to make is that do something because it has some reason or benefit. The more you bloat code with 'this is what I've always done' kind of things, you end up confusing those looking at your code. They can't tell the vital pieces of code from the noise that is left over from a coding habit.

There would be no need. The garbage collector would take care of it, once it is no longer referenced. Since it is a local variable, it will no longer be referenced at the end of the method. Assigning null is extraneous and a relic of VB coding.

Aside from going row then column, the code is equivalent to the original post. The code was just refactored to remove out unneeded statements and take advantage of the using statement to make it easier to read. The performance should be essentially equivalent.

Checking the state is unnecessary. The calling "Close" twice on a connection doesn't throw an exception. Plus, there are only two ways to close the connection. One is the normal path without exceptions where the "Close" immediately after the ExecuteNonQuery (which even this could be omitted if you favored leaving it up to the dispose of the SqlConnection). The other way is the exceptional, which is where the dispose will correctly close the connection for you.

I don't follow what you're trying to say. Can you complete your thought?

I feel like there's a joke somewhere in this title.. :D Good stuff.

I edited the above for grammar/readability and redid the summary because it was unclear.

You might want to look at the String.Trim, String.TrimStart, and String.TrimEnd to align it to the way that behaves. For instance, use char.IsWhiteSpace, rather than just checking against a space.

I personally like using Stopwatch over DateTime, but that'll work as well. Including the time to retrieve the columns, I get only about a 2x speed improvement using DataColumns. Not as fast as you got, but still significantly faster.

You realize your @Command variable is too small for your 500 character @Path input. Plus, based on your ISNULL replacement, @Path can never be null.

I also just steer clear of sp_executesql, unless its absolutely necessary. In this case, you can avoid it.

Oh, I wasn't saying you could get around the exists check, you can't. I'll post an alternate with what I'm saying....

Out of curiousity, why not just drop the function at the start and then create it, rather than alter? Using drop/create, you don't have to worry about the signature of the function matching up as much. Plus, I've read somewhere that using a single SELECT (with multiple assignments in it) over multiple SET's is actually faster. In cases where there is just one assignment, SET wins out. I wonder if that might improve the speed of this function a little bit.

Don't use 'throw ex', but rather 'throw'. If you use 'throw ex', you clear out all stack trace information as if you created a totally new exception.

Yeah, I'm not a big fan of "if (a == true)", but at least it gets you around having to declare a variable just to use it to check a return variable.