4

I was looking at a piece of code I wrote in C#:

if(string.IsNullOrEmpty(param1) && string.IsNullOrEmpty(param2) && string.IsNullOrEmpty(param3))
{
       // do stuff
}

and decided to make it more readable/concise

if(string.IsNullOrEmpty(param1+param2+param3))
{
       // do stuff
}

But looking at it I can't help but cringe. What are your thoughts on this? Have you ever done something like this and do you use it whenever applicable.

Note: The code previous to this line would manipulate a collection by adding specific items depending on if the a param (param1,param2,param3) is NOT empty. This if statement is meant for validation/error handeling.

Improve this question

6 Answers 6

Reset to default
5

Personally I prefer the former over the latter. To me the intent is more explicit -- checking if all parameters are null/empty.

The second also hides the fact it does handle nulls. Null strings are odd. Jason Williams above, for example, didn't relise that it does in fact work.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

I was also surprised that concatenating null string references didn't throw.
+1 for explicitness (i think that is a word) - the code is not as easily understood in the 2nd example
+1 for correct validation. In second example validation would pass even if some (not all) of strings were null or empty. This would change behavior. And the main purpose was validation, as Buddie explicitly wrote. Thus the latter example introduced a bug.
@Roman Boiko Would it change the behaviour? Surely even if some of the strings were empty or empty they are appended to a non-null-non-empty string resulting in a non-null-non-empty string, and thus the condition IsNullOrEmpty would fail.
4

Maybe write it something like this, which is a bit more readable:

bool paramsAreInvalid =
   string.IsNullOrEmpty(param1)
   && string.IsNullOrEmpty(param2)
   && string.IsNullOrEmpty(param3);

if (paramsAreInvalid)
{
       // do stuff
}
Improve this answer

Comments

3

It's a small thing, but I think a minor reformatting of your original results in improved readability and makes the intent of the code about as crystal clear as can be:

if ( string.IsNullOrEmpty(param1) && 
     string.IsNullOrEmpty(param2) && 
     string.IsNullOrEmpty(param3) )
{
       // do stuff
}

Consider this similar set of examples:

if ( c == 's' || c == 'o' || c == 'm' || c == 'e' || c == 't' || c == 'h' || c == 'i' || c == 'n' || c == 'g') {
    // ...
}

if ( c == 's' || 
     c == 'o' || 
     c == 'm' || 
     c == 'e' || 
     c == 't' || 
     c == 'h' || 
     c == 'i' || 
     c == 'n' || 
     c == 'g') {
    // ...
}
Improve this answer

Comments

2

That won't work. If any of the strings are null, you'll get a null dereference exception. You need to check them before you use them.

In addition, it's very inefficient. You are concatenating all the strings into a new string, then test if this is non-empty. This results in one or more memory allocations and potentially a lot of data being copied, only to be immediately thrown away and garbage collected a moment later.

A better approach is to write a method that takes variable arguments or a list of strings and checks them one by one using IsNullOrEmpty in a loop. This will be more efficient, safe, but still achieve the desired result of tidy code in your if statement.

Improve this answer

8 Comments

Concatenating null strings does not result in a NullReferenceException. Only dereferencing them does.
Agree with Michael. Yes, it will be inefficient - but it won't throw.
I'm glad I answered - I like it when I learn something new :-) Any chance you can stop downvoting me now? It's still inefficient.
Creating a list of strings would be inefficient too, of course - it may well be about as inefficient as concatenating the strings. In fact, it may be more inefficient - it would have to create at least two objects (the list itself, and the array within the list).
Yes - it would depend primarily on the average length of the param strings. I'd just stick with the original (multiple IsNullOrEmpty) checks myself.
|
1

If you can get the params in a collection (which if it's function you can with the params keyword) then this might work:

if (myParams.Any(IsNullOrTrimEmpty)
    {
        // do stuff
    }

The example uses this string extension and myParams is a string[].

Improve this answer

7 Comments

You can just to myParams.Any(String.IsNullOrEmpty)
@ICR: Can you explain that in more detail?
@ ICR- I think he meant p.IsNullOrTrimEmpty() is from that extension method, not the .Any()
yeh that's what i suggested right? your 1st comment is syntactically incorrect. the IsNullOrEmpty function requires a parameter.
@cottsak My example is correct, just not what you meant. Any takes a function which takes one argument, String.IsNullOrEmpty is a function which takes one argument thus it can be passed to Any.
|
0

The original code, though longer, is more clear in its intent, and likely similar performance-wise. I'd leave it alone.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.