8

I recently joined a team who are using github Pull Request feature as the single code review tool.

In previous teams, I was used to reviews done on a fully checked-out code base, done using IDE tools, code coverage and static code analysis tools, etc.

Compared to these reviews what I get today through github is abysmal. You see basically only the changes and the context is lost. With some efforts you can see a bit more of the file, up to the full file, but I see these code reviews as both very time consuming and of very few benefit.

What is the experience of others using only online reviews ? Did you get any benefit from it ? Was it a frustrating experience like mine ? Did your team eventually evolved to some other type of review ?

I understand I can configure my IDE to do github review from it. But I can't force the rest of the team to do it, merely propose it.

I'm not asking anything subjective. Either a tool has some value in some use cases or it doesn't. Improving the code base or not improving it, catching bugs or not catching them, help sharing knowledge through tools or not help sharing it is factual.

3
  • 1
    The answer will depend on a lot of things, most importantly the macroscopic workflow of the team; the nature of the software project and environment (e.g. dependencies); the modularity of the software architecture; the rate of code change (or, the speed of development); level of communications and planning by team before a code change is commenced; etc. – rwong Dec 11 '20 at 20:41
  • 5
    I think you are putting up a false dichotomy here. Many IDEs allow reviewing GitHub Pull Requests in the IDE, so it's in no way an either/or situation. – Jörg W Mittag Dec 11 '20 at 20:48
  • You mean in some context people can actually find value to look at small changes picked out of context ? (And I'm not speaking of the handfull of line before and after). The only such use case where I can see value is mere local changes to one file: ie: small local contributions. And changes limited to small local contribution looks like constant impediment to refactoring. – kriss Dec 11 '20 at 22:03
12

I'm curious how you know that all developers only use pull requests as the only code review tool. On previous teams, the organization's provided a diff-based review tool (like SmartBear's Collaborator and pull requests in Bitbucket), but developers were free to conduct the tool using other methods. In some cases, especially when the reviewer may not have been the most familiar with the code, developers did open their editors and check out different copies of the code, sometimes even running it. Just because it's not spelled out doesn't mean that people don't do it.

The advantage of the diff-based view is that it highlights exactly what the proposed changes are. Tools like GitHub and Bitbucket also allow the pull request author to link the pull request and/or commits to issues in the issue tracker, making them readily available. This gives the reviewer access to a description of the defect or proposed change and any relevant information about it in addition to the change itself.

In my experience, code coverage and static analysis don't need to be done by individual reviewers. CI tools should be executing automated tests, performing linting, and reporting on the health of the codebase. Again, tools like GitHub and Bitbucket can expose links to CI builds and related artifacts in the user interface. If someone has a particular question about code coverage, that information should be readily available.

Generally speaking, though, the diff-based reviews are often sufficient. However, there are different reasons to carry out a code review.

The bulk of my time carrying out code reviews is spent as a knowledgeable person looking for defects or pointing out opportunities to improve the code. If the codebase is well maintained and you have sufficient knowledge, the diff may often be enough to do this. However, when I'm less familiar with the codebase, I'd be far more likely to open the code in an editor and explore, perhaps even running code locally and debugging to observe the changes.

Another factor is the experience of the author. If the author is highly knowledgeable about the work they are doing and comfortable with the language and the codebase, I may trust them more to cover edge cases and not need to do as deep of a dive on their changes as I would be for someone working in an unfamiliar part of the code.

As a team member reviewing code, I'd prefer to not be told how to carry out my review. As a process improvement specialist, the advice that I give is to not be too specific in the details of how individual engineers carry out the reviews. I prefer to establish objectives for the review and make sure that those objectives are met. The objectives would be based on the level of risk of the change, the author of the changes, the reviewers, and the overall intent of the code review process.

So, I'd ask you a question: Are there problems in your code review process? Are there problems in other aspects of the development process that could be solved by adjusting your code review process?

It may be an interesting exercise to work with your teammates to share tips and techniques for good code review practices. However, I'd suggest starting with normalizing on outcomes and expectations for code reviews and how the reviews fit into the overall process so the techniques support the need. Then, each individual should go about conducting reviews in the way that they need to in order to support the goals of the team and organization.

4
  • The teams is rather small. Hence it's easy enough to know all developpers are only using online tools. We had some discussions and some of them are actually ready replace their current practice by IDE based review. But others are not (argument is online review is "enough"). – kriss Dec 12 '20 at 9:13
  • But indeed working on expected outcomes of reviews could be the right move. Specific troubles are that team alignment is bad on review outcomes. Some are expecting an in depth review (like me), other are merely expecting a quick glance. – kriss Dec 12 '20 at 9:16
  • 3
    @kriss "In-depth review" and "quick glance" aren't outcomes. Outcomes would be "understand the change", "ensure test coverage", "check understandability", or "find defects". The depth of the review depends on the outcome and the reviewer. I can understand the change and check for readability using an online diff. I can ensure test coverage with a CI report and the diff if test updates are part of the diff. I may be able to understand the change using a diff if I have existing knowledge, but may otherwise need a more in-depth review in my IDE. I'd probably need to use an IDE to find defects. – Thomas Owens Dec 12 '20 at 11:26
  • 1
    I was probably unclear in my comment, but I understand. "In-depth review" and "quick glance" are not outcomes, but about how people in my team are doing reviews (and how much time they are ready to spend). I believe we are not focusing enough today on the purpose of the review and what we get of it, but merely on the act of reviewing. In other words we are focusing on review cost and not actually caring of benefits and if we are actually getting them or not. Working on expected benefit could be a way to change review process and eventually tools. – kriss Dec 12 '20 at 15:22
7

The value of any code review is not nearly as dependent on the tool or the style as on your team.

Different teams have different values and will emphasize different things. So naturally the experience will be different.

What concerns me most is you don’t seem to be getting much out of the experience. That may be because you're getting used to a different style but it’s still something worth talking to the team about. Learn what’s important to them and let them know what you’re not getting.

The code review is where the shop's culture is communicated. Be sure to listen and to add to it. It may be that you have some good ideas that they will want to try.

As for context, once you’re used to the tools you should be able to build that yourself, regardless of style.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.