52

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.

16 Answers 16

73

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.

  • 6
    If the entire code base is not yet at the point it should be, it is also helpful to increase the set of rules in planned stages - giving them reasonable goals to meet. People tend to refuse out of frustration if there is too much to fix at once. – Rhayene Dec 11 '19 at 16:03
  • @Rhayene excellent point, thank you, I will add that. – Old_Lamplighter Dec 11 '19 at 16:07
  • 5
    Another point I found by thinking about this: Make sure that the document contains an explanation for each rule - why it is a good practice. And be brave enough to throw out rules whose benefit does not stand in relation to the cost to meet it. We had that one discussion about alphabetically sorted import statements that practically made auto-import from our IDE useless because you had to manually correct it anyway. Wasn't well received ;D – Rhayene Dec 11 '19 at 16:36
  • 1
    Exactly, setting expectations is key - you cannot say "be reasonable" because everyone has a different idea what that means. Additionally some parts of this can be enforced automatically, with formatting tools or checkers, a requirement to build without errors/warnings and ideally a requirement to pass regression tests. – Chris Stratton Dec 11 '19 at 18:44
  • 2
    one thing to add: once you have the standards/best practices, do not attempt a code review until these standards have been met, simply refer them to the documentation. given that he's already added tslint and they haven't used it, there's no reason to assume they will refer to this otherwise – aw04 Dec 12 '19 at 21:28
15

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.

  • 19
    This sort of misses the key point that code should not even be submitted for review until it has passed through the selected linting tool. – Chris Stratton Dec 11 '19 at 18:47
  • 1
    @ChrisStratton That's a separate issue if it is allowed to be submitted for review, in that case OP should speak with their boss to put some sort of filter in place prior to review. – sf02 Dec 11 '19 at 18:48
  • 1
    It's not really separate though since it's a large part of what wastes reviewer's time addressing the same issues over and over. Peer review is supposed to only come into play when the author can no longer see anything wrong with the code themselves, and now needs the view of other eyes for what they missed. An author who skips steps hasn't done their job yet. Code that hasn't passed that should be immediately rejected raising the lint issue alone, especially since running it will probably bump line numbers of everything else. – Chris Stratton Dec 11 '19 at 19:00
  • @ChrisStratton "Code that hasn't passed that should be immediately rejected raising the lint issue alone" I agree, however the code needs to be reviewed to determine that this is the case which is why OP should reach speak to the boss to find a way to prevent that code from being submitted in the first place. – sf02 Dec 11 '19 at 20:30
  • @ChrisStratton Not going to argue whether it's right or wrong, but just point out that I don't remember this being done at any employer I've had on any project I've ever worked. The closest thing I've ever encountered to this was a team which had a code format file for eclipse and people were told to install it if they used eclipse, and if the lead didn't like your formatting he would check out the code, open in eclipse, select "format code", and submit it back and complain at you. But generally nothing was done. – Aaron Dec 12 '19 at 0:20
7

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.

  1. 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.

  1. 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.

  • Regarding number 2 - the issue is not that const is immutable, while var is - there is a whole host of other differences between the two. Basically, var is the old way of declaring variables and it has different effects than the newer syntax const and let which are equivalent sans the mutability of the value they hold. There is no value of using var over any of const or let. However, with this aside - let's just assume it was using let. This kind of issue you don't have to engage in a conversation about why . It's simply wrong. A discussion wastes time on both sides. – VLAZ Dec 12 '19 at 7:32
  • Thank you for actually answering the question instead of focusing on a linter/style guide :) – Mars Dec 12 '19 at 8:59
  • 1
    @VLAZ Admittedly my knowledge of JS is rusty so maybe the example chosen was poor, but the points still remain. If this is something that is objectively wrong in such a way, it should be covered via point 1 with a style guide where this type of thing would be addressed. Regardless of how objectively wrong it may be, there is absolutely a kind and respectful way to mention that. The code may be the target of review, but a person wrote it. Being rude or flippant regarding the code could be taken as a slight against the developer. – jlowe Dec 12 '19 at 13:48
7

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.

  • the changing javascript variables referenced in initial question is probably the slippery slope – timpone Dec 12 '19 at 0:41
6

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.

  • "Maybe they don't use it because they simply don't know how to use it?" anecdotal, I suppose, but when I was introducing linting to my team I wrote a step-by-step guide on how to enable and use it. With screenshots and simple to follow steps to enable and use it in the editors my team used for code (two of them). Then we had a meeting of me showing them what linting is and why it's good. We had dome older code that everybody complained about due to its style, so I demonstrated formatting it in a moment and got a lot of enthusiasm. I also chose simple configuration for style - it was – VLAZ Dec 12 '19 at 7:40
  • reinforcing the dominant style within our codebase and I tried to avoid minute superfluous changes (e.g., if(cond) vs if (cond) - spacing) as it was all over the place. So, the style auto-fixing would cause minimal disruption in most circumstances. Once people were onboard with the idea, we started implementing more rules when needed. Also I found that some people simply refused to do a code review on unformatted code. They would simply write a comment to that effect, something like "It appears that the style is inconsistent, please format before the review". – VLAZ Dec 12 '19 at 7:40
  • "Insist on having peer-review done before raising the pull request to have the code verified against the given rules." Eh? The PR is the peer review. – Lightness Races with Monica Dec 12 '19 at 11:12
  • @LightnessRaceswithMonica PR == Pull Request. – Sourav Ghosh Dec 12 '19 at 12:03
  • Thank you, @Sourav, I know what a PR is. Unless your version control software is from the 1980s, pull requests include code review and comments before the pull request is approved. That's where code review takes place. – Lightness Races with Monica Dec 12 '19 at 12:10
