Anti-Pattern: Assign in if

In a lot of the code reviews I do, I see the same pattern coming back again that I want to remark on every time I see it.

The pattern looks like this:

<?php

if ($foo = Model::find($id)) {
  // Do some things with $foo.
} else {
  throw new \Exception('...');
}

This code exhibits 2 things that can be improved. We don’t need the else branch and it also does the assign-in-if. I think this is bad because:

1. It’s hard to find where the assignment of the $foo parameter happens when scanning the code.
2. At first glance, it looks like there was a missed strict call. This dissapears when using yoda if statements troughout your code, but I don’t find those very readable either.
3. There is no strict check of what is actually happening. We are falling back to null being casted to a boolean, which is falsey.

When rewriting this, I think the code reads a lot better if it is structured like this:

$foo = Model::find($id);
if ($foo === null) {
  throw new \Exception('...');
}

// Do some things.

1. We now instantly see the assignment.
2. There is a strict validation on what the falseness of $foo is.
3. We could eliminate the else branch.

I think this pattern, where we remove the assign-in-if, makes for more readable code. Because we read code more often than we write it, as long as there is no noticable performance difference it is best to optimize for readability.