What is “good code”? This question is probably as old as programming itself. Sometimes I forget how old programming is. (See my previous post “The Ten Commandments of Egoless Programming” a few years back, celebrating the 42^nd anniversary of this original list.) The discussion in https://github.com/magento/magento2/issues/1133 raised this question for me. My gut reaction was immediate – “I don’t like that code”. But why? Here is my “why” in greater detail.

(Oh, and if you are curious about the value of gut reactions, go read Blink.)

Example

/**
 * Get product status
 *
 * @return int
 */
public function getStatus()
{
    if ($this->_getData(self::STATUS) === null) {
        $this->setData(self::STATUS, \Magento\Catalog\Model\Product\Attribute\Source\Status::STATUS_ENABLED);
    }
    return $this->_getData(self::STATUS);
}

Is this a good or bad pattern for code? In general, it depends! But for this specific case it feels more ‘bad’ than ‘good’ to me.

When it is Good

If it is computationally expensive to work out the value for a getter to return, lazy evaluation is a useful strategy. That is, you work out the value only if requested, and you save it away in case someone asks again. If you never call the method it avoids computing the answer; if you ask several times you only work do the expensive computation once. This is absolutely a good reason to use this approach at times. It is a common and valid pattern.

When it is Bad

From a performance perspective, if the deferred work is cheap to do, then it is a net negative if the method is called at least once. If nothing else it has to do an extra ‘if’ statement. In this case, it also has to do several additional function calls. For example, it always calls _getData() twice. This is can become important if the function is called a lot. But frankly it is not my real concern here.

The real reason I dislike this sort of code is based on principles.

• The first principle it violates is a getter has side effects. The object internal state is changing by calling a getter. That is ‘strange’, unexpected. What if the setter is overloaded? What if it writes all changes to logs? It is not normally expected that a ‘get’ method will set the value.
• This code will also invoke plugins on the setter method. Do you think plugin writers will expect a setter to be called from a getter method? Should they be expected to check for null and setting to default values, so they can ignore the call? Yuck!
• If ‘null’ means the field has not been set, this might be useful semantics to other code. So instead of setting the field, just return the default. That is, if null, return STATUS_ENABLED rather than setting the field. The return value is just an integer in this case (not an object).
• Rules may change over time. Consider the previous point. When the class was first written, maybe ‘null’ is not used for special semantics. But maybe this proves to be useful in the future. It may be missed that the getter calls the setter, in which case the semantics of ‘null = value is not set’ would change, simply by calling a getter method.
• Immutable objects are less bug prone. Bugs are normally introduced by complexity. The more you can have an object fully set up at construction time, the less it needs to change afterwards, and the less buggy code using your class will be. There is less risk, for example, of some code keeping a handle on the object to access later, only to discover the object has changed. (This is one reason we tried to make data entities immutable. However, we found the problems this introduced outweighed the benefits, so we backed off from this decision.)

What is Good Code?

To me good code…

• Is readable. Simple code should be simple; “clever” code should be reserved for when it’s important. As soon as you see ‘if’ statements in a method, you need to start thinking about ‘why’ – what special semantics are there that meant an ‘if’ statement was necessary?
• Does not try to optimize performance too early. For most code, maintainability is more important than performance. (There are definitely exceptions.)
• Has a common set of principles it follows consistently. (Principles being more generic than just ‘patterns’.) Principles capture the ‘why’. It is much better to glance at code and be confident the pattern it is following being clear of ‘why’.
• Is designed to be changed in the future with change having a low chance of breaking something.

Like all patterns, there are counter patterns. This is where human judgement comes in. This this specific case I look at it and believe the negatives outweigh the positives.

Conclusion

So how important is this in this specific case? Not very!

I dislike it more because having it in the code base may lead to programmers following the pattern in other cases. The more it happens, the greater the chance of a bug slipping in, the less clear the code becomes.

But I am not going to lose any sleep over it.