7

We were using git gerrit for manual code review . but recently we are planning to integrate sonarqube in our Jenkins integration server. Do we still need manual code review? Or we can stop manual code review and sonarqube is enough. I would appreciate if you share your experience with sonarqube.

  • 4
    What are the features of SonarQube regarding semantics of names? – mouviciel Nov 28 '14 at 9:41
  • It covers 7 axes of code quality 1.Architecture & Design 2. Comments 3. Coding rules 4.Duplication 5.Unit Testing 6. Potential bugs 7.complexity. www.sonarqube.com – Sharif Nov 28 '14 at 16:18
20

Yes, you should keep code reviews.

SonarQube, like any other automated tool, is great as a tool which helps pointing out some of the possible problems, including problems which are not easily seen by humans.

Duplicate code, for example, can be checked automatically, as shown by Visual Studio or SonarQube, while a reviewer may not easily see that the same piece of code was copy-pasted and slightly modified afterwards.

In the same way, style checking can and should be automated, because an automated checker will perform better than an human. Static analysis is another great example.

But then, there are issues which can hardly be automated. For example, would an automated tool find something wrong in this JavaScript code?

// The price should be incremented when a rebate is available.
if (this.rebates.findFor(this.customer, this.product)) {
    this.customPrice -= this.computeRebate(this.customer);
}

On the other hand, any reviewer will immediately find two issues:

  • The comment is plainly wrong and misleading. We are not incrementing the price.
  • Even if it were correct, the comment remains useless. If the two lines of code are not clear in the context, a minor refactoring can make it explicit enough to be able to get rid of the comment.

Another benefit of code reviews is the ability to teach junior programmers. For example, an automated checker will find this piece of C# code written by a beginner programmer correct:

var result = new List<Day>();
foreach (var day in days)
{
    if (day.HasAppointment)
    {
        result.Add(day);
    }
}

return result;

while any more experienced developer will suggest rewriting it like this:

return days.Where(day => day.HasAppointment);

Even if an automated tool will do such suggestions (after all, it is possible to programmatically detect procedural code which can be rewritten using functional style), would this tool be able to explain to the beginner programmer the caveats of such rewrite (lazy evaluation)?

  • 3
    I wish I knew more beginner programmers who understand the imperative logic (like in your "junior code" example) behind lambda queries… – Pierre Arlaud Nov 28 '14 at 9:47
  • 4
    Another benefit of code reviews is the ability to teach junior programmers. because seniors surely can not learn from other seniors or even juniors... – Maxime Morin Nov 28 '14 at 10:23
  • Resharper detects code that can be rewritten as LINQ quite often. – CodesInChaos Nov 28 '14 at 11:29
6

IMO, Manual code reviews helps in forming a shared understanding of the codebase, reducing dependency on a single or a small group of individuals. It may also teach a thing or two in programming style/language features for the reviewer reviewing the code. It works great if everyone reviews every other person's code or pair with someone senior who is good at reviewing.

So unless it is a single person reviewing the code of 10+ individuals and you introduce sonarqube to automate some of that process, I would rather keep manual code reviews for the sake of the points stated above.

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.