When I need to constructively criticize someone's code, what can I do to "soften the blow" and increase the impact at the same time?

Question

A junior developer was having some issues on a project. As the deadline got closer without any resolution in sight, my manager asked me to help out. I'm a mid-level developer on the team.

After getting the rundown, I saw that there were three main issues:

1. Major issues in the logic
2. Lack of knowledge for how to trace back an issue to the root cause
3. Very obscure naming conventions (this was very problematic due to the nature of the project).

I helped him solve the issues, but b/c of the deadline I had to be very "to the point" and didn't have enough time to discuss the "whys" for everything I was recommending & helping him with. We hit the deadline in time, and now I'd like to go back & do a 1:1 post-mortem "code review", giving him a crash course tutorial on the 3 issues I mentioned above.

I have a good rapport with him, but this code review will literally shred apart the solution he built. I'm worried about this for a few reasons:

• He's very soft spoken & seems to take things to heart sometimes.
• He has only been with the company for 6 months and it's his first job out of college. I've gotten the impression that he's under a lot of scrutiny from our boss about his performance. Having a harsh code review at this time might be too hard on him (although it should certainly help him improve performance!).
• He doesn't seem to take good notes during reviews/trainings, and he also isn't very proactive about asking questions when he gets stuck on tasks. As such, I'm not so sure how much this code review will really "stick" in his brain.

So the bottom line questions are:

1. I'll obviously make sure to emphasize anything he did right & will speak as kindly as possibly, but what else can I do to soften the blow when I go through the laundry list of the things he did wrong in the code?
2. Given his personality & lack of note-taking, what are some methods to increase the impact of this training/review? I truly want him to succeed at the company, but having seen his work I don't think he will succeed unless he really makes an effort to learn the best practices & methods that everyone else on the team uses. One code review meeting probably won't do the trick.

