Better Syntax for an If statement

Question

I have a condition like this one:

if (string.IsNullOrEmpty(filename) || size != "Large" || size != "Medium" || size != "Small")

Probability in future I will have to manage more size in the if statement.

I would like to know if exist a more manageable and readable way to write this condition.

Please provide a real example thanks for your time on this.

score 6 · Accepted Answer · answered Aug 16 '11 at 11:07

6

You can keep a hashset of words and check:

HashSet<string> filterWords = new HashSet<string>();
// Put all words in the hash set


if (filterWords.contains(size))
{
    // Do what ever you need
}

answered Aug 16 '11 at 11:07

Lior Ohana

3,467
4
34
49

Any IEnumerable will do, won't it? HashSet is one example, but you might as well use an array. – Erik A. Brandstadmoen Aug 16 '11 at 11:08
A hash should do a string match more quickly than others though. – Aug 16 '11 at 11:15
Also consider using `var hashSet = new HashSet(StringComparer.InvariantCultureIgnoreCase);` if `larGE`, `Large`, `large`,etc. should be threaded the same. – Oliver Aug 16 '11 at 14:24

score 3 · Answer 2 · answered Aug 16 '11 at 11:07

3

// you could externalize and manage this list somewhere else 
var sizes = new[] { "Large", "Medium", "Small" }; 

if (string.IsNullOrEmpty(filename) || !sizes.Contains(size))
{
    ...
}

answered Aug 16 '11 at 11:07

Darin Dimitrov

1,023,142
271
3,287
2,928

Why use array instead of hash set? – abatishchev Aug 16 '11 at 11:11
@abatishchev, why use a HashSet instead of an array :-) A HashSet would indeed be better as it will be faster to find elements and they are guaranteed to be unique. – Darin Dimitrov Aug 16 '11 at 11:14
1

@Darin of course, if that performance improvement on a 3 item list makes a big impact on the code overall, I'd love to see how they got the other code so fast :-) – Adam Houldsworth Aug 16 '11 at 11:19

score 3 · Answer 3 · answered Aug 16 '11 at 11:08

3

Put the sizes in a collection of some sort, and use "Contains". See MSDN for an example: http://msdn.microsoft.com/en-us/library/bb352880.aspx

answered Aug 16 '11 at 11:08

Erik A. Brandstadmoen

10,430
2
37
55

Oded · Answer 4 · 2011-08-16T11:12:32.860

Option 1:

Write a small function that returns a bool and that only contains the size tests and use that in your if.

if (string.IsNullOrEmpty(filename) || GoodSize(size))
{
 //...
}

private bool GoodSize(string size)
{
   return size != "Large" || size != "Medium" || size != "Small";
}

Option 2:

Create a list of sizes to test against and use Contains:

var goodSizes = new[] { "Large", "Medium", "Small" };
if (string.IsNullOrEmpty(filename) || !goodSizes.Contains(size))
{
    //...
}

And you can combine both options for better clarity and encapsulation.

score 1 · Answer 5 · answered Aug 16 '11 at 11:07

1

If they are going to change, perhaps a static list is better:

private static List<string> Sizes = new List<string> { "large", "medium", "small" };

if (string.IsNullOrEmpty(filename) || Sizes.Contains(size.ToLower()))
{

}

For even cleaner code, encapsulate the checking of size into it's own method and amend that method when required:

if (MeetsSizeRequirementsOrIsNull(filename, size))
{        
}

private static bool MeetsSizeRequirementsOrIsNull(string filename, string size)
{
    List<string> sizes = new List<string>() { "..." };

    return string.IsNullOrEmpty(filename) || sizes.Contains(size.ToLower())
}

answered Aug 16 '11 at 11:07

Adam Houldsworth

63,413
11
150
187

You probably want to use `Enumerable.Contains()` with `StringComparer.OrdinalIgnoreCase` – abatishchev Aug 16 '11 at 11:14
@abatishchev semantics, they both achieve the same, it's only an example. This also assumes that case isn't required. – Adam Houldsworth Aug 16 '11 at 11:18
Anyway, string comparison using ToLower()/ToUpport() is very bad practice. SO I don't recommend use that even as example, because somebody can start to do such way. – abatishchev Aug 16 '11 at 11:49
@abatishchev Bad practice is opinionated - and the portion of the code you highlight is not even in question by the OP. Personally I'd go the ignore case route too, but I couldn't remember the syntax off the top of my head. – Adam Houldsworth Aug 16 '11 at 11:50

score 1 · Answer 6 · answered Aug 16 '11 at 11:08

If you have more than one thing to do as a result of the test, writing it out as a switch/case might be more readable.

Otherwise, if you have loads of those values, it might be nicer to keep the list in some dictionary - let's say notHandledSizes. Assign "Large" => true, "Medium" => true, ... and just check if size is existing and true in that dictionary.

Wil · Answer 7 · 2011-08-19T21:30:32.967

1

What you have done looks the most direct way of doing it. Any modification will simply be moving the mess elsewhere. If you do not need to reuse this code anywhere else, I would leave it the way it is.

edited Aug 19 '11 at 21:30

answered Aug 16 '11 at 11:09

Wil

10,234
12
54
81

score 1 · Answer 8 · edited May 23 '17 at 09:59

1

Below line breaks makes it more readable.

   if (string.IsNullOrEmpty(filename) || 
    size != "Large" || 
    size != "Medium" || 
    size != "Small")

Design Tip

If many objects involve with your long if condition, it's good to write small properties/methods that return true/false in those Classes.

if (string.IsNullOrEmpty(filename) || object.IsProperSize)

Sometimes Enum Flags Attribute also helps such case. Look at here for a good explanation.

edited May 23 '17 at 09:59

Community

1
1

answered Aug 16 '11 at 11:13

CharithJ

46,289
20
116
131

Interesting syntax and approch for flags – GibboK Aug 16 '11 at 11:48

Better Syntax for an If statement

8 Answers8