13

I'm currently working as a senior dev with 3 juniors below me and have introduced a code review process to help managed to quality of code going into production.

I feel its very beneficial for all of us to review each others work, however after about 5 weeks of the process I am the only one to make any comments in the tool (BitBucket).

I think there are part cultural issues at work, and perhaps a natural reluctance in case their comments are wrong, but is there any way and I can help the juniors feel more comfortable critiquing mine and each others work?

  • 2
    I wonder if this might not be a better question for Workplace.SE, but my own 2 cents as a junior developer. While I was an intern, I was very nervous about participating in code reviews for a few reasons: lack of skills, lack of familiarity with the code base, etc. I soon discovered that participating in the code review helped with both of those aspects (especially familiarity), and was also really helpful by making me feel like our codebase was something I had a vested interest in, so I wanted to contribute. I definitely didn't always leave great comments, but I didn't know until I (cont) – Dannnno Oct 20 '16 at 4:07
  • 2
    (cont) tried, and it helped me work better with the team as a whole. I think if you try to explain why it is helpful for you, the team, and the codebase for them to participate, even if their comments are wrong; if their comments are wrong, that's almost better because then they can learn from it. – Dannnno Oct 20 '16 at 4:08
15

To me, the question here is "what are you looking for your junior developers to get out of the code reviews?". To me, potentially the most important thing is for the junior developers to learn by looking at what is hopefully good code; if they happen to find issues in your code as well, that's a bonus.

If you are looking for your junior staff to learn from the code reviews, the most important thing you have to do is to create an environment where learning is valued, and not viewed as a waste of time. This means a number of things:

  • There is no such thing as a stupid question. If someone doesn't understand a bit of code, why you've used a certain pattern or whatever, they need to feel free to ask, without ever being made to feel that they're wasting either your time or that of the other developers.
  • Time spent learning is time well spent. You want your junior developers to spend time looking at code and learning from it, because it's going to make them better, more productive staff in the future. Unless it's a critical fix which needs to be reviewed now, encourage them to spend more, rather than less, time on the code reviews.
  • Senior staff are not always right. Just because you've been doing this for longer doesn't mean you're right. If they think they've found a bug, they're probably right. If they think another design pattern would be appropriate for this bit of code, then they need to feel free to say so without getting negative feedback.
  • Thanks a lot for the input, I'll will have a think about this and discuss it in our stand up tomorrow – Graham S Oct 20 '16 at 9:14
  • 1
    I would add: the way you get to be an expert is by making mistakes and learning from them. as a practical matter, it might be helpful for the sr. devs to tell some war stories about their past screw-ups. – user223083 Oct 20 '16 at 18:29
5

Have code review meetings in person at set time every week. I sold this to my teammate like this (we're actually both senior devs, but whatever):

"The code review is partially there for me to get to know your code a bit better and know what's going on in your side of things in case you get hit by a truck someday and I'm ordered to finish your sprint. But mainly it's there for you to explain your code to someone else, because when you do that, it engages a different part of your brain, and often times your explanation to them, and/or their questions or comments, might cause you to remember something you forgot to do in the code, or might cause you to realize a better way to make it more readable or architect it better. That leads to more beautiful code."

I like to think of it as a show-and-tell. People get to show off their work to their peers. It's not about your peers finding things wrong in your work, which nobody likes the feeling of. It's about impressing your peers with your awesome code, which everybody likes the feeling of.

However I think using code review tools where there's no human interaction, no meeting in a room, no whiteboard.. it becomes just another annoying "thing" to do. It's not that there shouldn't be such tools, but they should be something you resort to if, during the code review meeting, it's realized that a more in-depth review of a certain section of code might be necessary. Then you could assign one of the junior devs to review the other one's code on a certain area.

  • +1 for engaging a different part of your brain. In my experience, especially when I was a junior dev, just knowing my code was going to be peer reviewed made me pay attention to details I may otherwise have ignored. – Laconic Droid Oct 20 '16 at 17:13
0

You can help by setting a good example. You can't get defensive when someone points out your mistakes. Do a code review on your own code and note areas of improvement. Share this with the team. Eventually, they will learn that this is encouraged and no one is going to get a beating for having a bug in their code.

Having a job means taking responsibility and pride in your work. If code review is a part of that, then participation in code review should be included in evaluations. I've taken online classes where participation in online discussions is a part of the grade. Comments need to be elaborated on. "I agree" is not acceptable.

Code review should improve code. Depending on your situation it could be measured by sales numbers, user complaints, or some other rating if you writing code for internal use. The reality is your code serves some purpose and your team should be measured by how well they serve that purpose. Those you determine contribute to the success, get to share proportionately in the rewards.

Focus on releasing quality code. The goal isn't for everyone to feel good about themselves by shying away from bugs. I write bad code; I have to fix the bad code. That is work and life. I hate fixing bugs, so I try to avoid them. I take pride in my work, so it bothers me when my code doesn't work. I feel bad for the users or anyone else who has to take their time to point these things out and it motivates me to want to fix it.

As a side note, if you have an environment where no one can give or accept constructive criticism, you have a problem.

-3

The process: Someone wants to commit his changes. Someone is assigned as the reviewer and reviews the changes. Then the reviewed and fixed changes go to testing.

If testing finds bugs introduced in the change, then author and reviewer are equally to blame.

So doing a review without any comments will get you into trouble unless the code was perfect.

  • 5
    1) Assigning "blame" for bugs is a great way to cause your staff to leave 2) Assigning blame to junior staff for failing to spot bugs written by senior staff is doubly bad. – Philip Kendall Oct 20 '16 at 6:38
  • 2
    @PhilipKendall If my code has a bug, no ones needs to blame me. I'm a professional and take real pride and responsibility for my work. Is this some sort of new age thing where no one does anything wrong and everyone gets a trophy for participation? – JeffO Oct 20 '16 at 10:41
  • @PhilipKendall: I don't know where you work... Where I work I'll say "what a stupid mistake I made" and the reviewer says "and I missed it as well" and then we both laugh. "Blame" means taking responsibility, not standing in the corner with a dunce hat. – gnasher729 Oct 20 '16 at 21:52
  • 1
    @gnasher729 Yep. But nobody gets "in trouble" for it. – Philip Kendall Oct 20 '16 at 22:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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