Use isset() and empty() wisely

This is another part of the series "How to write better PHP code".

Sometimes it is the small things than can make all the difference. In this case simply using the correct way of checking
for variable content can avoid quite a few bugs in the long run.

What is the issue here

Very often people abuse empty()/!empty() and isset()/!isset() where they shouldn’t. I have seen this in many frameworks,
and in retrospective also in my own code for quite some time.

The main problem is that it does two things at the same time:

• Checks if that variable exists at this point in time, if not silently bail early
• If it exists it also checks of the content is not empty or set

Now, that sounds like "so who cares" – but it in fact has quite some impact in the correctness of code, especially when refactoring is involved.

Concrete issue

There was an actual bug in the CakePHP framework because of this.
The class attribute $compiledRoute was renamed, but a single use of that one was checked with !empty(), so that part of the code inside this check became unreachable and dead forever. Since this one piece of code didn’t have a test for it, it also remained dormant for a very long time.

Buggy behavior with methods

Since PHP 5.5+ it is now even possible to check expressions and more, so !empty($this->foo()) would be possible.
This should never be used though as this can have side-effects depending on the implementation of the class and is absolutely not necessary.

So what to do

The basic rule is: Always prefer non-silent checks.
Also always try to compare with === checks and expect the variable or attribute to be declared at run-time.

Only ever use empty()/isset() if that is not possible, that means if there is a chance that the variable or attribute could be not set at this point in time.
Same goes for array keys and alike.
But for variables and attributes you might want to ask yourself it that is maybe not a code smell after all if you do not know that for certain.

Luckily IDEs these days provide a lot more help than a few years ago, but there is still lot of room for error.
Let’s look into more concrete examples now.

Examples

Method arguments

public function read($key, $context = null) {
    ...
    if (!empty($context)) {
        // Do something
    }
    ...
}

What happens now if you refactor $context in the signature, but forget the one inside the if check?
Using this approach would have 100% identical behavior while making sure you couldn’t have forgotten it:

public function read($key, $context = null) {
    ...
    if ($context) {
        // Do something
    }
    ...
}

Ideally you would also look into more strict checks if applicable, but this is already way better than before.

Class attributes

protected $foo;

public function read($key) {
    ...
    if (isset($this->fooo)) { // Notice the typo
        // Do something (oh no, this will never happen now)
    }
    ...
}

While coding, we made a typo and never notice it. If you also don’t bother to write a test for this case, you might only discover it very late in the process, if at all.
So it should look like:

protected $foo;

public function read($key) {
    ...
    if ($this->foo !== null) {
        // Do something
    }
    ...
}

Renaming only one of the two would immediately trigger an error and the developer gets notified about his coding mistake.

Array keys

protected $config = [
    'recursive' => false
];

public function read($key) {
    ...
    if (!empty($this->config['recursive'])) {
        // Do something
    }
    ...
}

We can see that the key must exist. If not, we screwed up. So why using the wrong check that would only cloak spelling mistakes.

Better code like:

protected $config = [
    'recursive' => false
];

public function read($key) {
    ...
    if ($this->config['recursive']) {
        // Do something
    }
    ...
}

Optional array keys

public function read($key, array $options = []) {
    ...
    if (!empty($options['recursive'])) {
        // Do something
    }
    ...
}

In this particular case the recursive option is optional, thus the key could be empty here.
This would be OK.

One way to maybe improve this would be to merge the $options array with some $defaults which contain all required keys, then you can also use the proper non-cloaking checks again.

More examples

Don’t wrap methods, especially for "object or null" return values:

// Bad
if (!empty($this->getSomeObjectOrNull()) {}

// Good
if ($this->getSomeObjectOrNull() !== null) {}

I also often see the redundancy that makes no sense:

// Bad
if (isset($x) && !empty($x)) {}

// Identical (but maybe we also don't need the !empty here?)
if (!empty($x)) {}

// Better (maybe even strict checks?)
$x = null;
...
if ($x) {}

Usually variables should not just sometimes exist. Declare them as null first if you conditionally populate them somewhere.

Bear in mind here, that 0 and '0' would not work with either way and in that case you need to be strict in your checks anyway.
There also many buggy comparison cases around the 0 integer and a reason more to try to be more strict in general wherever possible.
That includes in_array() by the way, one of the functions probably most (framework and project) bugs have been discovered the last months due to this implicit casting issue leading to false positives.

Further reading

There was an RFC discussion in CakePHP around the same issue, as direct result of a bug that would never have been there if the above was applied properly.

Please share your thoughts on the topic as comments.