re: Some advice to do a great code review VIEW POST

VIEW FULL DISCUSSION
 

Many postings on code-review explain what good code should be, refreshing to read a posting on how a code-review should be done.

On pull-requests: I'd go even further, many times you should not do reviews via pull-requests at all. When you develop code in a team, all code should be (re)viewed by at least one other team member, preferably by more. Pull-requests are not the only way to do that, and when you work in a team, where everyone is on-site or easily reachable virtually, pull-requests are an overhead at best, and ineffective at worst.

If you pair- or mob-program, you have code-reviews built in your development ( from this friendly answer on twitter ). But even if you program at your own, instead of creating a pr and continue on your next jira-ticket, you can ask a team-member to review your code directly. Ask your colleague to sit next to you, while you let him or her view your changes, or share screen if you work remotely.

 

I totally agree with you but I see some advantages to use Pull Requests:

  • I share my knowledge with all the team (not only my coworker who accepts to do a "live" code review)
  • I share the responsibility with all the team. If someone see in my code a reason to not make it live (put it on master) because of his own code, he can provide missing informations before the merge.
  • I ask for the knowledge of all the team. Everyone has his own skills and often it's not the same as the skills of his coworkers. I want to enjoy diversity.
  • I keep a reference on the review work and could read it again later even after margin the branch.
  • I can define hook and CI pipeline based on the pull request (to simplify reviewer job, sanitize the code and many other boring tasks)

I don't tell we shouldn't do IRL code review, peer and mob but do pull request review in addition of the other forms of code review.

 
  • Sharing knowledge is important indeed. Instead of github-prs I'd put it one a wiki, or post it on our internal (slack) forum, depending on what kind of knowledge it is. In my experience, comments in prs get lost at some point.
  • Verbal, direct communication shouldn't be avoided for the sake of keeping a track-record. Writing something down can help structure your thoughts and is of course a bit more permanent.
  • I can't imagine developing software without a CI pipeline. I am glad that's common-place now. Our CI includes compiling, unit-tests, integration-tests, build&deploy and complete end-to-end-tests.
code of conduct - report abuse