When writing code it is easy to think in a logical way. With conditionals, people often write it as they would think it. If this condition is true then return this value, otherwise return a different value.
However there is a trap that people often fall into and one simple refactor can make the code more readable. If you have this conditional within a method or function that is returning a value, you can often just remove the else condition because if the condition is true it would never hit it in the first place.
Consider the following code:
public function getName()
{
if ($this->firstName && $this->lastName) {
return $this->firstName.' '.$this->lastName;
} else {
return $this->firstName;
}
}
This is a perfect example for refactoring.
You will notice that if the user has a first and last name, we return it. Otherwise we just return their first name.
However the else condition is redundant because we are already returning a value if the condition evaluates to true.
Therefore we can refactor this method to remove the else statement.
public function getName()
{
if ($this->firstName && $this->lastName) {
return $this->firstName.' '.$this->lastName;
}
return $this->firstName;
}
By doing this, it removes a level of indentation which makes the code more readable. It's a useful tip to remember and one that I find applying all over the place.
Originally Posted on George Hanson
Top comments (8)
The statement it removes a level of indentation which makes the code more readable is too generic. Not always "less indentation" means "more readability". In some cases, the code is losing balance which could lead to less readability.
In your example without the "else" part, the code looks like the "if" condition being an "exceptional path" (early return) and the rest of the code the "normal path". Was it your intention?
It is not always the case, that is correct. But there are a number of cases where this issue can prop up and it can lead to more readability. Personally I prefer that if code doesn't need to be there then it shouldn't be there. It often allows you to read what the code is doing at a quicker pace.
In this specific example, the "if" is actually an exceptional path. Considering the context of Mononymous people.
Good basic tip. Else-block is unnecessary in most cases.
I have to agree with others. I don't like the multiple returns, unless it's the early return because of an error.
I would prefer something like this(I don't know what language it is, so just guessing here):
This clearly reads top to bottom, GetName:
Name is firstName.
If lastName exists, name is firstName plus lastName.
Return name.
It's PHP.
It depends on the developer. I don't think early returns are much of an issue personally, depending on the use case. If there is an error, I would expect an exception to be thrown instead.
totally agree. less code to read, less indentation, less complexity. I am not a fan of multiple returns because they could easily be overlooked - especially "if block has a fair amount of code" as stated in other comments. But, if method is big and has a fair amount of code maybe it makes sense to split it into smaller bits and have the main method just deal with the necessary logic and return as soon as possibile.
Returning as soon as possible also means easier debugging because you dont have to go through the entire method to be sure your temporary variable is being modified by some other if-else down the chain.
Sometimes, yes. But generally, I don't agree. Omitting the else statement can be confusing to readers, especially if the if block has a fair amount of code (including other branches with return statements). I usually include the else statement for readability, except in trivial cases such as your example.
Like anything it all depends. There are cases when it is useful and others where it is not. In a simple case like this, I think it reduces visual debt. But it's all up to the individual developer.