Recently, I was hired to refactor a huge codebase that was messed up for years. The project was a Magento 2 project with more than 30 custom modules. The modules basically did everything you shouldn't do in Magento. I didn't find any tests or documentation. No static code analysis, nothing. The fact, that there aren't any tests is the reason for the messed up code base.
While I was studying the code, I stumbled over the same ugly patterns over and over. This article is a small summary of the 5 worst patterns.
#1 - Chained Function Calls
Chaining... Look at the following:
Chained function call are hard to debug, harder to read and difficult to understand. While debugging, you have to evaluate each of the function calls or step into them instead of just jumping over them...
A better approach would be:
#2 - Yoda Style
It might be a preference of some, but for me yoda style is one of the most unreadable style ever. Maybe my brain is not fast enough to evaluate the condition, maybe yoda style is the pure evil.
Here's why:
Do me a favor and stop doing it <3.
This feels so much more natural:
#3 - Missing early exits
Fail fast, fail loud. That's a common statement when writing public functions. I often see functions doing a lot of stuff before they fail because a given parameter has a value that was not expected. Fail fast, fail loud. Validate incoming params first, fail if they're not matching the requirements of the functions, otherwise continue;
Invert the conditions and add early exits.
#4 - Unnecessary else statements
When an if block is guaranteed to exit control flow when entered, it is unnecessary to add an else statement. Exits can be returns, throws, continues, breaks.
I would say, that else-statements aren't necessary in most cases. Isn't the following code much better to read?
#5 - Naming...
Old but gold. People still give a sh*t about how they name things. Naming is hard, I know. Nevertheless, here are some rules of thumb for many use cases:
-
Suffix Class Names with their purpose.
- CreateCustomerAccountService
- ImportProductManager
- ProductToArrayMapper
- ProtectHttpConnectionsGuard
- DateTimeHelper
-
Try to find meaningful variable names and add the following names to your variable-name blacklist:
- $data --> $productData
- $params --> $requestParams
- $model --> $product
- $message -> $userFlashMessage
- $id --> $cartId
- $item --> $cartItem
- $connection --> $redisConnection
Explicit function names. We all know, that our functions shall only do one thing. This in mind, there can be functions similar to each other but having different interfaces. Let's imagine a CustomerMapper Class, mapping customer data. Methods can be named like:
mapToArray(Customer $customer)
mapToJson(Customer $customer)
mapToArrayById(int $customerId)
mapToJsonByEmail(string $email)
-
For sure, we all work with return types nowadays but it's still a good idea to declare function names that return a boolean like so (Thank for the hint Pujan Srivastava)
- hasStock() : bool
- isGuest() : bool
- canBeAddedToCart() : bool
Disclaimer: Snippets are meaningless examples.
Have an additional tip? Looking forward to hear it
Top comments (10)
I agree with all the point except for the first one. Chained Function Calls, there is no need to use extra memory space when we can use chaining.
But again, its individual perspective. Nice article.
Hi Mohd,
I think now a days, we don't have to worry about memory. I would prefer to be able to read, debug and understand the code instead of having to worry about memory.
But for sure, I am talking about PHP. It might makes sense to worry about the usage of memory in different languages.
No offense, it's just I have a personal rule not to create an unwanted variable until that particular value is used more than ones.
You're right, not every function call must be wrapped in a variable. It always depends, right? 🙃
hell yaaa...
In the naming convention, whenever I use boolean variable I prefer to prefix is, has, can. For example
const isPerson : boolean = true;
const hasAge : boolean = true;
const canDance : boolean = true;
Good one! Added it to the post. Thanks
Thanks for the great tips!
I improved a lot my code quality and legibility after read Clean Code and some things about Object Calisthenics rules. Some of the tips in your article match exactly with this topics.
As Martin Fowler writed:
In the number 4 you can just return the conditional:
return $customer->hasId()
right?
As for me, lots of factors can influence to write clear code.