Translate

Sunday, November 20, 2016

Common Java Code Review Comments

I was assigned to work on a mobile project where the team does not practice code review.  Everyone in the team (16 developers) can merge the codes that they want.  No unit tests or integration tests were written to verify if their changes were correct.  Good thing they have a Quality Assurance (QA) team but only to test the mobile app and collect the list of bugs.
To make things worse, the project will be released to Production in two weeks time.

I asked at least 4 team members on how can I contribute to the release.  Each one is pointing to one after another.  They don't know what I can work on.  I even get the impression that they think I am slow.  It will take me some time to get to speed.  I may look young (and pretty) but I tell nobody that I have been working professionally as a Software Developer for almost 2 decades!  Oh well, people in the IT field can be sexist sometimes.

So being in this new team, my co-worker who also came from my previous project imposed more strict code review process.  I came up with a cheat list so that I can have comments in each pull request submitted by developers.
  1. Update Javadoc
    • If method has been renamed, make sure that the documentation has been updated.  If there is no Javadoc, ask the developer to add one.
    • When method parameters have changed, make sure to update the Javadoc too
    • Return object was changed
    • The logic has been changed
    • Missing Javadoc
  2. Set the correct access modifier.
  3. Do not save unencrypted passwords in the repository
  4. No unit tests
  5. Incomplete unit test - Unit test is testing only happy path scenarios.
  6. Too verbose logging
  7. Too deep if statements
  8. Eating exceptions. Not catching exceptions
  9. Returning null
  10. Not checking for null
  11. Duplicate codes. Copy-pasted codes which can be reused.
  12. Spaces vs tabs
  13. Too generic error messages
  14. Class names are not descriptive enough.  I even encountered class names such as Add, Remove and Rename.  Those are action words and those classes are the objects that are supposed to be added, removed or renamed in the application.
  15. Wrong spellings, incorrect grammar - I may not be fluent in English but I am a GrammarNazi when it comes to coding.  Working with developers of different cultures and educational background can be challenging.
  16. Description of the pull request
  17. Returning a very huge object.  This is same as logging the very huge object.
  18. Incorrect logging level
  19. Assuming that the project uses Spring Framework, change if (list != null && list.size() ) CollectionUtils.isNotEmpty(...).  Use StringUtils.isBlank(...)
  20. Unhandled or incorrectly handled exceptions
I hope this list can serve as guidelines to Java programmers who want to participate in their team code reviews.