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,
-
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.
Best Answer
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.