Can I safely ignore CodeAnalysis warning: replace string == "" with string.IsNullOrEmpty ?

Question

I've got code similar to this:

string s = CreateString();
if (s == "") foo(s);

If s equals "", foo should be called. If string is null, which should never happen, then a NullReferenceException is fine (as this is, after all, an exceptional situation).

CodeAnalysis tells me to test for s.IsNullOrEmpty. This would alter the functionality in a nunintended way.

Performance is not an issue.

Is it safe to suppress the associated CA1820 warning?

Edit: Updated code sample and text to better reflect my case.

Edit: This is the (slightly altered) actual code (it's in a standard IXmlSerializable implementation):

public void ReadXml (XmlReader reader)
    // ...
    string img = reader.ReadElementString ("Image");
    if (img != "") {
        Image = Image.FromFile(img);
    }
    // ...

Answer 1

If null is OK, you'll be fine either way.

Answer 2

yes.

But i would agree with CodeAnalysis with string.IsnullOrEmpty is a safe choice.

Answer 3

It would be better to write the test as:

if(s != null && s == "")

You can then process a null value in another if statement

Answer 4

It will behave differently with regards to nulls, so it depends what you want to happen; you mention that NullReferenceException would be OK, but there is nothing in the code cited that would raise this, hence why it could cause unexpected errors downstream.

I never have, but I'm always tempted to add:

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

so I can use:

if (s.IsNullOrEmpty()) foo();

Answer 5

Every Code Analysis warning has associated documentation that you can access by highligting the warning and pressing F1. You can also right-click on the item to get help.

In any case, here's the documentation that explains that particular warning.

According to that documentation, it is "safe to suppress a warning from this rule if performance is not an issue".

Answer 6

Not handling exception whereas you Can is gennerally a bad idea so CA is right in that you either need to treat null as empty or handle the exception. A null reference exception caused by using a return value is a very bad thing. At the very least put in a Debug.Assert(s!=null) and compare to string.Empty

Answer 7

You're not really ignoring the warning, you've looked at the code and decided that the warning doesn't apply. This is a perfectly reasonable condition under which to suppress the warning.

Pure Speculation

I wish I knew a little more about what you were trying to do, however. I suspect that there may be a better way to handle it. The pattern reminds me of returning an error message or empty to signal the success of a method. If that's the case, I would consider either returning void and throwing an exception on failure or returning bool and only throwing exceptions when the message is critical and returning true/false otherwise.

Answer 8

Specs :

If s equals "", foo should be called. If string is null, which should never happen, then a NullReferenceException is fine.

Just test the string length as adviced in the CodeAnalysis rule :

if (s.Length == 0) foo(s);

Your question :

Is it safe to suppress the associated CA1820 warning?

You can ignore it, your code will work but I wouldn't advice it, follow the guidelines as much as you can. Even if the topic (performance) is not an issue, your code will be more consistent and you get accustomed to write standard code.