How to do code reviews to people whose programming skills are weak?

Question

I am a developer and I was recently promoted so I am doing significantly more code reviews now. Part of that includes doing code reviews on Github with people who are either not very competent or lack any programming skills at all.

For example, there is a person who seems to write code almost randomly, without verifying that it works, and doesn't seem to understand even the difference between () and {} in JavaScript.

For some other developers, I have to comment very frequently which sounds like I am picking on them, for example:

var CountryCode = "1787";
  if (CountryCode && CountryCode.length) {
 this.setState({...this.state
 countryCode: "1787"
}

For this, my comments would be:

• use const instead of var
• indent this and that line with 2/4/6 spaces
• variable you created is unused
• some conditions are not needed
• use single quotes instead of double quotes, for consistency and so on
• 1787 is not a country code, should be 1

I was trying to mitigate this by using tslint but they don't run it.

There is an extra weird dynamic that I am the only white male in the development team, so it feels extremely strange to pick on minority colleagues and appear hostile to them.

I was trying to co-opt other developers by letting them do code reviews or answers questions from less skilled colleagues, but they keep coming to me.

I am quite hopeless about what to do, but I have been at this company for only 7 months so it would look weird on my resume if I leave.

Answer 1

One thing I did when I became lead was to create a best practices and coding standards document. I included everything right down to naming conventions for variables, objects, and procedures.

Code reviews are useless unless, and until, a well documented set of best practices and procedures have been established.

THIS

• use const instead of var
• indent this and that line with 2/4/6 spaces
• variable you created is unused
• some conditions are not needed
• use single quotes instead of double quotes, for consistency and so on
• 1787 is not a country code, should be 1

Could be THIS

As per our best practices, I've noted the following

• use const instead of var (see page 5)
• indent this and that line with 2/4/6 spaces (see page 32 on indents)
• variable you created is unused (see page 15 on variable usage)
• some conditions are not needed (See page 27)
• use single quotes instead of double quotes, for consistency and so on (page 11, code consistency)
• 1787 is not a country code, should be 1

Having set standards also eliminates your concerns about any perceived biases. So long as you are consistent, maintain high standards for yourself as well, and have set standards and expectations, you should be fine.

Another point, raise by Rhayene: If the entire code base is not yet at the point it should be, increase the set of rules in planned stages over time, to bring everyone under the same standards. You will probably get less pushback that way.

Answer 2

How to do code reviews to people whose programming skills are weak?

Your task is to review their submitted code, not to evaluate their competency at their programming language. You should treat each review the same regardless of your personal feelings about the programmer. As long as you are consistent with your comments and corrections among all of the code you review, there should be no fear of "picking" on your part.

Answer 3

Code reviews are an opportunity for you to provide knowledge and insight on the work that your colleagues have done. This is also an opportunity for you to learn from them. Looking at a review like that, there are three areas that I look for.

1. Linting/Style issues -

If you have a style guide in the company, it should be accessible and ideally enforced prior to the review process. If style issues arise at this point, I would kindly remind the developer of the rule and ask them to keep it consistent with the style guide. Highlight that the point of a style guide is to keep the code consistent and easily readable. If the developers are having difficulty with running a linter or what have you, set up a meeting with them and show them. Answer their questions. Part of your job as the review is to make sure that the person is able to fix their mistakes and insure that they do not continue to be problems in the future.

2. Logical issues -

When you encounter something that seems out of place, be kind about it. Double check to see if there's something that you didn't see. Ask them questions instead of demanding changes. When you ask them why they did X instead of Y, they may have insight that you don't. If you do suggest a change, do it politely. Instead of

use const instead of var

You could say something like:

I see that CountryCode was not modified, is this intended? Would it be better as a const to prevent unintended modifications?

This will help you better understand the decisions that went into the code, as well as express your concerns in easily understood terms. If you just say to change things without explaining it, it could be interpreted as picking. By asking questions, you are making an effort to understand their thought process and see how they solved the problem they were tasked with. When suggesting changes in this manner, you're providing the developer with reasons as to why these changes should be made. This helps make the reasons for the requested changes more easily understandable.

3. Good Ideas -

This is an important one. Code Reviews don't always have to be negative. Commend the developer where possible. If you see something good, point it out. Give them credit for the good in addition to the bad. I try to point out something good whenever possible. It helps to reinforce the idea that you're not just looking for bad. That when you're reviewing the code, you aren't just trying to impart knowledge to the developer, you are also learning from them.

Just remember that in the end you're all trying to do the same thing, learn and write better code. You can learn from them, they can learn from you. In the end everyone improves. The best way to help everyone is to explain your thoughts and most importantly, do it kindly.

Answer 4

It's kind of disturbing that much of these answers are trying to interpret a serious pedagogy problem as a technical issue about linting.

If the submitters are scraping together code that barely even works, putting that through a code review is only slightly better for them then reading compiler error messages.

You should see this as a coaching role and that means communicating with the submitters collectively and individually. It's A LOT MORE WORK than just coming up with the right comments for your code review. For one thing, you'll need to actually demonstrate over and over how to put together code that will pass a review. It will take time and practice with repetition.

If you're not up to the task, you will need to push for proper training to get these folks up to speed.

Answer 5

Congratulations on your promotion.

Now that you are promoted to a new role, there are new responsibilities which comes with that role, and that is the code review.

From your description, it seems either

• The organization / team does not have a coding guidelines and best practices rule-book.
• The team does not pay any heed to the existing guidelines.

Either way, this is going to end up causing wasted resource-hours, as many of them need to be re-worked.

What I'd suggest is that, instead to trying to correct each individual separately, call for a meeting / discussion / knowledge sharing sessions with the team and provide examples of improper coding practice/conventions that you have encountered during the previous reviews and how they can be improved.

Also, provide them with the references to the company coding guide(if one exists), or create your own guide and make that available to them.

Insist on having peer-review done before raising the pull request to have the code verified against the given rules.

For example:

I was trying to mitigate this by using tslint but they don't run it.

Maybe they don't use it because they simply don't know how to use it? Why don't you set up a hands-on demo and show them how useful linting can be, and how much back-and-forth communication and time it can save for everyone?

Then, once you have explained them the workings, put forward a rule that all pull requests must have associated lint run results. That way, you're empowering them and also guiding them toward better productivity.

Remember, you need not judge the individuals, you only need to judge the quality of the code and attack the problem at root: the lack of knowledge / proficiency.

Answer 6

Put it exactly the way you did to us, as that was a great review (except maybe for the "use single quotes" bit; that's nitpicking).

If the people who cannot program properly accuse you of racism, that's their problem. You're not being racist. You're literally doing your job. They'd need to learn to separate their shortcomings and their potential for learning how to do better at their work, from the colour of their skin.

Be factual and kind. That's it.

End of story!

Answer 7

A couple of points:

• Your colleagues should not be doing the same mistake more than once. Tell them to use spaced repetition learning with Anki. If they make the same mistake more than once, ask to see their Anki card dealing with the error they just made. At first, their Anki cards probably won't be good enough, but that's ok, focus on how they can improve those cards. On a side-note, do not allow them to copy the Anki cards of other people.

• The same goes for code that doesn't run or doesn't lint. Teach them how to lint their own code. And better still if you can automate that process for them.

• Forbid them to cut and paste code. If they want to copy code from someplace, they must type it out manually. If they don't do that, muscle memory is never going to kick in.

• Chances are that you are being too nice. If they're committing code before it's running. Or if they're coming to see you every time there is a minor issue, it means that you're rewarding their lack of effort with an answer each time. In other words, you are training them to be the way that they are. Instead, you should teach them how to rectify their own mistakes and teach them how to rectify their own learning, not give them the answer every time they come to you.

• And finally, consider letting go of the ones that are not progressing over time. Talk to your management about that. It's not pleasant, but it's something that every business has to do eventually, especially if your recruitment process wasn't very selective to begin with.

Answer 8

I, for one, believe it is quite complex, to not say impossible, to make people who know close to no programming to follow coding standards. I'd do the following:

1 - always be friendly in the code review. Never write "you did this wrong". Write "This can be improved by doing X, Y, Z". When they are more experienced you can even rephrase to something along the lines of "What do you think about doing X instead?"

2 - it might happen that they don't even know what is a code review and what it is for. Organize a metting to explain it's goals

3 - Talk to your boss to try to setup programming workshops, so people will improve with time

Then, after they have a good foothold and actually know how to program, I would go strong with the coding standards.

Answer 9

There are already very good answers here describing what you can do but I want to add some points to the how, from own experience (not as the lead but as a team member).

First, increasing the code quality to a new standard is not a sprint but a marathon. Suddenly setting up 100 linter rules in your CI won't get you to your goal. It will get people mad at you and/or they are getting very creative in circumventing your rules.

You have to cook the frog by increasing the heat gradually or it will leap out of your pot.

Automate

As others have said - everything that can be fixed automatically like formatting should be done so. Doing this manually is a waste of time. So set up a wiki site and document what steps are to be done to automate this in your IDEs on save or pre-commit scripts (whatever your team is more comfortable with). Documenting this has the advantage, that new members of your team are faster up to speed using this.

Rules

Here it gets trickier. Everyone has a different perspective and preferences - even when they are not bloody beginners. So write down the practices you like to see implemented. Sort them into groups must, should and nice to have. Write down, why these practices are good - what are the benefits and what can happen if you don't do this. I say write down, because you will be forced to think about and reevaluate them. Refreshing your understanding of these practices is good and you can drop rules you followed dogmatic until now without understanding them (if there are any), until you understand them yourself.

Take from the must group the ones that are easiest implemented and get your team into a meeting to discuss and explain these. And then listen. Programmers aren't always the best communicators, so what sounds like "meh, I don't wanna" first, may have an underlying issue that can be fixed. Be open for compromise. The benefit should stand in relation to the cost that is needed to implement each rule. Drop rules you can't implement without severely disrupting the workflow and killing productivity. This may be different between languages and their ecosystems.

This meeting should result in a documented standard that everyone is ok with.

Different measures for old and new code

If you have a project with 0% code documentation and add then a plugin like checkstyle, configured to break the build, you'll have hundreds of errors. This is frustrating because it is too much to fix in a reasonable time resulting in results like code comments alá todo. This is not what you want.

Enforce that new code has to meet the standard you all agreed upon.

Every old function touched, needs to look better than before, if possible. I say better, because refactoring to mint condition can take time you don't always get/have. Again, start here with the low hanging fruits.

Make it a team effort

What can work well is gamification. If you get points for each test you write and get the first place on the team score list - it may motivate the team member, that is now on place 2 to write a new test in response (of course, don't get carried away).

If you see a team member that implements the practices well - let them review too. Peer pressure can motivate to work more disciplined.

Do not - if you happen to have the power to decide that and can avoid it - let anyone work alone for prolonged periods. Even the most disciplined people will write better code if someone else can give immediate feedback, ask questions and clear misunderstandings.

Add new rules to your standard

After a while - get your team together again and discuss the next set of rules. Having a plan about when this happens avoids surprises. If, and how many new rules you all add is, of course dependent on how well the team copes with the last set. In this meeting you also have the chance to adjust existing rules to better portray reality.

New team members

As a new member it can be very intimidating to be confronted with a huge set of rules. Don't throw them into the cold water alone.

Edit:

I just reread the question and OP said that the less skilled members still come to them despite other members being able to review as well. If this happens often, then there is a reason. Maybe your reviews are better or there is an issue of trust with the other team member.

Answer 10

Senior I.T. guy here (20 years experience), I have been both on the "giving" and "receiving" end of code reviews, and what I can say is that language, approach, manners and praise can be very important to the person being reviewed.

Imagine you are a junior programmer, with good intentions, trying to code and learn and be better.

Reviewer A says: x is wrong, y is wrong, z is wrong.

Reviewer B says: thanks for the progress you made on this task, there's some really good work here, but may I suggest to you some improvements? x can be improved upon, let's look at it together... if I make this change, what do you think, do you agree it is better now? etc etc...

In other words, how you frame it, and whether you give positive reinforcement along with the review, the things you suggest can much more easily be interpreted as helpful suggestions rather than blunt criticim.

Sometimes as I.T. people we are very detail-oriented and less concerned about people-skills, tone, friendliness towards and emotions in our co-workers.

It sounds like a more patient approach, where you also always give praise along with review, can help here. :)

Answer 11

Start by making a meeting with the team to teach them best practicies, teach them how to code properly, and give them references to improve their skills (a good example is the book Clean Code). Once they have learnt how to be a better programmer, it is more legit for you to review them.

Answer 12

A lot of the answers above seem to be cautioning against nitpicking, which is understandable. Additionally, your writing seems to also want to caution against nitpicking.

Our development house takes a different approach. We appreciate nitpicking, but we lampshade it a bit. That is, when making a comment about a nit, we call it out as such.

nit: extra space

nit: unused variable

This includes things that linters often won't find, counter to the suggestions above:

nit: These two lines could instead be one, and it's more readable way.

or the opposite,

nit: This line is pretty long. It'd be more readable to split it into two.

This calls out to the person in question, "Hey, this is a minor thing, but is a change I think you should make." Note that this does not mean that nits can be ignored; they should be fixed. It just means that you're acknowledge that it's a minor deviation that may have only minor value, but is still for the betterment of the codebase.

Now, the suggestion of automating the running of linters is still a good one because picking nits may not be a good use of your time! But IMO it's an incomplete answer and doesn't get to the heart of how to give constructive, non-adversarial nitpicking feedback.

Answer 13

In addition to linters and other mentioned items I'd add.

If a new developer, instead of doing straight PR reviews, I'd do pair programming (maybe for a month or 6 weeks) especially if the remainder of the code base is in bad shape or there are not clearly defined standards. I'd this because people react very differently to the process of being criticized (ie being told that they are stupid is how they interpret it) for their code and, to be honest, it ensures that they understand what is being told to them (because it's done in person). Also, linters only have one correct answer and this allows trade-offs to be seen.

If the developer doesn't want to do this, he / she would be off the team.

Answer 14

Automation

Linting and automated checks are beneficial, but you are likely to get push-back from the team, as the checks will slow them down until they learn all the rules. Even so, I would even take it to the next level and make whatever IDE your team uses treat all warnings as errors, so your devs are forced to pay attention to everything the IDE notices.

Multiple Reviewers

Ask your team to support a policy that every change have at least 2 reviewers. That way, even if you end up looking at most of the reviews, you know someone else will also have to review the code. Also, publish the reviewer metrics, so the team sees who is pulling their weight on reviews and who is slacking. Even if someone is a bad coder, reviewing other code is a teaching experience for them. Teach your coworkers that it's just as important to ask questions on a code review as it is to suggest improvements. In this way, each review is an opportunity for bidirectional learning, which is why everyone needs to participate.

Pairing

I agree with the answers which identify this fundamentally as a teaching opportunity. I especially agree with the suggestion to pair program. It sounds like you basically need to train your entire team. If this isn't your cup of tea, then time to look for a new job. Otherwise, pairing is probably the most effective way to skill them up.

Team Reviews

Instead of doing code reviews by yourself, you should do at least some of them as a team. That is, schedule a meeting, invite several devs, and ask them to review the code, adding your comments and suggestions until everyone is on the same page. This lets you teach several people at once. Alternate between reviewing the worst code, which should fetch lots of comments, and your code, which should raise lots of questions ("Why did you do it that way? What does that line do?").

Books

If you have a favorite book containing best practices for your dev languages, ask your boss to buy a few copies for your team, and ask them to read it. For C++, Scott Meyers is a very good authority. For Java, you have Joshua Bloch, etc.

User Groups

It could be that your teammates see programming as a necessary evil, rather than their primary passion. There isn't a whole lot you can do about that, besides change companies. But if you feel they are on the fence, and they could be inspired to learn more, then you should try to see if there are relevant user groups that meet in your city, and invite them to join you there. They won't necessarily learn things that are directly useful to their daily work, but they should at least see the excitement of other devs about the language and libraries and frameworks. If you're lucky, some of that excitement will rub off onto your coworkers and motivate them to improve their skills.

Role Change

It could be that some of them would really rather be doing something else, like Program/Project Management, or even people management. Have a talk with each team member to gauge where they are at, what their ambitions are, where they see themselves in a few years, etc. If one of them expresses an interest in a different role, and you think they are hopelessly far behind on coding, then gently encourage them to explore that role, and do what you can as far as recommendations to managers to make that happen. Then tell your manager that you want to be on the hiring loop for any new coders on your team, and that you are going to raise the bar significantly. Really, this is something that the manager should be doing, but they may lack the experience or motivation to do so. If this helps you get better coders on your team, then it's worth getting your hands dirty.

Team Change

If your company has multiple dev teams, then you should either try to join the team with the best devs, or make your team that team. That means trying to manage the weakest coders onto a lesser team, and poach the good coders from other teams. Ideally, you should be working on the most impactful team (the one which delivers the most business value for the company). If so, then poaching good coders is actually beneficial for the company, up to a point. If you are not on that team, you should first try to get onto that team, and then build up your all-star squad.

You should, of course, recruit your manager into this task, as they will likely have much more influence and leverage than you do. You should explain that your team is actually delivering less than they would if you got rid of the weakest coders, because you spend so much time reviewing/fixing/undoing really bad code. But that they might be net positive on a different team, and so the company would benefit from a better alignment of devs with business projects. I.e., a little musical chairs. Of course, you want to scope out the work devs on other teams are doing, so you know which ones your manager should try to poach.

Naturally, the politics of this strategy can get quite messy, and it is not something everyone is willing to try. But at the end of the day, your company gets paid to deliver a product/service, so everyone who gets a paycheck should want the optimal configuration of workers + projects. Sometimes that means you need to make an all-star squad to work on the most important projects, and it may be that you are in the best position to help discover that.

Management

Don't forget that the all-star squad also needs the best managers, too. So if you get your hands dirty with trying to reshape the org chart, make sure you know how good each of the managers are. It seems likely that your team has weak coders because your managers is not a good judge of skill, and there is perhaps a better manager on another team that you would rather work for. That is a pretty important thing to consider, especially when it comes to further advancement.

Answer 15

It seems you are nowhere near the point where code reviews make sense. I looked at the JavaScript code, and it is godawful. When you write code, you make it work, then you clean it up to the point where you think it's Ok, and then you submit it for a code review.

The person who wrote this cannot possibly have thought this code is Ok. Or at least I hope they didn't. So with code like this your response shouldn't be a code review, it should be "make it work, clean it up, and then you can get a code review".

I don't think you should tell this person what's wrong with the code. Sit down with him and ask him "how could this code be improved". And see what happens. If it looks like he can't see what's wrong, then maybe you have someone here who should not be employed at your company.

Answer 16

You need to start from understanding:

• What's the cause for the situation
• What is possible for you to accomplish (not what is really good or desirable; if it cannot be done then it cannot be done)
• Your long-term plans

The cause for the situation is obvious - the company employed people without sufficient skills to do their job. The whole recruiting process - interview, testing etc - didn't work as it should, or this problem would not have existed in the first place.

The second cause - well, 99% of the people work based on stick and carrot. If the company is not willing to apply either or both as needed, there will be no improvement.

Yes, some people are on a higher level and don't need that, but most do, especially at the beginner level.

So, you cannot really get them to improve.

So, what do we conclude from this? The only thing you can accomplish is to cover your rear end.

It's a worthy goal in its own right. I'm merely saying, don't really hope for more than that.

In this light, having a clear rulebook about code will help you - in sense that you can tell to the management that you clearly posted the rules, and the rules are unbiased.

Code linting - well, it will reduce the noise, which will waste less of your time. A good advice. Just don't think that they'll start writing good, working code.

But this works only for very simple issues.

As the problems to be solved in code grow past the beginner stage, there will be a thousand situations that the linter won't recognize... and that a human will instantly recognize as a bad coding practice. What happens when those (unskilled) developers need to do something with multi-threading? Complex database queries? Client/server stuff? Anything past the basics?

Programmer workshops? I doubt it would help. If they wanted to learn, they would have learned already, or they would have asked for help. And this is something one learns for years and years. A few weeks of a programmer workshop is not going to turn anyone into a programmer, not any more than a few weeks of a crash course in boxing will turn an average Joe Shmoe into a pro boxer.

Strictly locked repository where no one can push a commit until it both passes the review and passes unit tests - this will help both you and the project in the long run. It will also cause the bad-code-authors to scream.

The rest is up to the company. Yes, it can and should organize some sort of learning and help, since it employed them in the first place. But it will be pointless unless there are consequences for not learning.