Need help simplifying a nested if block

Question

I have the following block of code:

if (x > 5)
{
    if (!DateTime.TryParse(y, out z))
        break;
    if (w.CompareTo(z) == -1)
        break;
}

Where x is an integer, y is a string, z and w are DateTime variables. The reason for the break; is that whole block resides within a loop.

Is there any way this could be simplified to make it easier to read?

I think that there is nothing wrong with your code. It is readable enough so don't try to shovel everything in one line just because you can. — nemesv, Sep 27 '12 at 17:46
@nemesv I totally agree. I'd suggest to use speaking variable names and a short comment to describe the reason for the break. — lupz, Sep 27 '12 at 17:57
Actually both points are already addressed in my code. The actual variable names are kinda too long so I replaced them before posting; and there is a comment just above explaining why the loop must be broken. — Ozzyberto, Sep 27 '12 at 17:59

score 3 · Answer 1 · edited May 23 '17 at 10:24

You don't need multilpe if blocks to execute the code because you are only doing one of two things, executing the loop or not executing the loop (one if and one else). As shown here you can use a single boolean expression to represent whether or not you should skip that loop iteration or not.

(x > 5) && (!DateTime.TryParse(y, out z) || w.CompareTo(z) == -1)

Having said that, including a complex condition like this inside of a loop can hamper readability. Personally, I would simply extract this condition out into a method so that the loop looked something like this:

while(!done) // or whatever the while loop condition is
{
    if(itemIsValid(x, y, w, out z))
    {
        //the rest of your loop
    }
}

//it may make sense for x, y, w, and possibly z to be wrapped in an object, or that already may be the case.  Consider modifying as appropriate.
//if any of the variables are instance fields they could also be omitted as parameters
//also don't add z as an out parameter if it's not used outside of this function; I included it because I wasn't sure if it was needed elsewhere
private bool itemIsValid(int x, string y, DateTime w, out DateTime z)
{
    return (x > 5) 
        && (!DateTime.TryParse(y, out z) || w.CompareTo(z) == -1)
}

This has several advantages. First, it is a way of self-documenting the code without even needing comments. When looking at the loop you can read it as, "while I'm not done, and if the item is valid, do all of this stuff". If you are interested in how validity is defined you look at the method, if not you skip it. You could also rename the method to something more specific, such as "isReservationSlotFree" or whatever this is actually representing.

If your validation logic is complex (this is somewhat complex) it allows you to add comments and explanation without cluttering the more complex loop.

I have found your answer to be helpful and so I will be implementing it partially. However, the answer that I find to be most to my liking would be the one provided by @Justin. If I could mark your answer as useful, I would. — Ozzyberto, Sep 27 '12 at 19:40

score 2 · Answer 2 · answered Sep 27 '12 at 17:42

2

if (x > 5)
{
    if(!DateTime.TryParse(y,out z) || w.CompareTo(z) == -1)
        break;
}

Since the two conditionals have the same result, they can be combined into one.

answered Sep 27 '12 at 17:42

Phillip Schmidt

8,805
3
43
67

Would you say that doing the same thing to the first conditional would further improve its readability? – Ozzyberto Sep 27 '12 at 17:47
@Ozzyberto I thought about that when I was writing the answer and I don't really know. It's a judgment call on your part, but if it were me, I'd probably do it this way. Personally, I hate having to read conditionals with a million different conditions. – Phillip Schmidt Sep 27 '12 at 17:54
The whole problem is that they are all required in that order. If the first conditional fails, nothing should be done. If the second one fails then the third one would fail as well since it requires the (acquired) value of `z`. – Ozzyberto Sep 27 '12 at 18:04
@Ozzyberto That's actually not a problem at all. The conditional operators all short circuit, so as soon as a single one fails (given the way that it's structured) it won't even bother evaluating the renaming expressions. This isn't just a performance optimization, as this answer demonstrates. It also allows you to both check a value for null and then use it's members in a subsequent expression. – Servy Sep 27 '12 at 18:51

score 2 · Answer 3 · answered Sep 27 '12 at 17:42

2

if ((x > 5) && (!DateTime.TryParse(y, out z) || w.CompareTo(z) == -1))
    break;

answered Sep 27 '12 at 17:42

Jakub Szułakiewicz

821
5
22

Although it's is very terse but absolutely unreadable and very hard to follow. OP wants to "make it easier to read"... – nemesv Sep 27 '12 at 17:45
@nemesv readability is subjective. I, for example, find this easier to read. In any case, the OP asked to simplify the statement, which is what this answer does. Whether simplifying an answer improves or detracts from its readability is another (also subjective) question. – Servy Sep 27 '12 at 18:53

score 2 · Answer 4 · answered Sep 27 '12 at 18:14

2

'Simplified' does not mean easier to read.

You can make your code easier to read (and more secured in regards of various coding rules) by:

1) always using brackets for if statements and alikes

2) avoid using '!' ( '== false' is much more explicit)

3) use variable names that explicit what those variables are.

4) avoid multiple break statements. Instead, use a flag that is evaluated in the while's condition.

5) if your code is still hard to read: use comments !

answered Sep 27 '12 at 18:14

Alexandre Vinçon

919
5
13

+1 for #2. Sometimes I feel I'm the only one who writes `== false`, I believe it's much more readable as `!` is very easy to overlook. – Brian Ball Sep 27 '12 at 18:19
Different people have different rules/guidelines for their code. For example, I would consider #1 a guideline, not a rule, as I feel there are valid exceptions. I disagree with #2 entirely. The rest are not absolutes, they are already listed as guidelines. Remember that readability is always subjective, and for that reason you won't find hard and fast rules that apply universally. – Servy Sep 27 '12 at 18:57

score 1 · Accepted Answer · answered Sep 27 '12 at 18:03

1

More important: use descriptive variable names for w, x, y, z (hopefully these names were just for your example):

You can also use the less than or greater than operators instead of CompareTo.

if (x > 5)
{
    bool isValidDate = DateTime.TryParse(y, out z);
    if (!isValidDate || z > w)
    {
        // comment like: stop processing if the current date 
        // is after the reference date, or if there was a parsing error
        break;
    }
}

answered Sep 27 '12 at 18:03

Justin

6,611
3
36
57

This is exactly the kind of thing I was looking for. Just one little thing - it should be `z < w` not the other way around. – Ozzyberto Sep 27 '12 at 19:48

Prabhu Murthy · Answer 6 · 2012-09-27T18:47:33.800

1

Here is one more version.

var Break = x > 5 ? ((!DateTime.TryParse(y, out z) || w.CompareTo(z) == -1) ? true : false) : false;

Short but hampers the readability.

edited Sep 27 '12 at 18:47

answered Sep 27 '12 at 18:40

Prabhu Murthy

9,031
5
29
36

What's the point of the conditional operators resolving to true/false? They are entirely redundant and can be removed. [example](http://stackoverflow.com/a/12627155/1159478) – Servy Sep 27 '12 at 18:54
I provided a link to another answer to this question without it. Just follow the link. – Servy Sep 27 '12 at 19:02

score 0 · Answer 7 · answered Sep 27 '12 at 17:54

0

if ( x > 5 ){
    if (!DateTime.TryParse(y, out z) || w.CompareTo(z) == -1) break;
}

answered Sep 27 '12 at 17:54

Favourite Onwuemene

4,247
8
28
46

Need help simplifying a nested if block

7 Answers7