How to give feedback after code review process

Question

I am currently reviewing some of the code of junior developers who just joined my team, I am wondering how should I deliver the output of this review:

1. Should I fix the code myself?

2. Should I give them feedback on the review process and let them do the fixes according to my instructions? And if so, how do I give this feedback, do I fill certain template document and send it to them, or there is some software that will help me mark things with problems inside the code files where they can later check for it? (I am using Visual Studio).

After I have done reviewing the code and fixes are done then some time has passed and some parts of the code I have reviewed in the past has changed, how do I do the re-review process? Should I recheck all the code all over again? Or do I just check the parts that have changed? And if so how do I keep track of the parts that have changed so that I avoid double reviewing code?

Answer 1

Short Answer

Should I fix the code myself?

No.

Should I give them feedback on the review process and let them do the fixes according to my instructions?

Yes. According to your suggestions, not instructions. Instructions sound too authoritative.

And if so, how do I give this feedback, do I fill certain template document and send it to them, or there is some software that will help me mark things with problems inside the code files where they can later check for it? (I am using Visual Studio).

Use a tool to give the feedback. You may use Visual Studio.

Long Answer

I used to use Visual Studio but I constantly had to ask other developers, "Hey can you send me your work so I can review it?" I did not like that and it never worked out really well. Now, I use Review Assistant because I can start a review by looking at checkins. I do not need to rely on another developer sending me work to review. It also helps me mark items as defects, or simply mark items to be looked at by the author. This works for our team because once we start a review, it stays right there on the review board and does not get lost in translation. This is integrated with Visual Studio. As I mentioned, Visual Studio also has its native review process but I find it has limitations and the process is not natural; therefore, I use Review Assistant.

This tool also helps with the back and forth process, discussions etc.

The process, more or less, is as following:

I review something, then I send it to the author (in your case junior dev). They make changes and then they send it back. I look at the changes and provide feedback. If I am good with the changes, I close the review. Otherwise it goes back and forth. Sometimes if there is too much back and forth, I will just walk over to their desk and use a whiteboard--it really expedites the process.

Code reviews are a sensitive area so be very careful with the wording choice. I never tell anyone

Poor choice of wording

I reviewed your code and there are some items you need to change.

I instead say this:

Better choice of wording

I looked at your code and I need some help. Can you please review the items I sent you and see if you can clarify some of my questions?

This makes the author think:

1. I need help so they do not go into a defensive mode.
2. It sounds like they are the reviewer, not me. Technically speaking, since I am asking them to have another look and change some things, they sort of are like the reviewer.

These simple word choices have helped me enormously.

I never underestimate junior devs. I have worked with some senior developers (over 10 years experience) and there code was worse than a junior co-op student. So just because they are senior or junior is not all that important; their work is really what speaks louder than years of experience.

Oftentimes, to encourage junior devs and get them participating in reviews, I will send them something to review for me: Something they can figure out, something they will find challenging but not totally beyond them. I may word it like below:

Can you please review some code for me because I cannot figure it out.

This helps me greatly again. This helps because it clearly shows that I am not the only one who reviews, but they also do reviews and they are also part of the process. It shows that the whole idea is to produce good, clean code and ask for help if needed. The review process is a culture so we really need to work at it.

Now some people may worry that if they do the above, the junior devs will lose respect because they just did something you could not do. But that is far from the truth: asking for help shows humility. Plus there are plenty of situations wherein you can shine. Finally if that is your fear, you have self-esteem issues. Lastly, maybe I really do not know it: I mean some of these devs have algorithms fresh in their head because they just studied them a month ago.

Anyways, back to juniors and reviews. You should see the look on their faces when they figure it out and send me a reply. I then may tell them, "OK, let me make the changes and if you are happy with it, please close the issue."

They feel awesome having the power to look at my work and say: "Yes, your changes are good. I closed the issue."

I never fix the code myself though because:

1. The author will not learn from that.
2. It is like me saying: "Move aside, let me show you how it is done. My code is better than yours."
3. Why would I? This is more work for me.

But I will suggest ideas and code snippets in my comments to help out the author. Please note that sometimes my reviews are simply asking the author that I do not understand their code; there may be nothing wrong with their code. They may need to change variable names, add comments etc. Thus, I will not even know what to change in that case; only they will.

If you keep doing reviews, you will, sooner or later, have a really good idea of the knowledge level of each developer on the team. Knowing this is really useful and you need to take advantage of it and unleash it. Here is how: If I review some code and see obvious areas for improvement and I know another developer may catch them too, I will get them to review it instead. Something like "Hey, I see a few areas that can be improved. Can you please review it in more detail and send your comments to the author?" This works great too because now I have 2 other devs working with each other.

