Learn to code review

So sometimes now I’m being assigned to review some PRs in diaspora. I can think of some general ways to do that, like

  • reading the code
  • thinking of questions about the PR that are not clear to me
  • checking design patterns and methodologies used
  • running the code

However I feel that I can miss something and I don’t feel very confident about the quality of reviews I make. For example @supertux88 writes quite good code and I never know if it is just good enough that I can’t find any issues or my review isn’t good enough and I don’t notice some issues.

How is it possible to learn to make better reviews? Can we think of some guidelines?

3 Likes

It’s a very interesting question, and the answer also could help me to get better :wink: Experimented developers, share your experience :wink:

What we need is that people really have a look upon the code. Pronto is that sort of syntax nazi tools which scares off new developers with little additional value to the real code quality. Clean software design is more important than indentation and bracket rules. In my experience as a professional software developer I realized that it’s a good approach to start with a design concept before beginning with any coding. Show that design concept to other developers, get your feedback and then start. This avoids to going in the wrong direction. Other people may have other ideas. Another important aspect is the testability of code. The more you focus on developing against interfaces and using small reusable and interchangable modules with single responsibilities (e. g. a Writer interface, which concrete implementations allow you to write to a file, the spool or the frontend), the more testable is your code. Testable code automatically leads to readable and maintainable code. I often do code reviews of unreadable and unmaintainable code. Refactoring is often the best way to deal with this (as long regression tests are available …) Just don’t try to enhance existing garbage code, when you have the feeling that you make everything even worse. Discussing design concepts with other developers helps to create understandable, good designed and future-proof code. So, these are just my two cents on this topic.

I learned it by doing more code reviews (and reading code reviews from others). I think there are no strict “guidelines”, every review is different (and maybe needs additional things to look at). The short answer is: “Just check if you are happy with the code/changes, because you are the one that needs to live/work with the code in the future.” :wink: That’s why a assign you for reviews where I think it’s useful when you have a look at them (but you’re welcome to also review other stuff :slight_smile:).

Here are some things I do when reviewing a PR:

First I read and (try to) understand the code. This is easier when the PR is separated in multiple commits with messages which describe what was done and why (or with a PR description containing the same). And this is also easier when refactorings are in their own commit. If I don’t understand something, I ask (I already learned some stuff by reviewing your PRs ;)).

While reading I always check if I would have done something different than the PR author did. This doesn’t mean that my solution would be better, so I need to consider, if the solution from the PR is OK, or if my solution has any advantages. If I think the code can improve with my solution, I ask (maybe the PR-author has done it that way because of a reasen, that I didn’t think about).

Also while reading, I can sometimes already spot things that maybe breaks in an edge case. I keep that in mind so I can test that later when running the code. And the better you know the code the better you know where you have to look for possible pitfalls.

Then I check if there are tests for new code, and if there are tests needed (sometimes things can’t be tested). And I check if the tests test useful things :slight_smile:

For PRs with multiple review rounds I only check the updates between the rounds, but at the end I always do a full review again (maybe there is stuff that is obsolete now, but was missed).

I know that feeling, and reviews are not easy. That’s why it sometimes takes time until you get a review :wink: But in the end, code reviews are no guarantee to find everything. So nobody gets angry if you approved a PR, but later a problem is detected with that code. People make mistakes.

So just continue doing reviews and you will improve (and also your own PRs probably will improve by reviewing others PRs) :slight_smile:

1 Like

Is something like this in the guidelines for contributing? And if not, would it be useful to add something like this to the guidelines? If it helps reviewers to do their job more effectively, it might be good to point this out to potential contributors. Just a thought!

1 Like

I think we don’t have real guidelines, the only thing I know is Getting started with contributing, but I think nobody reads it.

For the diaspora_federation repo I have a CONTRIBUTING.md (I stole that from the api-documentation repo). That is also displayed when someone creates a new PR:

So we should probably add something like this for the main repo too, and it should also include:

  • Write tests
  • Check codestyle with pronto before creating the PR
  • Translations are managed with WebTranslateIt
  • Don’t create PRs from your develop/master branch
  • Use meaningful commit-messages and split refactorings in separate commits
  • (maybe more, but that are the only things that came to my mind which are often done wrong)

So it would be cool if you could write some nice guidelines (If you have the time, but I think you’re better at writing than me :wink: ). I think the CONTRIBUTING.md from the federation gem is already a good start.

Thanks for sharing your thoughts :slight_smile:

I thought of every of this points and it’s nice to see them now as your practices.

I think that for convenience we can make a short checklist:

:heavy_check_mark: Ensure you understand the code
:heavy_check_mark: Check if you haven’t came up with a better solution than the author
:heavy_check_mark: Check the edge cases
:heavy_check_mark: Check that required tests are in place and that tests presented there are really required

2 Likes

I created a PR which adds the CONTRIBUTING.md file: https://github.com/diaspora/diaspora/pull/7477