Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Please share your views on the code review process. If someone gave a code snippet and asked you to review that code, then what are the major things you will focus on that code Review process?

For instance, I will check if any dead code is available in that code, other than checking dead code, what are the key elements to be focused on in the code review process?

Update: Please share any free code review e-book which helps me to understand more and more.

share|improve this question
    
- Can you understand your peer's code without any help? - Is the code doing what it supposed to do? - Is the code is beautiful ( no deadcode, no duplicates, naming is correct etc)? –  java_mouse Apr 20 '12 at 16:24
add comment

closed as not constructive by ChrisF Apr 20 '12 at 12:09

As it currently stands, this question is not a good fit for our Q&A format. We expect answers to be supported by facts, references, or expertise, but this question will likely solicit debate, arguments, polling, or extended discussion. If you feel that this question can be improved and possibly reopened, visit the help center for guidance.If this question can be reworded to fit the rules in the help center, please edit the question.

6 Answers

up vote 13 down vote accepted

I use the following check list. Many of these are from "Clean Code" book and SOLID principles.

Logic

  • Code duplication
  • Code is in the wrong place. For ex. presentation logic is in business logic layer and vice versa
  • Class has too many responsibilities. For ex Order - that handles payment, shipping, etc... Instead those responsibilities should be moved to other classes.
  • There are appropriate unit tests that provide good code coverage

Readability

  • Function is too long, tries to do too many things
  • Function/class names are not clear
  • Follows project's standards and naming conventions

Speed

  • Is the code sufficiently optimized
  • Can it affect adversely other parts of the system
  • Would there be issues under load
share|improve this answer
    
Yeah, this breaks it down quite nicely. –  Desolate Planet Feb 25 '11 at 12:15
    
code duplication comes in Checking of Dead Code Process, am i right?? –  Sankar Ganesh Feb 25 '11 at 13:08
1  
@Sankar, code can be duplicated (or a near-dupe) and still be in use –  Marcie Feb 25 '11 at 16:44
    
@Marcie: Thank you very Much, for your kind explanation –  Sankar Ganesh Feb 25 '11 at 16:47
    
While this may be Java/Eclipse specific, you can customize PMD, CheckStyle and FindBugs to spot most of these. –  Joset Mar 9 '11 at 2:37
add comment

During code review, with our projects, I've always started out by noting entry points and from there following the data pipeline(s).

Find each stop and each flow change or data alteration along the way until you have a grasp of all the course(s) data will take from start to finish. Compare the code to the function the code is actually performing.

  • Could the pipeline be simplified or does it need restructuring?
  • At each stage is data coming, going and operating as expected and intelligently?
  • Check the loops, check them again and again. Consider them often.

After becoming satisfied with the above, focus usually will shift to other factors.

  • Can the code be simplified by abstraction/unabstraction/better syntax or naming?
  • Review serialized data structures like arrays and vectors, sometimes variable should be promoted from these to their own identity and other times variables should be collected into them.
  • Can the code be simplified? (Just look harder.)
  • Second restructuring and restarting from the pipeline if changes are significant.
share|improve this answer
add comment

Since you specifically ask for e-books in your update, I'll point out that my company, SmartBear, gives away a book on code review. Unfortunately, it looks like you're in India which is not one of the countries we currently ship to. However, you'll be happy to note that the signup page linked above, includes links to several sample chapters from the book.

Our general recommendation on building code review checklists, is to spend time analyzing the bugs that you write. You need to go deeper than the individual cases and think about the underlying causes or classes of bugs.

Beyond the empirical data, there are some things that are necessarily bugs, but should be verified in code review to ensure maintainability of the software:

  • Are there sufficient unit tests?
  • Are classes and methods sufficiently and clearly documented?
  • Are classes and method appropriately scoped?

Finally, if your organization is just adopting code reviews, start slow and be patient.

share|improve this answer
1  
+1 for making the checklist relevant to your team. I found that getting a team together so that they agreed what a peer review checklist would be a really useful exercise. That way the team own the checklist so it is a tool to improve quality not a stick to beat people with. –  AlexC Mar 3 '11 at 10:12
    
Thank you for your book, it is a great free resource! My point of view is the same when it comes to checklists, you need to crate a specific one depending on language, technology, project, team and goal of the review. –  Gabriel Ščerbák Mar 4 '11 at 0:50
add comment

Did you try looking at Fagan's inspections? You need not use the full blown inspection methodology/checklist but you'll surely benefit from the insights.

share|improve this answer
add comment

Sounds like your organisation could benefit from keeping a code review checklist. Write one! They make reviews more effective.

  • Start with suggestions from the other answers here, or from books like Code Complete.
  • Add more items by looking through your bug database and finding any common coding mistakes that are responsible for many bugs.
  • Encourage everyone to use the checklist in reviews.
  • Keep the checklist up-to-date. From time to time, you will want to add new items and remove old ones.
share|improve this answer
add comment

(1) Does the code solve the problem you are trying to address? (2) Does the code clearly communicate the solution? (3) Can the code be optimised to the degree of (1) and (2)?

Obviously, your foucs may change if you are mentoring a junior/graduate developer. You may find he/she has bad programming habit, such as using magic numbers, duplicate code, excessive variables (the list goes on..) and you want to provide some feedback to help improve the developers coding skills, but I've have the above theree questions in mind and try to steer people towards that in my own reviews. You obviously want the feedback of your code review to be valuable and carry across to other developers.

share|improve this answer
add comment

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