Should I fix the code myself?
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.
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:
- I need help so they do not go into a defensive mode.
- 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:
- The author will not learn from that.
- It is like me saying: "Move aside, let me show you how it is done. My code is better than yours."
- 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.
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.