3

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?

5
  • 1
    other duplicates include: Approaches to checking multiple conditions? and Style for control flow with validation checks – user40980 Aug 20 '15 at 13:42
  • Side note: your return (t1 && t2 && t3) ? true : false; should be return (t1 && t2 && t3);, eventually without parenthesis if supported by the language. – Arseni Mourzenko Aug 20 '15 at 13:58
  • You've rolled up each conditional into the next but then you're still checking all of them at the end, which seems weird. Unless you have some reason to believe that the performance cost of not short-circuiting is significant, I'd get rid of that roll up. – Ben Aaronson Aug 20 '15 at 14:21
  • @BenAaronson Thanks, and yes, the cost of not-short circuiting is high. Fixed (by testing only the last one). – bishop Aug 20 '15 at 14:25
  • 1
    It might surprise you, but I find the "arrow" version very readable. – coredump Aug 20 '15 at 17:24
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.

3

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!

1
  • Thanks for your answer. Let me try this again. Please see my revision, which is a more specific and entirely different question. – bishop Aug 20 '15 at 14:11

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