CODEWITHSUNDEEP
c#if-statementsimplification
Ozzyberto
Ozzyberto

Reputation: 399

Need help simplifying a nested if block

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?

Upvotes: 1

Views: 233

Answers (7)

Servy
Servy

Reputation: 203802

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.

Upvotes: 3

Prabhu Murthy
Prabhu Murthy

Reputation: 9261

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.

Upvotes: 1

Alexandre Vinçon
Alexandre Vinçon

Reputation: 954

'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 !

Upvotes: 2

Justin
Justin

Reputation: 6711

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;
    }
}

Upvotes: 1

Favourite Onwuemene
Favourite Onwuemene

Reputation: 4377

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

Upvotes: 0

Jakub Szułakiewicz
Jakub Szułakiewicz

Reputation: 840

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

Upvotes: 2

Phillip Schmidt
Phillip Schmidt

Reputation: 8818

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.

Upvotes: 2

Related Questions