50 Things I Look For When I Do A Peer Code Review

(C) icodejava.com – Clean, well formatted, readable code.

I work as a tech lead for one of the fortune 500 companies, working with multitudes of developers on various projects. Some of these developers are fresh ones, others have come of age. Some of them are full time associates, while others are contractors who have seen various faces of software development styles at various other companies. In the end, everyone has their own coding styles, their own way of defining variables, their own way of writing classes and methods. Nobody purposefully wants to write bad code. Ultimately everyone tries to be a better coder.

Having said that, due to the varied nature of developers and their coding skills and styles, it becomes an important part of my role as a technical lead to make sure that the code that we are producing on a day to day basis are consistent, easy to read, are not nests for bugs, and are maintainable by people other than the ones who originally wrote them.

We work in an agile environments, pair programming kind of tends to do a first degree of code review by itself. But at times, I find myself still getting involved in a formal code review. Having worked on the enterprise application for several years, I do have a bigger picture of the application that some developers who are fresh hires. Coding clean is my passion. So when I start looking to peer’s work, I focus on certain standards of coding. Most of these standards are agreed upon by the team I work with and are implemented via static code analyzers such as Checkstyle. But it becomes necessary to remind to the new hires about these standards we follow. In this article, I am sharing 50 major things that I look at when I do a code review of my peer,co-worker, team mate.

  1. Does the code have proper error handling strategy? The common one being for the null pointer exception.
  2. Has the source code been properly formatted?
  3. Does every new method that is written have at least one unit test?
  4. Are there abbreviated variables or method names? I discourage using those. Variables, method names and class names must be meaningful.
  5. Are there any hard coded string literals? The string literals should be defined as constants.
  6. Is the method too long? Does it need to be split?
  7. Is the code efficient? For instance, are there any break statements in a for loop?
  8. Is the code repeated? If it is, then it’s time to refactor the code. Apply DRY – Don’t Repeat Yourself principle.
  9. Is the method having too many return statements?
  10. Are there too many if else conditions?
  11. Are there nested for loop? Every time there is a nested for loop, I recommend splitting that into two or more methods.
  12. Are there any default comments which add no values?
  13. Are there any TODO statements?
  14. Are the lines too long?
  15. Is the code consistent with rest of the code?
  16. Is the code readable?
  17. Is the code easy to follow through?
  18. Is the code maintainable? If someone else were to read the code and maintain it few years later, would it make sense?
  19. Are the resources, such as database connections, properly closed?
  20. Are there any tab characters? We have enforced to use spaces over tab characters in our team.
  21. Are there any unused imports?
  22. Is there any code with unhandled code warnings?
  23. Does the code have unused variables and methods?
  24. String Comparison. Does the code do CONSTANT.equals(variable) or varibale.equals(CONSTANT). The latter one can throw null pointer if variable is null. The former one does not.
  25. If you use static code analysis such as checkstyle, does the code introduce new warnings or errors?
  26. Does the code fail any unit tests?
  27. Are the variables public? variables must be private and have abstract methods.
  28. Is the file too long?
  29. Are there any empty methods?
  30. Do the methods have too many parameters?
  31. If the constructor definition in wrong order?
  32. Does the code catch the Exception class instead of the actual Exception such as NumberFormatException?
  33. Are there too many conditions being checked in an if statement?
  34. Are the variables and constants defined in correct order according to their visibility (public/private/protected/default).
  35. If you work on an agile environment, was there any pair programming done?
  36. Does the piece of code need performance testing?
  37. Is your method small? Does it do one thing and just one thing at a time based on your method name?
  38. If the code exposes any interfaces or APIs, are those clear and minimal?
  39. Is the proper level of abstraction used?
  40. If the code modified any existing logic, did the developer fix any existing issues such as abbreviated variable names in that method?
  41. Does the code use magic number such as 2,3,4 without stating what they mean?
  42. Does the code use magic strings such as “cp” (which could actually mean copy command) without stating what it means? The solution is to define description strings and use it, such as String COPY_COMMAND =”CP”;
  43. Does the class use a default package?
  44. Are there any variables defined within loops?
  45. Are there any variables defined that are used only once?
  46. Does the code use shared variables? Is the code thread safe?
  47. Is there any commented out code?
  48. Is the logging done properly, are there any left over System.out.println?
  49. Is the code clearly documented as needed? If the code is readable and flows through well, the needed documentation will be significantly less.
  50. Does a Switch / Case statement have a default?