Best practices for code review

Hey guys. I’m the lead software on my team and I want to improve our workflow, and what I want to do is implement proper code reviews. What do you do on your team when doing code reviews and what best practices should I / the mentors follow? Thanks.

On another note, I also want to use the other features of GitHub like issue tracking, projects, etc. I know this isn’t necessarily related but do you have any pointers?

1 Like

For me, the minimum set of best-practices for “good” process:

  1. Each software developer always works on their own branch.
  2. Reviewed and tested software always gets merged promptly to a special, golden, integration branch
  3. Tags get put on integration branch nodes when a set of content is at a useful milestone. (including major feature additions, or software intended to run for an extended time ont he robot).
  4. Prior to merging to integration, code is checked for formatting and functionality. Run all unit tests if you have any.

Process-wise, I believe what needs to change must be established prior to starting work. This what is documented as a single Github Issue. The developer then creates a new development branch at the appropriate start point, making note in the issue which branch they used to resolve the issue.

My big key is that you don’t go changing random stuff in code that’s unrelated to your task. The branch contains the work required to resolve the issue - no more, no less. If there’s something else you want to do, make a separate branch and issue for it. It’s the discipline to prevent the “wiz-bang” attitude of “but look how much better I made it” - Better means a better-working robot, and all code changes need to be made with that mentality. Don’t play with stuff for the sake of playing with stuff.

Use the tags and milestones to categorize and group issues. For example, issues may be grouped by where the test should occur (on robot? on test bench? no test?).

After development and test, the issue should have any comments related to requirements changes or review notes, and should be closed out after merge.

It’s important to remember that, at the end of the day, all this advice is simply to help write better software. Focus on fixing pain points, and don’t put additional work in that adds no new true value. In addition, regularly review the process steps to ensure they’re all working as intended, and not causing unnecessary burden.

Some additional resources:
Git Flow - a recommendation on branching strategy in Git.

Semantic Versioning - a standard for how to ensure tag names are meaningful.

9 Likes

Wow, this is great stuff! Exactly what I was looking for, thank you :slight_smile:

1 Like

If you’re going to do code reviews, the main thing is to do them before you do any sort of substantive testing.

Why? Because the point of code reviews is to catch errors early in the process. Code reviews save time debugging later. If you do the testing and debugging first, then you miss that benefit.

That doesn’t mean that there’s absolutely no value in doing a code review AFTER you’ve debugged. But, the value is certainly less than if you had done the code review on the front-end.

2 Likes

No problem! One additional thought: successful execution of this in a team does rely upon good software architecture.

If you have to have every developer touch the same five lines of code for every change they make, you’ll quickly have a situation of people stepping on each other’s feet. Breaking down content into a properly-granular set of classes is what helps ensure every developer has a small “sandbox” they can iterate quickly in, and minimize the amount of time they spend thinking about “how does this change break someone else’s content”.

Too much breakdown, and you have to edit twenty files every time you make a small functionality change. It become hard to see what the code does while constantly hopping between files.

Too little breakdown, and developers have to edit the same lines and files in different ways, leading to merging nightmares and code duplication.

Picking the happy medium is nothing short of an artform :).

2 Likes

This is very true. The axiom I like to keep in mind - every time a line of code changes, it’s an opportunity to introduce bugs (especially with less experienced developers). If you write, test, review, and then re-write, you (arguably) have to re-test again to ensure no new bugs came in.

When using git, its also helpful to have a good git convention

Its not necessary, but definitely prevents the rookie from spamming “AHHHHHHHHHHHHHHHHHHHHHHHHH” in the commit message and helps with organization

1 Like