Introduction
This is the first article of a series I will dedicate to tips to write PHP code that is easier to maintain, review, refactor,... These tips may be applied for other languages but are mainly focused on PHP.
The first one could be entitled as "return as soon as possible"™. It may be summarized as changing:
if ($conditionToPerform) {
perform();
} else {
return false;
}
into:
if (!$conditionToPerform) {
return false;
}
perform();
The benefit of writing code this way may not be obvious, but take a closer look at the next implementations of the setPassword() function of an imaginary User class:
Update: please, don't focus on the questionable usage of the exceptions here, the goal is to provide different behaviors that exit from the function. Returns has to be understood as a family containing: return, throw, trigger_error, die, exit,...
function setPassword($password, $oldPassword) {
if ($password === $oldPassword) {
return;
}
if (!$this->checkPassword($oldPassword)) {
throw new Exception('Wrong password.');
}
if (strlen($password) < 8) {
throw new Exception('Password too short.');
}
if (empty($password)) {
throw new Exception('Empty password not permitted.');
}
$this->password = $password;
$this->save ();
}
compared to:
function setPassword($password, $oldPassword) {
if ($password !== $oldPassword) {
if ($this->checkPassword($oldPassword)) {
if (strlen($password) >= 8) {
if (!empty($password)) {
$this->password = $password;
$this->save();
} else {
throw new Exception('Empty password not permitted.');
}
} else {
throw new Exception('Password too short.');
}
} else {
throw new Exception('Wrong password.');
}
}
}
From this two implementations, we can see several benefits of the first one:
- it is easier to read
- reordering conditions is easier because they are not nested
- the link between a condition and the corresponding Exception is more obvious
- keep the indentation lower, which makes life easier with the limit of 80 chars/line
- less line are changed across revisions which decrease the number of SVN conflicts possible and easier the code review. Take a look at the following pictures showing the same change requests (doing nothing if old and new password are the same + checking that password contains 8+ chars) implemented with the two styles:
Using "Return as soon as possible"™
Using nested conditions
Posts
6 comments:
Good clear description of this best practice!
I once heard someone say that an OOP best practice is no more than 2 levels of indentation in your method, this practice would flow from that.
It's the classic reading v.s. writing code difference.
When you write, the method is in your head already so it makes sense to nest the dependencies, but when you're debugging or reading the code (which happens a lot more than writing) you don't WANT to read the entire method to find out the first if statement failed.
Your usage of exceptions is just plain wrong. You must NOT use exceptions in situations you expect, like to short passwords, but in exceptions to that situation. Throwing exceptions is expensive, and in this case just utterly wrong.
As for the article, I disagree. I like the single point of exit strategy and will use a variable to store my result and return it at the end. This improves the maintainability of the code, because maintainers wont have to search for obscure returns halfway your methods.
Am using this type of coding since two years now. Since then my coding style improved a lot and is more readable.
As "boy baukema" stated, it's in "your head" to code nested. But once you start to write using "Return ASAP", you'll progressively stop thinking nested :)
Now this is not an absolute rule. You still need nested conditionals. But keep them the least nested as possible.
For the surrounding code, let's say it's just an exemple, cuz ... setPassword throwing an exception for wrong password ... kinda strange.
Sure, throwing Exception isn't a good choice here but it's just an example showing that "return" may be understood as a family of event like:
return, exit(), die(), throw, break, continue,... This has nothing to do with explaining Exception usage.
This is clearly in the right direction, but having the single return point does make it better. Set a $validationFailed = false; at the top, then instead of exceptions or returns, change it to true, and set a message.
Then, check once at the bottom before performing the function. This gives you the single return point (which is great for debugging and logging) and also opens up the ability to return multiple messages to the user, so that they know their passwords didn't match and they were too short and they were dictionary based, etc.
Everyone should disregard Alex's comment, the article is not about the use of Exceptions also his method of storing a return value in a variable and waiting for everything to process rediculous and in his own words "expensive"
This is a great article by Patrick of exactly how to write clean code.
Return as soon as possible, clean up and move on.
Post a Comment