If I am reviewing some work and I notice something that the whole team can benefit from, then I will create a hypothetical scenario and explain the issue in a meeting. I will start by explaining the scenario and ask everyone if they can find the issue or see an issue, get them involved. Get everyone to ask questions. Then finally present a better approach. If someone else has a better approach, I thank them and acknowledge in front of the team their approach is better. This shows I am not "my way or the highway" type of personality. Furthermore, I NEVER open someone's work and start pointing out the issues in a meeting, the author will not appreciate it--regardless of how nice and harmless I think I am being.

When I do reviews, I do not just look for good clean code but also what the code is doing. If the business requirement is: If an employee has been with the company for longer than 10 years, give them 5% increase. Otherwise, 2.5%. The first thing I check is if it is actually doing that. Then I check if it is doing it it in a clean, consistent and performant way.

If I do a review, I make sure to follow up or no-one will take the reviews seriously.

I used to work with someone years ago who would do a review and cc the dev manager, and the QA manager but both managers came from a business background or had little development experience. He only did this to impress them. No one liked it and that is when I told myself I would never make that mistake.

The other thing he used to do is pick on programming style and was convinced his style is the best ("My kungfu is way better than your monkey style..."). That was another lesson for me: there is always more than 1 way of solving a problem.

Answer to some of your numbered questions

1- should I fix the code myself?

No, please see reasons I stated above.

2- should I give them feedback on the review process and let them do the fixes according to my instructions?

Yes, try to use sentences and a tone that is friendly but yet requires attention. Be as clear as you can. State what the issue with the code is, how to improve it. DO not simply ask to change it. But provide reasons.

how do I give this feedback, do I fill certain template document and send it to them, or there is some software that will help me mark things with problems inside the code files where they can later check for it? (I am using Visual Studio).

Like I said, you can use the tool I use or another tool. Do not use email or word documents because they get lost and it is hard to keep track of it.

After I have done reviewing the code and fixes are done then sometime has passed and some parts of the code I have reviewed in the past has changed, how do I do the re-review process? should I recheck all the code all over again?

Mostly what I do is to check the delta (changes only). But you need to have the overall picture in mind to ensure nothing is broken and it follows the architecture.

Final Thoughts

I personally think the word "code review" is a poor choice and do not know how it got started. They could have chosen a much better and less authoritative word.

Answer 2

A lot depends on how you understand code reviews at your company. There are companies where a code review is a highly formalized process which takes place rarely but is a big deal. At others, code review is a mandatory part of each task which gets implemented, and is a very mundane and quick thing with little formalism. Personally, I opt for the latter approach, but it may or may not be your decision if you can use it or not.

I wrote a blog post titled Are code reviews worth your time? to summarize the process used by my team. The takeaways, as they relate to your question would be:

1. Let the developers fix the code. This will allow them to better understand your comments (or realize they don't fully understand them and ask), and performing a task is a better way of learning than only reading about it.
2. Software for code reviews is the way to go. There are many options available, both open-source and proprietary. Most of it works with git. My team uses BitBucket (formerly known as Stash), there's also GitLab and GitHub, Gerrit (which I personally am not a fan of), and a bunch of others. Most of these apps are web-based so it doesn't matter what IDE you use, but there are also plugins for many IDEs, so I'm sure there are some for Visual Studio as well. Software like this allows you to review code before it is merged into the main branch (usually via Pull Requests) and it shows the modified parts and some context lines around each change. This makes the code review fluent and hassle-free.

As for the review-fix-check-the-fix cycle, what you choose should depend on the developers' maturity and the gravity of the issues you spotted. Once teams make code reviews a daily process, you can expect trivial changes such as renaming a class, to simply be applied and there is probably no need to re-check them. If the team is not yet sold on code reviews or the people are really inexperienced, you might want to check such things regardless. On the other hand if your review spotted a complex concurrency issue which junior devs may not fully understand even after you point it out to them, you better check the fix and not give the change your approval before you are sure it was really corrected.

Tools can help you with this since if you work with pull requests, you can easily set up the software to not allow merging a change until it has the approval of a predetermined number of developers. Usually you can view the changes in individual commits of a change which allows you to easily look at only the changes added since your last round of comments.

Answer 3

I vote for the second option. Juniors may may have a better "lerning curve" when doing the changes themselves.

Also: don't be the only one reviewing the code.
Let some of the experienced members of your team also look at the code and schedule a regular review meeting where the reviewers present their findings (they made before the meeting!) to the author. This will raise the motivation of both, the juniors and the experiences team members.