Refactoring wordy conditional tests [duplicate]

Question

This question already has answers here:

Approaches to checking multiple conditions? [duplicate] (3 answers)

How would you refactor nested IF Statements? [duplicate] (12 answers)

Closed 5 years ago.

My first attempt at this question was too theoretical, so I've rewritten it with actual code. See the edit history if you care.

Supposing this logic, "suffering" from the arrow anti-pattern:

/**
 * Function:effective
 * ... the DateTime the org should be considered expired (or null if never).
 *
 * If a non-group org has a parent, inherits the parent's properties, and
 * the parent isn't a root org, return the parent's expiration date.
 * Otherwise, return the org's expiration date.
 *
 * Compare to <get>, which always returns the org's expiration date.
 */
public function effective() {
    if (! $this->org->isGroup()) {
        if (($parent = $this->org->getParentOrg()) instanceof OrgModel) {
            if ($this->org->isParentPropertyInherited()) {
                if (! $parent->isRoot()) {
                    return $parent->expiration()->effective();
                }
            }
        }
    }
    return $this->get();
}

I have rewritten this a few other ways (including the "fail early" approach from this question), but the one that seems the most clear to me is:

public function effective()
{
    $nonGroup  =               (! $this->org->isGroup());
    $hasParent = $nonGroup  && ($parent = $this->org->getParentOrg()) instanceof OrgModel;
    $inherits  = $hasParent && ($this->org->isParentPropertyInherited());
    $nonRoot   = $inherits  && (! $parent->isRoot());

    if ($nonRoot) {
        return $parent->expiration()->effective();
    } else {
        return $this->get();
    }
}

In my opinion, the logic tests are compact and simple, but verbose because of the method names and the language overhead of calling methods.

I am reluctant to go with my second, re-written approach because it may be obscure and not hold up under maintenance. On the other hand, I don't like the nested if. I didn't like the fail-early approach, because it repeated code.

Are there other options I'm not considering here?

Answer 1

At first glance, your 'clear' approach does look compact and clean, but if you were to unravel the statements...

// PHP?
public function effective()
{
    $nonGroup  = !$this->org->isGroup();
    $hasParent = false;
    if ($nonGroup) {
        $hasParent = ($parent = $this->org->getParentOrg()) instanceof OrgModel;
    }
    $inherits  = false;
    if ($hasParent) {
        $inherits  = $this->org->isParentPropertyInherited();
    }
    $nonRoot   = false;
    if ($inherits) { 
        $nonRoot   = !$parent->isRoot();
    }
    if ($nonRoot) {
        return $parent->expiration()->effective();
    } else {
        return $this->get();
    }
}

The use of temporary variables here just to simplify the arrow anti-pattern manifests into another problem, which is what I characterize as weak standalone if statements that doesn't communicate as clearly the original logic. If I am literally reading this:

• nonGroup checks if this org is a non-group.
• if non-group, hasParent checks if this parent is an instance of OrgModel.
• if has-parent, inherits checks if parent property is inherited.
• if inherits, nonRoot checks if parent is non-root.
• if non-root, return the parent's effective expiration.
• otherwise, the org's expiration date.

This means I have to remember four temporary variables, when are they 'skipped' (and given the default false value), and am also unable to see the linkage from one condition to the other, described clearly in your original code documentation:

• If a non-group org has a parent,
  • inherits the parent's properties, and
    • the parent isn't a root org,
      • return the parent's expiration date.
• Otherwise, return the org's expiration date.

You didn't reveal the fail-early approach you picked, but this is how I might attempt it (pardon my extremely rusty PHP skills):

public function effective() {
    $org = $this->org;
    if ($org->isGroup()) {
        return $this->get();
    }
    $parent = $org->getParentOrg();
    return $parent instanceof OrgModel
            && $org->isParentPropertyInherited() 
            && !$parent->isRoot() ? $parent->expiration()->effective() : $this->get();
}

The only unnecessary temporary variable I created here is $org, but that is just so that I don't have to repeat $this->org.

A further refinement of your codebase, is for isParentPropertyInherited() do the instanceof check internally as well, if appropriate. This can greatly simplify the body to just:

public function effective() {
    return !($org = $this->org)->isGroup()
            && $org->isParentPropertyInherited() 
            && !($parent = $org->getParentOrg())->isRoot() 
            ? $parent->expiration()->effective() : $this->get();
}

This is not far-fetched since if the org inherits a parent's properties, it implies the parent exists.

Answer 2

This seems to disregard the nice and simple:

return complicated_test_1() && complicated_test_2() && complicated_test_3();

This is certainly simpler than either of your options, and I assume you rejected it because:

the complicated_test are not single function calls, but themselves Boolean expressions

But this is a self-inflicted problem! You extracted out those boolean expressions into methods in your question for readability, so why not do the same thing in your real code? Just apply an extract method refactoring and pick some more expressive names than complicated_test_n, and you're good to go. And we didn't even need monads!