“Code review is systematic examination … of computer source code. It is intended to find mistakes overlooked in the initial development phase, improving the overall quality of software.”
Code review, in summary, is a collaborative process between the developer who writes the code and the developers who review the code to ensure that new code is written in a robust manner. Robust programming results in software that is easier to maintain; easier to modify; easier to troubleshoot; less likely to fail; more secure; and less likely to create conflicts with other components. A large aspect of code review is to ensure that new code adheres to established coding standards which are refined over time to increase the quality of the code.
There are hundreds of articles on coding standards used in Magento. We are not going to go into detail on them since what those standards are is irrelevant to how to apply them in the real world. A website built on an open source platform such as Magento will also include a large number of third party modules which extend the functionality of the website. They will also include a customized child theme to provide site-specific features, as well as additional custom extensions to address feature gaps.
Therein lies the problem – we are dealing with potentially dozens of different companies, each producing code based on coding standards specific to their programming team. In addition, coding standards evolve over time! And it is extremely rare to find any project which goes back and rewrites thousands of lines of existing, perfectly functional code just to make sure it adheres to the current standards.
In order to extend and enhance the functionality of a Magento website, it is frequently necessary to override existing Magneto and third party classes, as well as customizing existing presentation files [phtml files in the template]. When making these changes, we strive to only make the barest minimum of changes. Most of the time, doing this ensures that the changes are “upgrade safe” – if Magento or a third party extension is upgraded, our changes will continue to function. In cases where our changes are not completely upgrade safe – since we minimized our changes it is relatively easy to compare our custom code to the third party original file and to the new third party file in order to integrate these changes. If we had completely rewritten the vendor’s code to match our coding standards, it would require many additional hours.
The three links below explain and include information that our team uses as a guideline for Magento coding standards, and is in general where a Magento developer must base their coding standards.
This is an overview of my experiences with code review in the real world – as opposed to shiny theoretical absolutes.
At Shero, we use Jira to manage issues and Bitbucket to manage and review the source code. Most Code Review processes require one or more individuals to act as the Gatekeepers. The Gatekeeper’s job is to say “no”, over and over again. This means the gatekeeper’s job in practice is to be a jerk. He can be a nice jerk…or a complete jerk. The gatekeeper is also generally seen as an obstructionist and is under a lot of pressure. Relax the standards and let broken code get through – be fired. Maintain impossible standards and let almost no code through – be fired. Every gatekeeper has a list of code practices to avoid which seem irrelevant to everyone else but are based on having spent days fixing problems because of that issue.
Based on the stress of that job, I try not to start off any reply to a rejection with a list of everything I think the Gatekeeper is being not serious about. Instead, my real world process of dealing with any code rejection is, FOR EACH rejection:
First, take 30-45 minutes and fix everything that is simple, regardless of how serious it is. Change the spelling of a word in a comment, sure. Add a one line description to right before 6 lines of code that are screamingly obvious what they are doing? Ok, implement. Then update my request with those changes.
Only after showing I’m willing to implement changes regardless of my feelings, I then go through the changes left. Explain things I think were not understood. For items where I understand what was asked for, but it would take a lot of time to implement – note that it will take a good bit of time and OFFER TO ASK THE PM for that time. For example, I take responsibility for letting the PM know more time is needed. My experience is that the response for those items is split pretty evenly between:
- The Gatekeeper insists on it anyway, and I’m no longer involved in the discussion since it is now between the PM and the Gatekeeper.
- The Gatekeeper decides to let it pass this time.
- The Gatekeeper says “why the heck would it take 3 hours? All you have to do is use these 3 lines of code!” – which I then implement and test and am done in 5 minutes
- The Gatekeeper says “wow, I did not know it would take 3 hours, ago we will pass it for now”
- The Gatekeeper says “why the heck will it take 3 hours, you can do that in 5 minutes” – at which point I pass it on to the PM and then the Gatekeeper either has to provide this mythical 5-minute solution or admit they don’t know how.
In addition, always make sure to factor in testing time for your estimate. Adding a configuration option to an extension will take less then 15 minutes. But TESTING it can take over an hour. You have to reset your local, Vagrant or Docker, dev environment. Change the configuration in the admin. Flush the cache. Perform the test. Then change the configuration option to a different value in the admin. Flush the cache. Perform the test. A bad Gatekeeper will often only specify the “less than 15 minutes”
As an added rule of thumb:
- No one should ever review their own code. Even if the other reviewer does not really understand that code[frontend vs backend] have the other reviewer look it over since they WILL recognize short cuts in commenting and such.
- The Gatekeeper’s code should be reviewed by the other reviewers. Gatekeepers are just as prone as anyone else to taking short cuts to be done sooner. Having their code reviewed by the people they have been rejecting code from ensures that they follow the same rules as everyone else. Nothing can get a stupid decision/rule thrown away faster than making the person who made up the stupid rule follow it.
Finally, the gatekeeper does not have the last word! The Project Manager has the last word. The PM can overrule and accept any request! That does not mean the gatekeeper should change their decision, rather it should be released anyway with the objection on it. Take a look at the percentage of overrides every 6 months or so, only for overrides over 6 months, and if most of them did not cause any issues, the rules need relaxing. If there have been some major problems due to those overrides, then stop making so many. At the end of the day, the PM is responsible for any override and the Gatekeeper is responsible for any breaks due to code they passed.
A recent example of this was when we upgraded a website from Magento EE 1.13.1 to Magento EE 1.14.3.2 – in part to fix a number of issues with products disappearing from the website after being updated. On the newsletter subscription forms, there was a single, one line change to add the Magento form key. Due to this change, it was no longer possible for a customer to edit their subscriptions. Because the changes had been kept to a minimum, it was simple to identify the one line change that was required and implement it.
Whenever customizing third party code, our developers make the minimal number of changes that are needed. During code review, we will review the original code against our coding standards as well as our experience to eliminate security issues as well as rewriting original code that we know can cause problems. Frequently the original code is itself fine if it was to be deployed on a website with no other third party extensions. Most websites, however, will include modules from multiple third party vendors, and interactions between these extensions can cause unforeseen difficulties.
For example, there are frequently issues with SAS subscription modules and other third-party extensions which enhance the way shipping and payment processing is handled in Magento. The collaborative process between senior and junior developers during code review allows senior developers to ensure the released code is robust, as well as transferring the knowledge they have gained through experience to others.
Systems Manager
