views:

953

answers:

10

After an incident at work where I misused String.IsNullOrEmpty with a Session variable, a fellow coworker of mine now refuses to accept my usage of String.IsNullOrEmpty. After some research, apparently there's a bug listed for IsNullOrEmpty on MSDN (link) (read note at the bottom):

As of April 4, 2006, there is a bug (possible in the JIT) that makes this method fail when optimizations are turned on. It is known to affect both C# and VB.

More information can be found here (link). Microsoft the bug is 'supposedly' fixed post-Orcas, but unfortunately my employer still uses VS2005. But if the problem is fixed in 2008+ so be it. That's just fine with me.

While my colleague's refusal of my code with IsNullOrEmpty to me is blind ignorance (IMO) he certainly can't tell me why not to use it other than the misuse with the session variable. I've used IsNullOrEmpty all over our code with no issues whatsoever. Personally, I find it much more readable in addition of doing two things in one statement.

After googling for opinions on the subject, I've found sites that take the pros/con stance. Here are some of the sites I've read about this:

http://cs.rthand.com/blogs/blog_with_righthand/archive/2006/06/22/1063.aspx

http://www.omegacoder.com/?p=105

One site (http://dotnetperls.com/isnullorempty) sums up the method (IMHO) pretty well:

Here we looked that IsNullOrEmpty method on the string type, which provides us with a good and relatively efficient method of checking whether a string is OK to save or use. However, for performance, it may be better to use manual null checks. Empty strings can also be tested in other ways, and my research here shows that checking length is fastest.

Assuming the bug fix is in place (and working correctly) in VS2008/2010/etc., is there any reason not to use String.IsNullOrEmpty with VS2005 and beyond? I realize this may seem a little overkill over such a silly little method, but I'd like to know if there's more behind the scenes going on and if anyone has alternative explanations.

+19  A: 

This issue was fixed in .NET 2.0 sp1. There is no reason to avoid its use now.

If you're using .NET 2, you should have sp1 for many other reasons anyways - I see no reason to avoid this for a bug that no longer exists.

Reed Copsey
Thanks, I'll be looking into getting the service pack if it's not already installed. I'm not sure of all the hoops I'll have to jump but either way, we should be running the latest service pack on our web server regardless. Thanks!
osij2is
A: 

If it is broken in your version, then it is trivial to just have a static method that will do the check, so just do:

public static bool isNull(String s) {
  return s == null || s.trim().length == 0;
}

No point in getting into a big issue over something that should be relatively easy to fix.

You don't need to change it everywhere, though you can do a global replace of one static method with another.

James Black
There's no reason to duplicate framework functionality. This is a bug that was fixed in .net 2.0sp1 - why avoid it now?
Reed Copsey
Thanks, yes I agree. No need to get into a big thing over something so trivial but as long as it is/was patched I couldn't find a reason to *not* use it. Thanks for the replacement code. I may implement it.
osij2is
@Reed Copsey - If they were affected by it, then they may not have upgraded to .NET2sp1. Granted, they should upgrade, but if they haven't then rather than fighting about something fairly trivial, just work around it.
James Black
@Reed: I don't know if our webserver is running .NET 2.0SP1 so if it *is* I won't bother or if I can get our IT staff to add the service pack I won't need to implement it. But still, it's nice to have a workaround should it arise.
osij2is
If the bug is there it's likely to happen using a replacement method also, as the bug is not with the IsNullOrEmpty method itself.
Guffa
@James - A user (peterchen) on the MS connect report also also demonstrated the same problem with the code you have above in the broken version.
Kev
@Kev - That is surprising, but OK. So what is the solution, if you can't upgrade?
James Black
@James - I'm guessing install Service Pack 1 for FW 2.0 or stop using `IsNullOrEmpty`.
Kev
+4  A: 

you could write a unit test that passes an empty string and one that passes an null string to test this stuff, and run it in VS2005 and after in 2008 and see what happened

Omu
Unfortunately, my current employer doesn't really implement unit testing. Not to say that I wouldn't jump at the chance but thanks for the idea. Maybe it's just one more reason I can give to management that we *should* be unit testing to begin with.
osij2is
+2  A: 

In that bug report in the link you include it states:

This bug has been fixed in the Microsoft .NET Framework 2.0 Service Pack 1 (SP1).

Since that is the case it shouldn't matter if you're using VS 2005 as long as you have SP1 for .NET 2 installed.

As for whether or not to use it, check out this post by CodingHorror.

Bryant
+1  A: 

I am pretty sure it was fixed on the SP1 but anyway you can create your own null or empty method :)

