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.

I am looking for some ideas here.

I read the article How should code reviews be Carried Out and Code Reviews, what are the advantages? which were very informative but I still need more clarity on the question below.

My Question is,

  1. Being the target developer, can you suggest some best practices a developer can incorporate before his code is going get reviewed.

    • Currently I practice the following methods

      • PPT for a logical flow
      • Detailed comments.

Issue: Even though I have implemented the above practices, they do not help on the review. The problem I faced is, when certain logic is referred, I keep searching for the implementation and the flow and too much time is wasted in the process and I get on people’s nerve.

I think a lot of developers would be going through what I am going through as well.

share|improve this question
2  
Only one : don't do stupid things in your code. –  BЈовић Nov 18 '11 at 7:36
1  
KISS: if the code is simple, your brain is able to manage it all. –  mouviciel Nov 18 '11 at 8:21
    
when you do code review in your company, who usually leads the meeting? you or a person who is reviewing your work? I ask because code review meeting in IMO is not the place to spend time searching for bits and pieces of code even if you were really fast at looking things up. –  DXM Nov 18 '11 at 8:24
    
@DXM Thanks for reply. It is my TL would leads the meeting. –  Karthik Sreenivasan Nov 18 '11 at 8:30
    
@Karthik: k, that part is good. So based on your question, you are not asking how to write and produce high quality code that is ready for code review. Instead, your main concern is this: "I keep searching for the implementation and the flow and too much time is wasted". Can you elaborate on that? why are you doing any searching if TL has the code in front of him/her and is leading the meeting? –  DXM Nov 18 '11 at 8:39
show 4 more comments

3 Answers

up vote 8 down vote accepted

So based on details OP provided, it sounds like the question is, "how do I learn my own code so that when asked to find X or explain Y, I'm able to respond quickly."

Few suggestions that I can think of:

  • When coding, you need to take the time to learn and understand your own code. This could be what your TL is trying to get across to you in not so many words. Being a TL on the current project, I've done a lot of code reviews in the last 11 months and I do notice a practice of some developers to search for "example code" either in our own code base, or somewhere else (google, etc...) and copy/paste it in. Personally, I can't stand it because while their code passes the simple unit tests, they do not understand what it is actually doing, so we are never guaranteed that there isn't some boundary case or an expected failure condition that could occur.

  • As a corollary to previous statement, if you have to copy/paste, try to only copy/paste the code YOU have previously written and that you understand. It is certainly ok to "borrow" other people's idea but in that case, rewrite their code line by line because as you are writing it, you will gain better understanding into what it does. If you are using external APIs, even if you have an example that uses that API, take a few minutes anyway to find a reference and learn how that API works. Don't just assume that if it worked before, it will also work in your situation.

  • Read up and learn to love the DRY principle. A lot of times what you are tempted to copy/paste could be placed in a common location (separate function, separate class, separate library...)

  • Read up and learn to love SOLID principles and while you are at it, review KISS which was already mentioned by mouviciel. These principles are all oriented at producing very concise, clean and modular code. If you have large classes and large functions within those, it is clearly going to be much harder to find things and on top of that try to explain what the code does. On the other hand, if you follow (or at least try to follow) SRP and make each class/function responsible for one thing only, your code will be small and very readable.

  • Pick up a copy of Clean Code. Very good book. It talks about writing code that is self explanatory and easy to read, maintain and extend. If you practice writing code that is easy to read, you shouldn't have problems reading your own code in the code reviews. And this is the funny part, I've asked people to read their own code or simply tell me what the variables were representing and they couldn't answer even though they wrote that code (brand new classes, not legacy) only a week ago. Good naming goes a long way.

  • If after all the simplification and refactoring, you still have a function that has to perform some kind of algorithm which is not very apparent, take the time and write a comment block in that function explaining the algorithm. Not only will it be helpful when you have to modify that function 2 months from now, but if you get ambushed in a code review, you'd be able to simply read back what you wrote.

  • If after all the items above, do you still find yourself in trouble? are you new to the team and asked to work with a lot of legacy code? In that case, it could be that your TL is being an A$$ and you could be proactive by asking him before the meeting to go easy and not waste the time of everyone involved. When new people join a team, TL needs to have enough patience because working in a new platform, new product, new people, new environment takes a lot of concentration from a new person, and that person will be missing some details in the beginning. Works as Designed and your TL should just accept that.

  • If after all items above, you still feel that you have horrible code reviews. Talk to your TL. Sometimes people feel bad because of the nature of code review meetings when in fact TL is perfectly happy with you. When I do code reviews, my goal is to highlight what needs to be changed, make sure you understand the changes and move on. A lot of times I don't have time to be polite and some people get defensive and attempt to answer every single one of my comments. In those situations code review meeting grinds to a halt so I tend to interrupt them and move on. Generally, after the meeting I would talk to the new guys to make sure they understand the process and that it is nothing personal. After few code reviews people are generally much more comfortable.

share|improve this answer
    
