14

I am part of a team of seven developers in a small software company and I am trying to introduce regular group code and design reviews. We have carried out some reviews in the past, but it has been sporadic. I would like to make it a more regular thing.

I have read Code Complete and other similar resources and they talk about the mechanics of how to carry out code reviews but I have been unable to find any best practices on how to choose what to review. We have a code base that is more than eight years old and covering a variety of languages, so there is plenty that could be looked at.

Here are some of the factors that I can think of that might affect the choice:

  • Language: C, Java, SQL, PL/SQL
  • Code age: New code vs old code
  • Code usage: Frequently used code vs (effectively) dead/little used code
  • Code importance: Critical code vs non-critical code
  • Developer: Junior developer code vs Senior developer code

I understand that this isn't a question with an absolute definitive answer, but any guidance would be useful.

Some peripherally related questions:

Improve this question

5 Answers 5

Reset to default
12

In general, you must review everything. If a fresh application has 2 000 LOC, all 2 000 LOC must be reviewed.

That's why there is no best practice on how to choose what to review.

If you approach an existent large codebase, never reviewed before, then it's the same thing when you must rewrite an existent large codebase, and to choose where to start. It strongly depends:

  • on the codebase (a single monolithic code would be more difficult to rewrite/review than a set of separate components, etc.),

  • your context (can you stop everything you work on and spend three months (three years?) working only on rewrite/review, or you must do it by small lapses, only when you have free time)?

  • the type of review you do (do you have a checklist of things to review? Depending on the items of the checklist, you may want to review some parts first).

If I were you, I would:

  • follow the 80%-20% principle, mentioned in the first comment of the second question you linked to.

  • take in account that 100%, being an ideal, isn't maybe worth it. It's like 100% code coverage for unit tests, except that such code coverage is mostly impossible or extremely expensive.

  • start with the parts of the code you use the most and which are the most important. If the codebase has a library which authenticates and registers new users on your corporate website, review it first, because you want certainly find security holes before hackers do.

  • use existent metrics to determine what is more important to review. If a part of the codebase has no unit tests at all, while another, equally important part, has 85% code coverage, start by reviewing the first part. If a part of the codebase was written by a developer who was known to be inexperienced and to introduce more bugs than any of his colleagues, start by reviewing his code first.

Improve this answer
1
  • If you do TDD properly, then 100% coverage is not only easy, it's also inevitable, and actually turns out to be a very low bar indeed.
    – Jonathan Hartley
    Commented Jul 1, 2016 at 16:46
5

Start by reviewing all changes you make to the code; that will stop the problem getting worse. Then start reviewing code based on frequency of changes; these will be the 'problem' areas.

You'll want to figure out some way to track that you have reviewed a section of code so you can analyze the review coverage of your code relative to other concerns.

If you can determine what code is not covered by your tests, that becomes higher priority for review.

Improve this answer
3

Review all new changes that have been made before they make it to production. installation scripts, source code, database changes, everything! The whole point of code review is to stop bad code from making it into production. Be it a poor organizational scheme or simply an introduced bug because something was missed.

Refactoring the current code you are working on goes hand in hand with code review. For example, when I'm reviewing code, if there was duplicate code in a class that contained a bug fix, even if the developer didn't change that code in their fix, I would not pass it. I would have them go back and remove the duplicate code.

If you refactor relentlessly then code review becomes useful. Otherwise it is a waste of time.

If you incorporate the code review process as step in your development process the code base will get better over time. Better yet, you should not allow your developers to take on new feature work or bug fixes until the code review backlog is empty. This ensures that code review gets done.

If there are known areas that need to be refactored, but it will take a long time to do them (i.e. 1 week or more). Then create a work item for that refactoring by itself and add it as an item to be worked on.

Improve this answer
6
  • 1
    I disagree - let the code go to production, and review it when you can. The trick is to trust your developers and assume they do not make huge mistakes. If they do (we all do) then you can fix and refactor the problems after checkin. Trying to stop all code from reaching production before review usually means no code goes to production (in my experience). Of course, if you don't trust your devs, then feel free to lock them down along with the sharp scissors in the stationary cupboard.
    – gbjbaanb
    Commented Jan 18, 2012 at 21:25
  • "Review all new changes that have been made before they make it to production" I mostly agree with this. "Better yet, you should not allow your developers to take on new feature work or bug fixes until the code review backlog is empty." I'd love to do this, but given the realities of commercial pressures it's sadly not realistic.
    – Burhan Ali
    Commented Jan 18, 2012 at 21:28
  • I always trust my devs. The devs are the ones doing the code review. Also, putting a process in place to ensure that code review gets done in no way reflects a lack of confidence in the ability of the developers. The developers, project managers and business owners should all be more relieved that there is one less thing to have to worry about, namely: remembering to do code review.
    – Charles Lambert
    Commented Jan 18, 2012 at 21:32
  • @BurhanAli - It is very realistic. Our customers have been high profile customers (think Microsoft) and our release cycles are very fast. It should take all of about 30 minutes, maybe an hour on occasion, for a developer to review a change. If it takes longer than that, then you most likely are not dividing your work into small enough pieces, which is a different problem altogether.
    – Charles Lambert
    Commented Jan 18, 2012 at 21:38
  • 2
    @gbjbaanb You let unreviewed code go into production? Wow. It's not about not trusting your developers (you can get one of your developers to review someone elses code), it's about finding glaringly obvious mistakes
    – Rob
    Commented Jan 18, 2012 at 22:30
2

Start by reviewing all new code, and modifications to existing code.

When reviewing modifications to existing code, the developer should follow the boyscout rule. Leave the code better than he found it.

That doesn't mean that you have to fix the entire file to be perfect. But it shouldn't add to the mess, it should make it a little better. Perhaps by moving the modifications into new classes that are properly structured, and leaving the rest of the original code file as is (after all, its' working).

Once you start improving the code by reviewing all new code and modifications, as developers, you should know what areas of the application require the most change. Then, review those, discuss how they can be improved, little by little.

Reviewing code written 10 years ago, for the sake of reviewing it, is pointless, the developer should have improved over those 10 years. So, there's no point reviewing it just to learn what you all know.

The purpose of code reviews is to improve and correct mistakes you are currently making and to share that knowledge amongst the team.

Improve this answer
1
  • +1 for "Leave the code better than he found it." I always try to encourage that. As for reviewing 10 year old code just for the sake of it, I agree with what you say. But is there perhaps some benefit in doing it though to make the team more comfortable with the idea of reviewing? There's not much danger of people getting defensive over code they didn't write (or is so old they have forgotten they wrote it.)
    – Burhan Ali
    Commented Jan 22, 2012 at 18:42
1

In my project we do include code review as a must-have in most cases for any assignment/user story/bug being developed. We are using scrum/agile processes and ticket/story is not moved to built (that is a backlog for QA) until unit tests are written and code review is completed.

We do use Atlassian FishEye analysis with Crucible code review integrated with JIRA+SVN for this purpose.

When developer checks the code in for a specific story he creates a new code review in FishEye, where he selects the other members of the team to do the review.

Once code review is completed, (tool highlights the changes submitted and allows to leave the comments for the specific line of code) the developer corrects the mentioned issues/implements the suggested improvements and moves the ticket into Built column in JIRA - that means story is ready to be tested and that there are no more code changes expected for this specific work item.

This also ensures QA doesn't test anything that might be changed and potentially be broken while refactoring the code after code review.

To sum up, all code must be reviewed - this supports the high quality of code, which usually results in a better design, readability, maintainability and testability of the code and improves development performance in a long run.

Improve this answer

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