Yassir
+1  A: 

As with any language or portion thereof, it's all about knowing the pros/cons and making an educated decision based on that information. IMHO.

jaywon
"...it's all about knowing the pros/cons and making an educated decision based on that information." - Isn't that *why* I'm asking here? To *make* and *educated* decision?
osij2is
@osij2is - my comment was not meant to be insulting at all. others had already stated that the bug was fixed, so i felt no need to repeat the answer. i was simply putting my perspective on the differences in opinion between you and your coworker. if you now know that the solution is perfectly acceptable, you now have the argument for why it is okay....IMHO :)
jaywon
+2  A: 

We use an extension method for string.IsNullOrEmpty:

public static bool IsNullOrEmpty(this string target)
{
  return string.IsNullOrEmpty(target);
}

Using this approach, even if it were broke in some previous version, a bugfix is only one line of code.

And the added utility of being able to use the method on a string instance that might be null:

string myString = null;
if (myString.IsNullOrEmpty())
{
  // Still works
}
Yannick M.
VS2005 won't work with extensions. I tend to use an extension myself just because I find it more natural to write.
James Black
I don't think VS2005 is the issue. He is using the .NET 2.0 pre SP1 framework version. Starting with C# 3.0 extension methods were added, but it is perfectly possible to use them on 2.0 framework library with a small tweak: http://geekswithblogs.net/robp/archive/2007/12/20/using-extension-methods-in-.net-2.0-with-visual-studio-2008.aspx
Yannick M.
Had no idea (until now) that extension methods can be successfully called when the instance is null.
Mike Powell
+3  A: 

I've heard about that bug before, and from what I can gather it never occurs in any real code, only in code like the example that doesn't really do anything. Besides, the bug is not with the IsNullOrEmpty method itself, so it would occur regardless of how you check the string.

If the method does exactly what you want to do, you should use it. However, you should not use it in every situation to check for an empty string. Sometimes you only want to check if the string is empty and not if it's null.

If the string variable is null, this will just skip the code block:

 if (!String.IsNullOrEmpty(str)) { ... }

If the string variable is null, this will cause an exception:

 if (str.Length > 0) { ... }

If the variable is not supposed to be null, you probably want the exception instead of the code treating the null value as an empty string. If something is wrong you want to catch it as early as possible, because it will be harder to track the problem back to the source the longer the exception is from the cause.

Guffa
Hence why I always *prefer* the IsNullOrEmpty method. Two actions in one simple step. When it comes to strings, I find that most people tend to code against one or the other (empty vs. null) but rarely both. I prefer to check both empty and null if I don't have much experience with a particular application or implantation.
osij2is
@osij2is: Consider the semantics of the code also... If you use IsNullOrEmpty that means that you expect the reference to be null sometimes, and if the reference is never supposed to be null the check for it makes the code confusing.
Guffa
"It never occurs in real code" is a very optimistic measure.
peterchen
A: 

I wonder why people use string.Empty, it's not a good thing because it's an initialized string & this concept exists only in .Net frame everywhere this is a valid string with len of 0(db servers make very clear destinction between this and will complain if you have logic checking for null but you get and empty string). I think that string.IsNullOrEmpty is one of the top 5 worst practices/functions I have ever seen because somehow it encourages/makes it look ok people to init their strings and can be treated as null. This function should have never been added and I think the .Net guys should try to phase it out:) Who needs and empty string anyway ? I've never used it unless I had to because of existing projects used it

Ivo S
A: 

When implementing argument checking in APIs, I usually check for each condition separately and throw different exceptions: ArgumentNullException for a null reference or, depending on the API specifications, ArgumentException for an empty string. In this case, using String.IsNullOrEmpty doesn't allow you to distinguish between these two separate error conditions.

if (str == null)
{
    throw new ArgumentNullException("str");
}
if (str == string.Empty)
{
    throw new ArgumentException("The string cannot be empty.", "str");
}
CodeSavvyGeek