+1 for "do not copy and paste code you do not understand". That's intolerable! Also +1 for "talk to your TL" –  MarkJ Nov 18 '11 at 9:25
    
@DXM Your ability to understand the finer nuances of the question was very professional not to mention your answer is very informative and descriptive. Mind = Blown! –  Karthik Sreenivasan Nov 18 '11 at 10:13
    
@DXM From your reference "On the other hand, if you follow (or at least try to follow) SRP and make each class/function responsible for one thing only, your code will be small and very readable." Can you let me know what does *SRP mean ?* I saw another interesting post on code clarity here. –  Karthik Sreenivasan Nov 18 '11 at 12:31
1  
@KarthikSreenivasan - In the context used its a pratice where a method or class is responsible for one thing. For instance a method that adds numbers together should not also return the average. Simple search found this: en.wikipedia.org/wiki/Single_responsibility_principle –  Ramhound Nov 18 '11 at 13:37
    
@Ramhound +1 for the link. Thanks! –  Karthik Sreenivasan Nov 18 '11 at 13:41
add comment

Practices vary, but in my experience:

  • Don't do anything special to the code. It's natural to spiff up your code a little more when you learn that it's going to be reviewed, and there's no harm in fixing obvious things like spelling mistakes and such. But don't go in and add a lot of detailed comments or otherwise change the code just because it's scheduled for review.

  • Code is prepared and distributed to reviewers well in advance of the review. This is usually done by a neutral third party, probably the code review facilitator. If printed out, the code should be small enough that lines aren't wrapped too often, but large enough that everyone can read it easily. Print it in landscape format if that's what it takes.

  • Code should be printed or displayed with line numbers. Preferably, the number should continue from one file to the next. It's so much easier to refer to "line 3502" than "line 238 of foo.c", and having the numbers lets everyone talk about specific lines without wasting time finding those lines.

  • There should definitely be a facilitator, btw. His or her job is to keep the review from getting bogged down in minutia, prevent it from getting personal or heated, and strictly limit the length of the review.

  • As the author, you should review the code yourself before the review meeting. Write down the changes you'd suggest if this were someone else's code. This jogs your memory of code that you might not have looked at in a few days, and it also helps you practice looking at your own code with a critical eye. After you've been through a few reviews, both as reviewer and as author, you'll find that your own notes will more closely match those of the rest of the group.

  • Be prepared to take notes during the review. This shouldn't be your main concern -- someone else should be recording the action items that the group agrees on so that you can focus on explaining the code and listening to the feedback. But there will be times when you get some valuable feedback that isn't an action item, and you should right such things down as they occur.

  • Remember that it's not personal. It's hard to avoid feeling (and acting) defensive during a review. It's fine to explain your code if you think it was misunderstood, but more than anything else try to just listen.

share|improve this answer
    
I would add one thing : "line 3502" would be a big red mark. Having very long files is definitely a bad thing. –  BЈовић Nov 18 '11 at 8:16
2  
@VJo: Caleb suggested to have the line numbers continue across files, so line 3502 is actually line 238 of foo.c. –  Heinzi Nov 18 '11 at 8:40
    
I disagree with the line number continuning across files. To me, that's just confusing and awkward. If there are any issues found, they need to be tracked by module (class, file, maybe even method) anyway. In addition, during a code review, you shouldn't be reviewing an entire system, but rather a subsystem or even a couple of classes or files, so it shouldn't be too hard to track where the changes are. –  Thomas Owens Nov 18 '11 at 11:50
1  
@ThomasOwens The line numbers are solely for the purpose of easily describing a location in the reviewed code during the review. It's faster and less error-prone than using "file foo.c, line 123," and the OP specifically asks about spending less time finding code. Agree that issues should be tracked by file. IME, reviews tend to cover a group of classes, maybe two large ones or a dozen small ones. 3500+ lines is too many to review at once -- was only trying to make the point that the numbers continue from one file to the next. –  Caleb Nov 18 '11 at 13:29
    
If you're organized, it shouldn't matter. To me, I feel that it would slow me down. I've been involved with code reviews and I always print out the files, staple them by class/file, and then read and annotate them. If someone wants to tell me where to look, I want a file name/line number pair - it would make it much easier for me, especially since my IDE prints out the file name in the header/footer on each page and I print the line numbers on a per-file basis. –  Thomas Owens Nov 18 '11 at 13:42
show 1 more comment

One more thing to add to the other answers: to make formal code reviewers easier, conduct LOTS of informal code reviews! For instance:

"Hey Bob, can I show you how I implemented the foo() function?" "Hey Steve, can you take a look at this class diagram and let me know what you think?" "Hey Karen, can you help me think through this problem? I think I've got a good solution, but I could use your help..."

Make this a regular habit. When you involve your coworkers early in the design process, you:

  • Build relationships
  • Gain new insights into the problem
  • Improve your ability to explain the problem / solution at hand
  • Save time later in formal code reviews
share|improve this answer
    
+1 for team building and Improve your ability to explain the problem. That is indeed a great idea! –  Karthik Sreenivasan Nov 19 '11 at 6:18
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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