DEV Community

Some advice to do a great code review

Nathanaël CHERRIER on October 29, 2018

I work with Git and tools like Bitbucket, Gitlab, Github... I know my team work hard to do a great work and follow the Gitflow development workflow...
Collapse
 
gerbrandvd profile image
Gerbrand van Dieyen

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.

Collapse
 
mindsers profile image
Nathanaël CHERRIER

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.

Collapse
 
gerbrandvd profile image
Gerbrand van Dieyen
  • 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.