With all the advancements we make in programming, and all the new technologies we work with as Magento developers on a daily basis like Dependency Injection, code generation, etc and benefitting from IDEs like PHPstorm I tend to forget PHP, is in essence, still basically a language.
And as with any language in programming or in everyday life clarity of structure and quality improves performance and prevents misinterpretation, or bugs. Even without writing a single test for that code.
This article is written by Sander Mangel, Magento Certified developer at fitforme.nl.
At the basis of any amazing functionality you write or module you create should be the discipline to write the best, and highest quality code you can. Older or messy frameworks withstanding. The team I work with uses the same rules for the code it writes in Magento 1 as it does in SlimPHP or Laravel.
This strict style of dealing with code can be annoying at first. It takes more time to ship your code, it requires multiple iterations over anything you write and it requires you to think more about what you write. But over time you will learn to appreciate it, requiring strict type parameters or defining return types will let you code with confidence and every comment block and annotation will let you extend and refactor code more easily.
The purpose of this article is not to tell you what high quality clean Magento code should look like (ok maybe a bit but treat them as suggestions), it’s purpose is to inspire you to improve your, and your team’s code, to make sharing knowledge something that is at the core of your code reviews and to give you the tools to help you do all this.
Any code written by you or your coworkers is the product of the team. Of the way the team shares knowledge and a reflection of what priorities the team sets. If someone is taking shortcuts in the code he or she writes that means that developer felt inclined to do so because a deadline was prioritized over code quality.
As such this means a code review should not be addressed personally to a developer. Feedback should be directed at the team, it should examine why that code was written the way it was and not just state it’s wrong or bad.
Code Reviews are, in my opinion, the first step to improving your code as a team since it’s the whole team that participates in defining what it appreciates in code. Always try to write your reviews in a positive way. Point out code you like, encourage discussion on code you would personally like to see refactored.
And feel free to take the review beyond a simple comment on Github. Talk to someone face to face, sit down and walk through the code.
Set aside time to do reviews twice a day. Make sure people receive feedback in a timely manner in that way pending code reviews won’t halt work.
Once you start reviewing each other’s code you can also start working on common coding guidelines. Personally I prefer to do use them as guidelines and not rules. Code should, above all, work well and be practical to read and work with. It’s fine to write down if you want an accolade on the same line as the function but that should not be the focus, we’ll get to that later on.
Guidelines should tell developers more about how to solve common problems. It could give suggestions like “Try to use array_walk and maps where possible” or “Try avoid anonymous methods”, with bonus points for adding the why instead of just telling the reader what to do.
When dealing with frameworks like Magento I would also recommend writing some specific guidelines on how custom code should interact with various framework objects. We for example try to introduce bridges to act as an abstraction for code we write.
Make sure the guidelines evolve as your team, tech and ideas evolve. They’re not commandments, their guidelines. Put effort into refining and improving them.
Document your code
Now whether you do this in confluence, a wiki or as comments inline, documenting your code is a great way to improve overall quality. For me personally I like to use short, inline comments over confluence although I use both.
Inline comments act like a rubber duck. When you’re explaining your code in a storytelling fashion illogicalities or flaws in the thought process tend to stand out.
Equally when you need to write a comment or story every other line to explain what’s happening the overall complexity of your method might just be a bit too high.
And do be clear in your documentation. Assume that the next person reading it has no clue what this code is about or why it was ever written. Let’s be honest, in six months even you won’t know why you wrote that specific line of code.
Make code explicit
Explicit code is more readable, it will result in better typehints in IDEs, it’s easier to test and it’ll fail in a clear way when something changes along the line.
Type Casting method parameters and return types
Defining the type of a method parameter means you’re getting a hard guarantee. For simple values primaries like integer, boolean and string will do, try to avoid arrays as it isn’t clear what it should contain.
When dealing with more complex parameters like classes try to use interfaces instead of the class itself to avoid hard coupling.
When you do need to pass an array take a look if you can represent it with a value object. Again, since that object is explicit it guarantees you certain values.
The same goes for the return type which, in PHP 7, can be defined not only as a parameter will ensure the method you called will always return the expected value. Definitely a reason to switch from PHP 5.6 to PHP 7 on your hypernode!
Use value objects and make them immutable.
Passing address data to another class? Why not use a value object instead of an array.
Value objects define the data they contain by exposing getter methods.
Making value objects immutable means you have less to keep track of in your code. By only allowing assigning values in the constructor you’re sure the values you set are not mutated by an external factor.
Any class you write will have methods other code depends on. When you extend a class or alter it’s traits to deal with various cases throughout your code refactoring that class can become challenging. Using interfaces will allow you to define which methods and variables are required by code even when swapping out a certain class and will throw visible errors both in the code and the IDE when those requirements aren’t met.
Write small, single responsibility methods
What a method does should be clearly definable in just a few words. Single responsibility methods keeps methods small and makes for testable code. Descriptive method names means it’s easy to step through your code since it’s it tells you exactly what you’re looking at without reading to full code.
Let’s say we have a class that retrieves a collection of records from a database, loops through these records, validates the data per record and then writes it to CSV after which the record is deleted.
Now we could write this in a single method which would be about 50 lines of code or more. Debugging any part of the behavior, for example the validation of the record, will require you to write those 50+ lines of code to know which part does what.
But what if we had one method named fetchDatabaseRecords that retrieved a collection which was iterated calling the methods validateData, writeToCsv and cleanUpRecord I could skip directly to the validation method ignoring the rest of the code.
If you’re finding it challenging to split up code right away just go ahead and write code as one big method, afterwards using the refactor > Extract > method (ctrl + alt + M) option under the PHPstorm context menu.
So what would that look like as actual code? Check out my Github to see some basic PHP code refactored.
Use code sniffers
Code sniffers are a great way to catch small mistakes and improve overall code quality. Static code is evaluated against a standard set of rules that you can tweak to your own needs and preferences.
A must have tool that helps a lot during coding is the PHP Inspection plugin for PHPStorm. It gives suggestion right while you’re writing the code which does improve your code a lot.
It’s mostly the small stuff like method comments etc but it’s easy and quick to fix.
PHP Mess Detector
The PHPMD mainly looks for sub optimal or over complex code, next to that it will give you feedback on syntax like unused variables, naming of methods and a host of other things you can find in the rules list.
Now I would suggest tweaking this to your needs since, especially in Magento, a lot of code might not pass certain rules.
I would use at least Npath complexity, Cyclomatic complexity and Excessive method length as those 3 do force you to write clear and easy to read code.
For Magento 1 the Magento ECG published extra rules you can find on Github.
For me this ruleset works best for basic PHP projects (SlimPHP, Laravel). While the following is more tweaked to work with Magento 1’s coding standards
Add the rules in a file called phpmd.xml in your root and you’re good to go.
Next to that the PHP CodeSniffer runs tests mostly on syntax and proper formatting of code. Again, I recommend you to tweak this to the way you like to write your code. If you use it out of the box this will likely frustrate more than do any good.
Use this sniffer together with the fixer tool to quickly resolve any detected issues.
I love to use code analysers, they made me a better developer for sure but at the same time they do have a downside. Since the rules are pretty strict and analyze code without much context they can force you to refactor code in a suboptimal way just to pass the test. Sometimes having a slightly higher npath complexity in a method while the code is still clearly readable isn’t that bad.
For that reason I prefer not to include them in any CI build test but use GrumPHP to run them as a pre commit hook. GrumPHP will show any configured tests that fail but, in my opinion, at the end of the day it’s up to the developer to determine if he or she should refactor the code or ignore it if there is a good reason to.
Here’s a basic grumphp.yml configuration that runs phpunit, phpmd, phpstan and phpcs:
Although these are some of my favorites there are tons more out there and with libraries like PHPStan, a library that parses PHP code into objects to be read and manipulated, you can easily write your own tests or other tools. Check out this documenter I made for Magento 1 observers for example.
Please note that in Magento 2 many of these libraries are already present. This means you can already run them on your codebase and install GrumPHP to let them scan your commits.
Wrapping it up
We’ve gone through several different subjects here and I do hope at least some of them appealed to you for taking a first step into improving your, and your team’s code. Treat it as a game and don’t try to force it. Above all else writing code should be fun and improving quality an inviting challenge.
If you have questions above any of the above mentioned topics feel free to contact me via twitter.com/sandermangel, it’s definitely the best place. Or attend my “Basics of Magento development” course at Byte on the 6th of October in which I’ll not only discuss these topics but also others that any modern PHP developer will find helpful.
More information about the course “Basics of Magento development” at byte.nl/events.