Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a somewhat unwieldy IF statement and am wondering if anyone has any suggestions on how to make it more readable.

One one line it was all but impossible to understand, so I broke it up into multiple lines:

if (
        empty($row)                                                            // IF the row doesn't exist
    || (!empty($row)                                                           // OR it does exist
        && (     date('j', time()) != date('j', strtotime($row['date_added'])) // AND hasn't been update this day
             && (   empty($row['ignore'])                                      // AND has never been marked to ignore
                 || time()-strtotime($row['ignore']) > 3600*24*7               // OR was last marked to ignore over a week ago
                )
           )
       )
) {
    return $this->updateRow($id);
}
else {
    return 0;
}

However, it's still kind of hard to decipher. I thought about breaking up it into IF/ELSE statements, which makes it a lot more readable, but it also means having redundant code:

if (empty($row)) { // if the row doesn't exist
    return $this->updateRow($id);
}
else { // the row does exist
    if (date('j', time()) != date('j', strtotime($row['date_added']))) {             // if the row hasn't been updated this day
        if (empty($row['ignore']) || time()-strtotime($row['ignore']) > 3600*24*7) { // if the row has never been marked to ignore OR was last marked to ignore over a week ago
            return $this->updateRow($id);
        }
        else { // the row was marked to ignore within the past week
            return 0;
        }
    } // the row has been updated this day
    else {
        return 0;
    }
}

What is the most readable way to format this IF statement?

  1. The first method as it's more concise and has no redundant code
  2. The second method since it's a little more readable (even though it has a lot of redundant code)
  3. A different formatting method
share|improve this question
add comment

2 Answers 2

up vote 5 down vote accepted

I've used the second snippet since I've found that much easier/faster to understand:

  1. Here you could omit the else keyword:

    if (empty($row)) { // if the row doesn't exist
        return $this->updateRow($id);
    }
    else { // the row does exist
    ...
    }
    

    The following is the same:

    if (empty($row)) { // if the row doesn't exist
        return $this->updateRow($id);
    }
    // the row does exist
    ...
    
  2. Then, you could create an explanatory variable to make the comments unnecessary:

    $rowExists = !empty($row)
    if (!rowExists) {
        return $this->updateRow($id);
    }
    ...
    
  3. Another explanatory variable with an inverted condition:

    $rowExists = !empty($row)
    if (!rowExists) {
        return $this->updateRow($id);
    }
    
    $updatedToday = (date('j', time()) == date('j', strtotime($row['date_added'])));
    if ($updatedToday) {
        return 0;
    }
    

    Explanatory variables give names to the expression and reveal the developer's intent, so maintainers will know what's the purpose of (date('j', time()) == date('j', strtotime($row['date_added']))) faster (it should mean that we have an update today) (he or she doesn't have to figure it out or reverse engineer).

    It also makes bug fixing faster since a maintainer easily can validate whether the purpose is wrong (we need to check whether we have an update this week instead of this day!) or the implementation (to check that we have an update today or not we should use created field instead of date_added, for example).

  4. Yet another explanatory variables and flattened conditonals:

    $neverIgnored = empty($row['ignore']);
    if ($neverIgnored) {
        return $this->updateRow($id);
    }
    
    $ignoredLastWeek = (time() - strtotime($row['ignore']) < 3600 * 24 * 7);
    if ($ignoredLastWeek) {
        return 0;
    }
    
    return $this->updateRow($id);
    

Final code:

$rowExists = !empty($row)
if (!rowExists) {
    return $this->updateRow($id);
}

$updatedToday = (date('j', time()) == date('j', strtotime($row['date_added'])));
if ($updatedToday) {
    return 0;
}

$neverIgnored = empty($row['ignore']);
if ($neverIgnored) {
    return $this->updateRow($id);
}

$ignoredLastWeek = (time() - strtotime($row['ignore']) < 3600 * 24 * 7);
if ($ignoredLastWeek) {
    return 0;
}

return $this->updateRow($id);

You could also create a boolean isUpdateNeeded() function:

function isUpdateNeeded(...) {
    $rowExists = !empty($row)
    if (!rowExists) {
        return true;
    }

    $updatedToday = (date('j', time()) == date('j', strtotime($row['date_added'])));
    if ($updatedToday) {
        return false;
    }

    $neverIgnored = empty($row['ignore']);
    if ($neverIgnored) {
        return true;
    }

    $ignoredLastWeek = (time() - strtotime($row['ignore']) < 3600 * 24 * 7);
    if ($ignoredLastWeek) {
        return false;
    }

    return true;
}

if (isUpdateNeeded(...)) {
    return $this->updateRow($id);
}
return 0;

References:

  • Clean Code by Robert C. Martin, G19: Use Explanatory Variables
  • Clean Code by Robert C. Martin, Bad Comments, p67: Don’t Use a Comment When You Can Use a Function or a Variable
  • Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable
  • Flattening Arrow Code
share|improve this answer
    
Thanks you for spending so much time on this, your examples are really helpful! May I ask why in you tend to define a variable right above each IF statement instead of simply putting the expression inside the IF statement? –  Nate Jul 17 at 3:46
    
@Nate: Sure! I've edited the answer, check it please. Or did you mean if ($neverIgnored = empty($row['ignore'])) { ... }? –  palacsint Jul 17 at 7:22
add comment

Here:

date('j', time()) != date('j', strtotime($row['date_added'])

instead of j (Day of the month without leading zeros, 1 to 31) I would consider using Yz where

  • Y: A full numeric representation of a year, 4 digits (Examples: 1999 or 2003),
  • z: The day of the year (starting from 0) (0 through 365).

to avoid exactly one-month (two-months etc.) old rows being considered as updated today.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.