4

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!

4

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.

  • 7
    "Chances are that you are being too nice." YES. A spreading obsession with always being "nice" is overshadowing the need to actually give constructive feedback. For some reason. We'll all lose in the end if those zealots win. – Lightness Races with Monica Dec 11 '19 at 22:54
  • 5
    "Unless you are running a non-profit..." - A non-profit doesn't have the purpose of making profit, but it also has to be a non-loss company or it will go out of business. – gnasher729 Dec 12 '19 at 8:46
  • @gnasher729, Sure, I've just removed that phrase. – Stephan Branczyk Dec 12 '19 at 10:19
3

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.

  • For more abstract standards, sure, but you can certainly make a hard rule that no code is even reviewed until it's been run through a formatting tool and builds without errors. Building without warnings, passing automated tests, and having a good commit message are also typically wise requirements. – Chris Stratton Dec 11 '19 at 18:43
  • 7
    If something is wrong, you say it is wrong. When the heck did we become afraid of facts? Don't let people bully you into avoiding stating facts. – Lightness Races with Monica Dec 11 '19 at 22:56
  • @LightnessRaceswithMonica I completely agree. There is place for being soft - "I see you are using function X, but perhaps function Y is better suited here". It can be made better. But there are things that don't deserve this treatment - you shouldn't go "Oh, I see the code you've written doesn't compile. This can be improved by adding a semi-column at the end of the line" or "You can get a more better result if you sum the prices of items, instead of their IDs". If something is completely wrong, it has to be fixed. That's not a suggestion - it's literally a requirement. – VLAZ Dec 12 '19 at 7:46
  • @LightnessRaceswithMonica, this answer isn't saying not to call a spade a spade. It's saying that when there's a problem with the code, the reviewer should present it as a problem with the code rather than a problem with the programmer. It's a form of "Be Nice". – Peter Taylor Dec 12 '19 at 9:12
  • 1
    @PeterTaylor Yes, I understood the answer. – Lightness Races with Monica Dec 12 '19 at 11:17
3

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.

2

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. :)

  • 1
    I think your suggestion B is needlessly verbose and slow to read (and let's not call things "suggestions" when they are not; they are hard requirements from your boss) - but I definitely agree that praising the bits that are right is vital to morale. Don't only point out problems - point out successes! – Lightness Races with Monica Dec 12 '19 at 11:23
  • @LightnessRaceswithMonica sure, you can use less words. My suggestion in the scenario above was a verbal code review situation. If you are doing code review online (via Pull Requests), consider for example leaving a review comment like, "fix x" with "can x be fixed with abc?". There's a big difference in how the feedback is framed (and interpreted) – vikingsteve Dec 13 '19 at 9:58
  • I still think it's possible to overdo that, making every request/suggestion/order into a hippy placating question, but there we go :P – Lightness Races with Monica Dec 13 '19 at 11:34
  • So dont overdo it. Adjust as you see fit ;) – vikingsteve Dec 13 '19 at 13:52
  • Yepper, indeed :) – Lightness Races with Monica Dec 13 '19 at 14:15
1

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.

1

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.

  • Being exacting is ultimately far more efficient than ongoing variation. Many "nits" are only "nits" because there hasn't been a firm ruling. For example, line length isn't a "nit" - you set a rule and you enforce it automatically and no one ever wastes a thought or social capital unit on it again. And the reason you do that is not only to avoid opinion-driven format oscillation, but so that you can read a side-by-side diff on a laptop. Commit messages need a line length limit, too, as many tools conceal the overflow when viewing in an overview mode. – Chris Stratton Dec 12 '19 at 1:29
  • @ChrisStratton Well, on the line length note, sometimes it's not just about the specific length. Like, when writing LINQ queries, sometimes it's fine to have them be all in one line and sometimes you want each new segment to be on their own line. Having this be the case for all instances though may be overbearing and not help readability.This is also true with method length (num lines). For instance, some places have a rule that each method should be able to fit on one "screen". Saying a method is "too long" can similarly be vaguely defined. Opinion still has a place in code design/review. – Nate Diamond Dec 12 '19 at 21:28
  • In terms of line break positions, it often turns out that consistent is more important than the best. For example, I strongly disagree with a lot of how "black" formats python code, but I use it anyway, because it yields cleaner diffs while avoiding wasting thought cycles on what is ultimately trivial. Your method length example is a better example of things that are less trivial - fortunately when the trivial things are done automatically by the arbitrary book, then you're left with only the more complicated ones to fret over or debate :-) – Chris Stratton Dec 12 '19 at 21:37
1

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.

1

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.

1

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.

0

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.

  • It's entirely true that you can fix the raw sloppiness and have beautiful looking code which is still horrible as a way of accomplishing anything. But the examples in the question are mostly sloppiness - the concepts which are hard to teach are more about good vs. bad architectures and when to change things vs when to leave them in long incumbent form. – Chris Stratton Dec 11 '19 at 22:40
  • 1
    Code review and management issues have nothing to do with one another. Politics should not affect the integrity of your technical verifications. Period. – Lightness Races with Monica Dec 11 '19 at 22:54
  • @ChrisStratton: I assume that OP used simple examples to keep the question reasonably short, but that in practice the same individuals when working on more complex stuff would make more complex errors. – Dragan Juric Dec 12 '19 at 16:03
  • 1
    @LightnessRaceswithMonica: I agree that politics should not affect it, but try working in a company that's big on "diversity" for some time and then tell me that it doesn't affect it in real life... – Dragan Juric Dec 12 '19 at 16:04
  • 1
    @DraganJuric Oh definitely: "should" not "does" ;) – Lightness Races with Monica Dec 12 '19 at 16:16

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