CODEWITHSUNDEEP
c#string
Omar
Omar

Reputation: 40182

Concat string in IsNullOrEmpty parameter

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.

Upvotes: 4

Views: 1836

Answers (6)

Matt Kocaj
Matt Kocaj

Reputation: 11535

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

Upvotes: 1

Michael Burr
Michael Burr

Reputation: 340218

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

Upvotes: 3

Chris Fulstow
Chris Fulstow

Reputation: 41882

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
}

Upvotes: 4

Jason Williams
Jason Williams

Reputation: 57902

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.

Upvotes: 2

ICR
ICR

Reputation: 14162

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.

Upvotes: 5

Michael Petrotta
Michael Petrotta

Reputation: 60902

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

Upvotes: 0

Related Questions