HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Formatting options for a long IF statement?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
formattingstatementlongoptionsfor

Problem

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?

  • The first method as it's more concise and has no redundant code



  • The second method since it's a little more readable (even though it has a lot of redundant code)



  • A different formatting method

Solution

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

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


-
Then, you could create an explanatory variable to make the comments unnecessary:

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


-
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).

-
Yet another explanatory variables and flattened conditonals:

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

$ignoredLastWeek = (time() - strtotime($row['ignore']) 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']) 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']) 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

Code Snippets

if (empty($row)) { // if the row doesn't exist
    return $this->updateRow($id);
}
else { // the row does exist
...
}
if (empty($row)) { // if the row doesn't exist
    return $this->updateRow($id);
}
// the row does exist
...
$rowExists = !empty($row)
if (!rowExists) {
    return $this->updateRow($id);
}
...
$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);

Context

StackExchange Code Review Q#57030, answer score: 5

Revisions (0)

No revisions yet.