One side note - Due to the nature of our work (I'm leaving out details for privacy), there is no option for automated code reviews before pushing the code to "production". This makes it harder to catch the types of issues I described above.

Also, I did read this & this question but the focus of my question is pretty different than either of those.

Answer 1

There's a couple of things that I've found help a lot with the reception of code reviews:

Don't say "you", or variations on it ("your code", etc).

You're not ripping apart "his" code or "his" solution. It's our code, or this solution, and we need to do things to it.

As soon as someone starts talking about "your" code, a lot of people's natural reaction is to become defensive, or even combative - especially if they don't feel very confident to start with. If you avoid implying ownership of the code to the junior developer then they will feel less like they're being attacked. The company owns the code, after all.

Instead use language that makes it clear that the team is working together to improve both the quality of the product, and the skills of the team members.

Use positive language, and avoid being negative as much as possible.

For example, instead of talking about why the naming convention chosen by the junior is bad, talk about the benefits of the convention that you're recommending.

Similarly with logic problems, rather than spending ages talking about how bad the first solution is, quickly touch on the major problems but spend the majority of the time explaining a better way to do it, and the benefits that come with the second way.

Answer 2

I have been in such a situation a lot of times. And the first thing I usually say is: sorry....

Yes, right... because if he did not get easiest things right, like naming conventions, that means that you (or whoever was responsible for him) did not make a good job in introducing them to him...

So apologize for not having given him enough information to do a good job. Even if this is only partly true, this will make him feel understood and he will see, that you don‘t want to blame him for something he did wrong, but to improve together as a team as a learning process for both of you.

When you then go through the code, the most important thing is to make him understand his logical flaws instead of telling him what's wrong.

I personally use something quite like „rubber duck debugging“ in that case: make him explain what his code is meant to do. Now there are two possibilities:

First: If his understanding of the task is right, but the code is wrong, then go deeper into the code review.

Try to make him realize - by asking him questions about „what will the code do, if....“- what is wrong with the code and make him try to fix it himself. The same with the variables: ask him which names he would give them now, after you taught him the right naming conventions.

Second: If his understanding of what was recommended is already false, then explain him what the task really was about and let him explain how he would implement that.

That way you don’t destroy his solution, but make him improve it himself. And even if after that time the new code is 100% different from what he had first, this new code is still „his“ code.

TL;DR: take the responsibility for the quality of the code by apologizing for not having told him enough what to do / what to expect, and then let him correct his code himself.

In other words (very famous quote):

Tell me and I forget,
teach me and I remember,
involve me and I learn

Answer 3

First of all, don't insist on a particular way of taking feedback and learning from it.

In particular, not taking notes on paper is not necessarily a sign that the junior does not get anything/enough out of the code review. For most "programming style" code reviews I've never taken paper notes, because I rather focus on talking about the code and thinking about the "rules" and their benefits during that time where I have someone to discuss this right next to me than being busy writing stuff down. And I have the "notes" anyway - in the code repository! (At least I should have!). Focussing on the actual issue helps me a lot more than writing stuff down and then tossing the piece of paper on a heap of other notes never to look at again. If being forced to appear taking notes, all I'd take from the meeting is that I'd not like to work with that senior because he's all about having everything his way.

So: yes, please suggest options, e.g. by asking whether the junior needs his notebook, but don't impose your way to learn on them. Rather, as far as possible, make sure the junior has the relevant information available afterwards anyway - e.g. by having the naming conventions written down somewhere in a wiki for everyone to look up and by committing necessary changes identified in the review - or at least some examples - into a review branch.

If you provide code reviews only orally at the moment, explaining the Junior what he did wrong in a conversation, I'd strongly suggest to either directly make sure to commit changes you suggest in the respective code or make the junior do the changes and then re-check the code. In the latter case, keep the amount of rework small per session and rather do multiple sessions. If you already did the changes point them out in the code repository, such that the Junior can remember the places rather than random examples not connected to your daily work.

Show him he's not alone, go public

As for how to make the code review open and not intimidating, I agree with Player One's answer and would add one more suggestion: Don't just have code reviews with him. Make code reviews an official regular part of your process and do them with everyone.

Furthermore, if you feel he is defensive, make (some of) the code reviews with the whole group, but start with some of your more senior colleagues and treat them the exact same way you would treat him: Talk about their code, not them. There is nearly always something worth discussing about even in code from seniors. And if it only is a case of "yeah it's done that way because of a),b) and c), but one could have made it more generic/following principle X by doing Z, but that would have drawback Y". Make sure you use the same tone and neutrality for everyone. This shows the junior that you are not trying to bully him with your stupid rules but that everyone is under the same scrutiny and that it aren't "your" stupid rules, but the team's common rules - preferably based on benefits the team as a whole likes to have. In addition, the junior can also learn from the mistakes (or great design choices) of others. That makes it even less personal for him and gives him opportunity to get in touch with other business logic areas and software principles before having to work on them - thus he may have the necessary time to "digest" them before having to apply / work on them.

Answer 4

When someone messes up badly at the beginner level, it just means they need to grasp the basics. I don't analyse their mistakes and try and teach that way. I already solved those, and there was a probably a bit of cursing going on so old news.

I just run through the basics in detail. By that I mean the conventions, procedures, etc. Importantly I teach him how to plan a project before starting, and I'll check the next one. They bring their issues to me if they don't understand something.

I don't want to impress him with how good I am. I want him to learn how to get things done without me having to babysit on top of other tasks.

Answer 5

The situation isn't bad, it is good. Once you get your head around this fact, the talk will be much easier.

• The project was finished on time.

• You have the time and expertise to further improve the project that already does what it is supposed to do.

It is perfectly natural in software engineering to have architectures that are not optimal and to improve them.

As for style issues, such as naming conventions, people usually follow them once they know about them. Just explain them once and he should be fine. Do check on him though.

Answer 6

• Major issues in the logic, lack of knowledge for how to trace back an issue to the root cause, and very obscure naming conventions (this was very problematic due to the nature of the project).

I would start with the easiest first.

"Naming conventions"

• Your company probably has a guide. The book Code Complete 2 can be a pretty good guide too. It explains the whys and gives a checklist.
• I would make him produce his own cheat sheet and have him tape it to his own computer monitor.
• If he doesn't get into the habit of following the company conventions or his own cheat sheet. Force him to maintain a paper and pencil lexicon of all the variable names he uses in a table format. That too should always be at his desk next to his keyboard.

Next, I would tackle "how to trace back an issue to the root cause".

This is not a simple issue, it's probably going to take multiple sessions to cover every technique comprehensively. Nor can you rush him. Even if you tell him everything at once, he's not going to absorb everything in one go.

It may even be possible he already knows what to do, but is just not doing those things. So you should quiz him on those topics and see how he handles those problems in practice with minimal guidance on your part.

As to the problem of "Major issues in the logic". Those are results. If you help him fix the underlying processes he uses, then the results should improve. You can explain his logic errors if you like, but don't expect that this will be enough.

Think of yourself as a teacher. You can't overwhelm a student with too many corrections. Instead, you have to help that student focus on fixing the easiest mistakes to correct and on fixing some of the more foundational mistakes, even if that means not addressing the rest of all his mistakes.

Over successive code reviews, you can then call attention to the improvements he has been making over time, all the while, slowly addressing the other types of mistakes he might still be making.

Answer 7

By your use of the quotes in "1:1 post-mortem 'code review'", I take it that this is an informal process that you can shape the way you want. In that case, I would recommend:

Instead of starting with the junior developer's code, start from scratch.

This has the following advantages in my opnion:

• It won't feel like anything is being "ripped apart", because it's not.
• You'll be able to more fully demonstrate the good practices you want the junior developer to learn.
• For the parts of the programming that the junior developer did well, you can say "and now let's do this part exactly like you did the first time"; that will feel like positives interspersed among the joint code-building (whereas in a "ripping apart" session it would feel like neutrals interspersed among the criticisms).

On a side note, you are indeed trying to mentor this developer and install skills that will benefit the team in the future. One of those skills is how to learn from feedback and to have that learning endure into the future. So, in that context:

Explicitly guide them on how to take notes during a feedback session. That's a skill that can be learned consciously just as much as any other skill.

Of course you'll want to frame the meeting so that you have buy-in from the junior developer. Presumably, being honest that your goal is to help them improve their skills, in a way that they will find satisfying and for which they will be recognized, should be enough. One framing technique you can use is:

Always talk about skills in a growth mindset—that is, skills are something that everybody gets better at over time, with practice, effort, and conscious self-assessment. The opposite would be a fixed mindset: programming is something that people can either do or not do coming in the door. The combination of a fixed-mindset frame and facing one's own errors is typically what triggers defensiveness and closed-mindedness. But in a growth-mindset frame, facing one's own errors is more easily seen as a golden opportunity to learn.

Finally, as a general rule:

Clearly communicated expectations is a huge part of any process involving more than one person.

Answer 8

Pairing

Instead of a code review, I would offer to do pair programming with him instead. It doesn't matter if it's on your project or his, but it can be helpful to some of both. When he is typing, and you notice that something can be improved, just ask gentle leading questions like: "Do you think there's any way to improve that?" Let him work through some options, and then use hints to guide him to your solution. However, try to analyze each option he offers to help explain the rationale and pros/cons. After all, some options are better than others without being the best option, and understand the reasons is important. Whenever he gives a good answer or surprises you with his knowledge, praise that so he gets some rewards during the pairing.

When you are typing, make sure he knows to ask questions whenever you do something surprising or differently than how you would do it. If you know you are about to do something which is different from how he solved the similar problem in his code, then pause and ask him for suggestions, talking through the alternatives.

Also, the most important conventions are hopefully practiced and promoted by the team. In this case, you should be able to find code that neither one of you wrote which illustrates the points you wish to make. Find some examples and discuss, so that he can see that just reading existing code is a good way to learn, and that he should learn from existing code. Let him know that copying code style and patterns is a virtue, and one that he should practice until he gets more experience. Of course, that means he will copy some bad code habits as well, but that is the price of learning.

Socratic Method

In general, people learn better when they feel they have arrived at the solution themselves, especially if they have a "Eureka!" moment. Thus, instead of saying: "You should do X. It's best" I prefer to say: "I notice you do X. Have you seen anyone solving this in a different way? Oh, why do you suppose that Alice used Y instead of X? What about this scenario? Which solution do you think is better?" So, by asking leading questions, you can challenge the status quo and guide your coworker down a more enlightened path. Sometimes, you just need to offer some real-world scenarios which illustrate why Y is better than X, even if you don't have any code examples handy. But if I can get someone to the point where they say the principle I am getting at, I find that it sticks much better, because they arrived at that point via their own answers and deductions, which means that they agree with the principle.

Testing

One of the three things which no software engineer does enough of is testing. Most likely, your junior partner is not an expert on unit testing. Perhaps you are also not an expert. Either way, spending a lot of time writing unit tests and using them to exercise code is an excellent way to show off the pros and cons of a particular technique. You have ready-made minimal test cases that can be easily tweaked to emulate different scenarios. Especially for logic defects, showing how to identify good boundary conditions for test cases as well as exercising all of the code paths will help tremendously in making his code more robust. You can also demonstrate how running unit tests helps isolate problems by giving you confidence in some methods while leaving others suspect. This is often an easier divide-n-conquer than bisecting the code with comments.

The important part is that doing all of these things together, simultaneously, is much more effective than giving a lecture in which much of the information is "lost" because your student didn't take notes. When you've introduced a new concept, make sure you give your partner an opportunity to exercise it, so you can offer guidance and feedback on its application. Try to do this both soon after teaching, as well as much later, to check retention. Writing code as a pair reinforces that you are on the same team, working towards the same goals, and that the process is about learning, not judgment. My guess is that you will skill up the junior very quickly, as well as build your own leadership and mentoring skills, which is always a nice accomplishment to share with the boss.

Answer 9

I would not focus too much on your current experience with their code.

What I think works best for all junior developers is to work closely together with mid-level and senior developers. That might not always be possible.

But the company should at least provide regular weekly one-on-ones between junior developers and mid-level or even better senior developers. The topic of that one-on-one meeting can be everything the junior developer is interested in:

• a current task they had problems with
• a blog article they read and didn't understand
• some hint on the implementation of an upcoming story
• in detail training based on their latest pull request

The key is:

• the meeting is one-on-one. That allows the junior to ask everything without being ashamed that they might ask something obvious in front of others
• the junior drives the meeting. It is their time to ask everything
• the meeting is not with their manager, and it is not about their performance. It is with a senior who is interested in training.

Answer 10

Learning to code is all about writing it, or at least watching someone else write it. Therefore, I think it would help if he watched what you do on a screen (preferably in an IDE he prefers or is familiar with, even if you use something different), so he can see what the process of doing it better looks like. Let's discuss some examples:

Major issues in the logic

In other words, the code doesn't do what it should; it has a bug. Explain one to him, and how you spotted it, and what code would correct this. But how do you do all that? Well...

lack of knowledge for how to trace back an issue to the root cause

Related to the previous point, show him how you would diagnose and solve the above bug(s).

obscure naming conventions

Don't make this merely about saying, "we want things named this way". Show the best ways to change the names. The IDE makes that easier; depending on what it's set up to detect, it might also suggest or even automate certain changes. If you have such things setup but he doesn't, change that situation for him, or show him how to do it.

Note that this approach isn't just about bringing in intimidatingly long notes you wrote to say, "please remember all these rules". It's about showing him how to do what he's meant to do. It teaches the savoir-faire.

[I] didn't have enough time to discuss the "whys" for everything I was recommending & helping him with.

I think what I've suggested helps with this time issue too. I needn't tell someone with your experience how much the code can improve, before an onlooker's eyes, with a few minutes of these tricks. He might find it a pleasant surprise, though, depending on how many ropes he's been shown in the past few months.

He's very soft spoken & seems to take things to heart sometimes.

@PlayerOne's excellent answer highlighted the need for making "this needs to be improved like this" an impersonal observation. This is easily incorporated into the above strategy. The phrase "let me show you how to do it" will be more or less personal depending on how it's said, but the noblest spirit of what it's saying is the whole point of this exercise.

he's under a lot of scrutiny from our boss about his performance

If this is worrying him, it can motivate his agreeing to follow your demonstration. If you don't know he's worried about it, you needn't bring it up, or moderate the above approach based on it. I'm sure if you followed my suggestion you'd do it in a fear-mitigating way, anyway.

He doesn't seem to take good notes during reviews/trainings, and he also isn't very proactive about asking questions when he gets stuck on tasks. As such, I'm not so sure how much this code review will really "stick" in his brain.

That's a definite reason reading out your notes has a reduced chance of success, regardless of how mean or kind you are with it. I can't prove seeing it in action, as something that happens when we interact with the living, breathing beast that the code is, would work any better. But the idea that it would comports with my experience. Invite him to ask you to pause or slow down when he wants to write down something he chooses to think especially warrants it.

what else can I do to soften the blow when I go through the laundry list of the things he did wrong in the code?

You don't need a laundry list - not all in one session, anyway. The code he submitted's not quite right. That's fine: the code he had to work on in the first place wasn't quite right either, at least not for modern requirements. The fact you're starting from "his" code in this in-person demonstration is beside the point. It shows him how to address any code he encounters. You might need to do it more than once, depending on how many things he needs to pick up on; you can gauge, a few days after he's digested the first session, what would be mutually agreeable, given his and the team's needs. But teach him the key strategies, and the issues that arose this time will take care of themselves in the long run.

what are some methods to increase the impact of this training/review? ... I don't think he will succeed unless he really makes an effort to learn the best practices & methods that everyone else on the team uses.

I'll leave it to you to decide the extent to which you should invite him to "take the wheel" at moments you hope he can infer what to do based on an early part of the demonstration. Apart from that, I have one suggestion that's separate from the general premise of this answer: recommend a good guide for him to read (or watch, if you know of really good video material). One guide, for now, or maybe two or three very short ones. If it's a guide that helped you, further guides will have diminishing returns, so don't overload him, or seem like you might.

there is no option for automated code reviews before pushing the code to "production". This makes it harder to catch the types of issues I described above.

OK, time for one more idea. Tell him he can ask you to look at his code just before he submits it, to see what you think of it. If you can, make such code reviews, even if they require pulling a chair over instead of using something you can't have on this project, more common in the team. This isn't just about not singling him out. The last company I worked for felt all code should have two non-authors review it before a push. Maybe he could be one of two reviewers for someone's code, even your code! All of this should only be introduced gradually, of course, as he might not have the right abilities, or be confident in them, to be such a reviewer straightaway.

Answer 11

When dealing with major corrections to someone else's work, issues that involve ego, it is very difficult to get the party who needs the help to accept the correction, and the worse the mistakes, the more difficult it is.

The way to get someone to accept change is to get them to realize it themselves. You can say that you need to review the code and ask the programmer to step through it, but try to do everything as a question. Try to get the junior programmer to figure out for themselves. For example:

int xj6b7 = 99.2;

Can you see anything wrong with this declaration? Can you see why another programmer might have a problem with understanding this? Can you see any problem with the constant 99.2? He won't necessarily find it. You can do this one issue at a time. You can reasonably insist that the junior remembers to take notes so they can go back and do it again.

x = x / 2; // divide x by 2

Can you tell me why you wrote that comment? To whom are you trying to explain this code?

And yes, it is better to break it up and not try to cover every problem with this person's code in one session. Give them a simpler assignment, finer grained and let them develop skills in smaller increments, it was a mistake to give them this big a chunk evidently.

Answer 12

How successful this is depends on the personality of the learner (and on my talent at applying it). What I try to do in such cases is to ask questions instead of making pronouncements.

Examples:

• “What does this subprogram do? …… Oh, then maybe we could call it that instead of calling it ‘M400’”
• “Is this loop doing something different from that one? …… Oh, then could they be factored into a subprogram?”

Answer 13

There are representative 2 quotes I've come across in my time as a developer that I've really latched onto, and like to repeat when discussing the issues involved here. This situation needs to address both of them respectfully - your coaching is a valuable gift to someone new to the industry, but the way you present that gift will make a huge difference in how it's valued.

Self-awareness and asking for help in the context of a team and the larger ecosystem:
"The average developer has the same ego as the average jet pilot."

Now, both characters here are nice people, and (Top Gun caricatures aside) are probably perfectly respectful to others. The point here is that they have both had a lot of training to develop their skills in a highly specialized field, and that results in a lot of self confidence in their abilities. Which is perfectly appropriate and a vital part of performing both jobs.

The problem on the dev side is that we start to believe we should know everything and be able to tackle any problem, usually by ourselves. We will sink hours and days into researching and attempting solutions on our own, rather than reveal our ignorance by asking a coworker for their input and domain knowledge. And we will probably come up with a great solution, given enough time.

But coding isn't about personal knowledge or skill, and it's definitely not about self-image. Devs are paid to solve problems effectively, and we should not be spending 3 days doing something by ourselves instead of an hour getting help when both give the same result for the project owner. Even if that's us on our own projects.

Developer time is one of the highest expenses on a project, and it's a vital skill to manage your own time well. Whether it's time boxing, Pomodro technique, or anything else, devs need to learn to collaborate and maximize the use of their time, not maximize their skill set.

Separating yourself from your work product:
"You are not your code."

There have been plenty of good articles written on this over the years (like this, or this). Any crafts-person who has spent so much time developing their skills and building something, will feel their self-worth connected to the end product. After all, its quality and usefulness says something about your own.

But again, the value of code to the business and the users is how well that code works, not how smart or independent the person who built it is. A developer's value is not the code they've already produced; it's their ability to create useful code in the future. And the longer they've worked with a given technology or business, the higher that value should be - but the only way for it to increase is to put time and effort into it. And that means feedback on completed work, and (hopefully) more exposure to the goals of the business, not just a task list.

Code reviews improve the code, yes, but they should also improve the developer's skill set and knowledge of the problems the code is supposed to address. Don't focus on telling a dev that they are wrong, because their value is not what's under review at this point. The correctness and usefulness of that chunk of code is being reviewed, just like the inspection of parts coming out of an assembly line are quality checked, not the worker running the machine. That's why it's a code review, not a developer review.

Of course, uncovering a pattern of continued flaws in the end product of an assembly line may guide long term action (e.g. more training or investigating additional problems further up the line), but that's completely secondary to the immediate short-term goal - ensuring that the already-finished work can be shipped out. Ideally that also helps the dev improve their own work.

But that will only happen if the reviewer focuses on the resulting product and (where appropriate) how the existing tools are intended to be used and what other tools are available. Discussing the ways that the person is mis-using or ignoring the tools wrong doesn't help them do it better, it just makes them embarrassed and resentful.

Answer 14

"I'll never forget my first job out of college, either. (Even though it was 'at the college.')"

Here's my suggestion:

"There is one thing that both of you objectively have in common: the source-code that was produced."

And also:

"The process that led ("let's both wink-wink pretend that (s)he's anonymous ...") Developer-X to produce Source-Code-Y in response to Requirement-Z."

Talk about the work product ... the source code ... and the work process. Not about the worker. Make it absolutely(!) clear that the person's employment is not at risk ... "if we didn't think you were good at this, we never would have hired you." Make it clear that the company is firmly committed to now help this person grow.

Full disclosure: "The first computer program I ever wrote [in BASIC] was eight lines long, took me six months to write [in the late 1970's], and had a bug in it."

"Computer software development is a damned difficult thing to do," and we need to always make it very clear that we understand that. "Yes, we know it's hard, and ... we've got your back."

Answer 15

Ensure that he brings a pen and paper to the code review.

Before the code review, tell him to bring a pen and paper, and that you will expect him to take notes. If he shows up without them, instruct him to return to his desk, collect them, and then return once he does. Do not start it until he has the materials he requires. Make sure to pause during the review after each point so that he can take appropriate notes.

Then, once he's ready to begin, rip apart his code, and explain why it's bad and why your way is better. Stick purely to the facts; don't speculate about his motives. If there's anything he's done well, begin and end your code review with those areas - the "sandwich technique" for feedback. If he starts getting emotional or defensive, stop for a moment, and make sure that it's clear to him that you're not attacking him, you're trying to help him. He can't improve if he doesn't understand what areas he's not doing well in, after all.

Answer 16

Split two things:

• the formal part
  1. naming conventions
  2. unused code
  3. code formatting
  4. style of comments
• The experience/logics/architecture part

The formal part you have to address in a "sorry, i am your boss/project lead", do it. If you don't do it then there will be disciplinary consequences. Tell me how much time you need and we will plan for that.

For the rest, ask him/her to show you a sketch or outline of how a new code will be implemented before starting it and discuss with you. Declare certain things to be off-limits for now (I did that in a project when it came to touching the data structure).