Concat string in IsNullOrEmpty parameter

Question

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.

score 5 · Accepted Answer · answered Dec 12 '09 at 07:31

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.

answered Dec 12 '09 at 07:31

ICR

13,896
4
50
78

I was also surprised that concatenating null string references didn't throw. – Michael Burr Dec 12 '09 at 07:37
+1 for explicitness (i think that is a word) - the code is not as easily understood in the 2nd example – Matt Kocaj Dec 12 '09 at 07:48
+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 Dec 12 '09 at 08:08
1

@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. – ICR Dec 12 '09 at 08:15

score 4 · Answer 2 · answered Dec 12 '09 at 07:39

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
}

answered Dec 12 '09 at 07:39

Chris Fulstow

41,170
10
86
110

Michael Burr · Answer 3 · 2009-12-12T08:01:52.650

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') {
    // ...
}

Jason Williams · Answer 4 · 2009-12-12T07:35:24.860

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.

edited Dec 12 '09 at 07:35

answered Dec 12 '09 at 07:29

Jason Williams

56,972
11
108
137

4

Concatenating null strings does not result in a NullReferenceException. Only dereferencing them does. – Michael Petrotta Dec 12 '09 at 07:31
2

Agree with Michael. Yes, it will be inefficient - but it won't throw. – Jon Skeet Dec 12 '09 at 07:32
1

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. – Jason Williams Dec 12 '09 at 07:36
1

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). – Jon Skeet Dec 12 '09 at 07:42
Yes - it would depend primarily on the average length of the param strings. I'd just stick with the original (multiple IsNullOrEmpty) checks myself. – Jason Williams Dec 12 '09 at 07:53
@Jon: I only suggested the array in the event that it was the initial form. I inferred it i guess from the example in the question. Sure i wouldn't suggest creating an array of strings from explicitly defined variables in local scope. That's a waste. – Matt Kocaj Dec 12 '09 at 08:25
@Jon: concatenating the strings will likely result in more copies than the list and array within the list though, especially if you call the method with large strings. the list only holds references, whereas the concatenation results in string copies. – Sander Rijken Dec 12 '09 at 12:25
As addition to the previous comment, I don't suggest creating a list. If there's 3 params that need to be checked for null, check them individually like the first example in the question – Sander Rijken Dec 12 '09 at 12:26

score 1 · Answer 5 · edited May 23 '17 at 12:26

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[].

edited May 23 '17 at 12:26

Community

1
1

answered Dec 12 '09 at 07:28

Matt Kocaj

11,278
6
51
79

2

You can just to myParams.Any(String.IsNullOrEmpty) – ICR Dec 12 '09 at 07:32
@ICR: Can you explain that in more detail? – Matt Kocaj Dec 12 '09 at 07:35
@ ICR- I think he meant p.IsNullOrTrimEmpty() is from that extension method, not the .Any() – Omar Dec 12 '09 at 07:37
yeh that's what i suggested right? your 1st comment is syntactically incorrect. the `IsNullOrEmpty` function requires a parameter. – Matt Kocaj Dec 12 '09 at 07:38
3

@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. – ICR Dec 12 '09 at 07:41
Also, if you're using .NET 4.0 you can use the new IsNullOrWhitespace in place of the custom IsNullOrTrimEmpty extension method. – ICR Dec 12 '09 at 07:45
i understand your example now. that's why you omitted the '()'. nice shortcut! – Matt Kocaj Dec 12 '09 at 08:04

score 0 · Answer 6 · answered Dec 12 '09 at 07:27

0

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

answered Dec 12 '09 at 07:27

Michael Petrotta

59,888
27
145
179

Concat string in IsNullOrEmpty parameter

6 Answers6