When doing a review of a pull request I try to follow a list of common errors, mistakes or shortcomings. There are two main categories:
- Cosmetic: code style, formatting and readability
- Substantive: functionality, architecture, etc
Let’s dig into them.
Code cosmetics
Those issues are usually trivial to fix, so I assume they shouldn’t be even present in the pull request in the first place. If there is no time pressure I require the OP to fix them, since there’s no justification for their presence, and a fix is a matter of minutes.
Readability
If your code is hard to read it probably needs a refactor. Extract large methods into smaller ones, change your architecture to be more straightforward. There should be only a limited number of places where a really complicated code is needed. All other cases are just bad design.
Conform to coding standards
Every company has — or should have — a coding standard. If you’re to busy to create your own just borrow one, for example from Symfony or even better from php-fig PSR.
Next step is to configure your IDE to format code automatically and maybe even reformat all files before commit.
Above all, learn those standards and don’t try to be smart. It doesn’t matter if some other formatting is more readable, more jazzy or less lame — the higher value is a common standard (not necessarily the best).
Naming conventions
This technically is a part of the coding standard, but it’s big enough on it’s own. Don’t break the patterns. If your Query classes have filterBy… methods — stick to them. This particular scheme ensures discoverability for other team members.
If classes or methods need to be more verbose — change them. There’s no reason to shorten them (they’re autocompleted eiter way).
Commented out code
You’re using version control. There is no justification for „saved for later” chunks of code. They only confuse when refactoring: does this chunk even work? Should I refactor it? What if someone would like to uncomment it in the future? Just drop it already. And use reverts if necessary. Learn how to use your VCS.
Inspect your code
Pay attention to PHPStorms inspections. If the square isn’t green — there’s a reason for that, and it’s more probable that it’s you that fucked up and not the IDE. There aren’t many cases when inspections cannot be corrected. And if you’re sure you’re right just add @noinspection.
Document smartly
Not every method needs to have docblocks, but usually your package-level services do. Documenting getters / setters is usually just noise, unless there are some gotchas. Don’t forget to comment any non-trival code paths to warn future visitors („do not refactor this code. Total hours wasted here: 42”).
Clean up after you code
If you overload some functionality — remove the old remainings to avoid confusion! Remove unused classes, templates, css declarations, database tables, etc. Remember — when using version control nothing is really lost!
Code function
This may include large refactorings or changes in architecture. You should always evaluate each issue separately. You don’t need to refactor the entire feature if it’s not essential or just a proof of concept for futherer improvement.
Check for logic errors
This depends on each case, but try to think about the implementation before you look at the code. If something in the code differs from your mental model there should be a warning triggered. Talk with the OP about it.
Avoid copypasta
Look out for copied code out of lazyness. Usually people don’t care about code duplication, but it may lead to hard to trace bugs. When moving functionality from legacy to project core make sure to implement it the best you can and replace the legacy usages with your new lib/bundle/etc.
Don’t use deprecated / legacy code
It’s deprecated for a reason. That code should be removed, not the other way around. Find a replacement in newer codebase. If there’s a whole legacy platform in your application, try to avoid it altogether for the same reasons and use only state of the art practices.
Reject God Object
If you see a fat controller, command or similar — ask to extract that functionality into services. Even the trival business logic refactored into a separate class / bundle will profit in the future. Not to mention the readability of the
Package visibility
Take care of what methods and services are visible outside. You may need a lot of services inside your bundle, but there are usually only few required outside of your bundle. Same goes to methods. All methods should be private/protected by default. Same goes for sevices, use private scope. Publish only services exposing some high level API for your lib.
Think about performance
There are some common pitfals when it comes to performance. Try to memorize them and make quick decisions to avoid creating bottlenecks. I’m not talking about premature optimization, but if there are many ways of acomplishing your goal without noticible side-effects — pick the most effective one.
Ifs and switches…
A lot of conditionals are a strong sign of a badly written code. Try to use a design pattern, abstract the logic from the interface, etc.
Avoid magic numbers and use constands
Instead of using magic values use more descriptive class constants. Utilize them as well when refering to the same value in same context in different parts of code.
Use DTO structures
An object is usually better than a hash. IMMV.
