Unit Testing – Advancing Code Review and Unit Testing Practices

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.

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.