DEV Community

Discussion on: Code review checklist

Collapse
 
rafalpienkowski profile image
Rafal Pienkowski

Very nice list.

I think that list such as this are always handy. Fortunately I know that most of devs are lazy (and this is not a drawback) and I could add such rules but they won't use them if they don't have to. To avoid such a situation
when nobody follows such a list I'm adding it to my pull request policy. I always setup such as policies:

  • build based on feature/bugfix branch has to be succeeded
  • all tests have to be "green"
  • there shouldn't be any new code smells or code duplications after static code analysis
  • author cannot accept own changes
  • at least two developers are required to accept changes

I think that we should automate as much as possible and focus on things which can not be autometed like:

  • checking functional and nonfunctional requirements
  • checking if the code could be refactored
  • wear the tester hat (I like this term)

Last but not the least, always encourage all team members to actively participate in code reviews regarding their experience. Sometimes junior dev could find something interesting and there is also great place to knowledge and experience sharing.

To sum up, I think that this list is a great starting point for a pull request policy in a project.

Cheers.

Collapse
 
uday_rayala profile image
Udayakumar Rayala

The idea of the post was to bring common understanding to the team about what to look for in pull request reviews. Because everyone has their own understanding of what to check for in the code reviews, it is good to write down things to share knowledge and also act as an agreement. Such a list should be owned by the team and enhanced further.

I agree with automating as much as possible and such automated checks should be part of the build pipeline.