Code Review Checklist

What should you be looking for? Here is an extensive (but not exhaustive) list of things to watch out for:

• Solving the problem: Well, if the solution doesn't solve the problem, what's the us of it?

• Magic strings: Avoid them entirely. Instead of writing text all over the place, create constants that you can reuse across the code; if you are using HTTP status codes, use a library instead of creating your own; and so on.

• Naming conventions: Check out for variables, methods, classes, etc that don't follow standard naming conventions. Conventions prevent us from chaos and inconsistency across our code base. Make sure everything follows our standards.

• Exception/error handling: They have to be treated properly. Catching a general exception and not adding information to the message is bad practice and should be avoided. Always catch the most specific exceptions instead of going for the general one. If you are just are catching Exception, or you not writing a good error message, you are doing it wrong.

• Commented code: Make sure there is no commented out code committed. Always follow the either use it or lose it rule. Also, check out for unused imports. If they are not being used, make sure they are not there.

• Release notes: Check out for a properly written release notes. It's not just an annoying piece of text that has to be written. It helps us track what has been done. Be mindful of other contributors and spend some time writing and checking the release notes. It's in everyone's best interest!

• Modifiers: Access modifiers (and others) are generally easy to understand but hard to use. A lot of people feel the need to make methods or variables more visible than what they should be just so they can test the code. That is generally a code smell. If a method has to be private, it has to be private. Check for such things when reviewing the code.

• Logging: This is a tough one. Log messages have to be meaningful. They have to contain as much information as possible. And they need to have the right level of visibility (i.e. debug, info...). If log messages are not meaningful, it is always hard to debug or find root causes for our issues. Check for log messages that say "I'm in this method" or "test" and have those deleted. Meaningful log messages should contain variable values, proper reasons for failure, actions that have been taken...

• Readability: One liner solutions make us feel proud. To solve a problem in a single line is cool. But in occasions it makes it hard for people to read it. Methods that are too long are hard to follow. Too many jumps between classes makes it is difficult to follow. Check for items like this and, if you struggle to follow the solution, flag it. It might need some re-writing.

• Dependencies: There are people out there that will include a whole library just to use one method. Sometimes people will be using different libraries to do the same thing (i.e. Jackson and Gson). Look out for versions as well, and make sure all projects are using the same (and latest) stable version of the dependencies.

• SOLID principles: SOLID. We all learned this in university. We all answered the question "what are SOLID principles?" during a job interview. Look at the code with SOLID eyes and make sure we follow these principles.

• DRY principle: If there is already a solution for the problem, check that it's being used. If the solution has to be used in several places, make sure all the common code is in one unique place. Check out for keyboard warriors that write the same solution time and time again!

• Design patterns: We are engineers, not craftsmen. Design patterns are known, repeatable solutions to multiple problems. We want to use them because we know they work; and they work well. Factories, adapters, facades... Check out for their usage!

• Code cohesion: A common problem that people face is "where do I put this method/constant?". The answer is always "where it belongs". But where does it belong? A good rule of thumb is "where the knowledge is". So the data and the operations should be sitting close together. If you see that there is data very far away from where the data is operated, that is a code smell.

• Version changes: Major, minor, patch. You know the drill: Major === breaking changes. Minor === new functionality. Patches === fixes and small improvements. If we are not following that, we are not doing it right.

• Indentation: Very simple. Where do the curly brackets go? How many spaces is a tab? Tabs or spaces? There is a standard. Make sure the standard is follow across the code