52

I'm technical lead and we have a recent hire that's very inexperienced. He's also very opinionated and proud for a newbie and his code style diverges too much from the team. But he still produces low-quality code compared to the other employees.

This is not a problem, though: I'm supposed to catch those issues and teach him to improve in code reviews, feedback sessions, etc. Problem is: when I review his code, I have to leave too many comments to the point feel like I'm overdoing it. A few times I did let some issues slide, but this always ends up costing another developer's time (or mine).

I also tried one-on-one feedback sessions to avoid public reviews, but it was fruitless, as the developer was trying to justify every piece of feedback to the point of derailing the session.

What's the best way to deal with this? I'm getting good feedback from the team regarding the reviews, and I am preventing some production issues, but I feel like the "bad cop" every time I walk into his pull requests.

  • 7
    How many devs are in the code reviews? I always start a code review by reciting the ground rules, e.g. to treat the critique in an abstract manner; make it impersonal. When other devs undergo the code evaluation, make sure that the new dev critiques the less experienced people too. This will help the new dev converge into the expectations a bit easier than endless 1-on-1s. – jdb1a1 Jul 14 at 19:11
  • 32
    Have you considered automated tools to help? If issues are well-defined, there are lots of tools out there that can automatically point out the problems. The developer might get conflicting views about ones you let slide or miss, "but it wasn't a problem last time - why are you saying it is now". It's hard to know the exact issues but at least this way they can get instant feedback and potentially fix a lot of problems before you see it. – Eliott Robson Jul 14 at 20:31
  • 24
    Especially regarding "code style", having automated checks such as Checkstyle is a good idea even with experienced developers. – chrylis -cautiouslyoptimistic- Jul 15 at 1:44
  • 3
    For the issues raised in the code review, are they objective corrections that are clearly outlined or something subjective that is a personal style? If it is objective, does this developer have access to proper documentation of what is expected of him? A couple sample, some less severe, some of the most severe, and we can better gauge what is being reviewed. – Nelson Jul 15 at 2:13
  • 4
    +1 for automated code analysis. We took a lot of heat out of the code review process by delegating the first run to Sonar. Fix the auto-generated flags then we'll look at it. – Trent Bartlem Jul 15 at 6:30

13 Answers 13

55

On being a "bad cop"

As was mentioned before, the way to go is detaching yourself or any person for that matter from the issues to be raised. This means:

  • Your rules need to be clear and written down, be it in a wiki, a styleguide, company documents, whatever you are using. This material must be accessible to the dev in question.
  • When pointing out mistakes in a review, do not use phrases that involve you in any way. Instead shift blame to your documents, such as the styleguide and to your processes in general. An example of this can be, "Line X: According to the styleguide [link] static member variables have to follow the Y pattern."

You will not be able to avoid the bad cop feeling entirely, this is part of reviews. However with careful tone you can establish a review culture, where it is clear that not a developer is questioned, but only the code itself. It needs to be understood by all parties, that a review is not about criticizing a person or their work, but merely about impoving code and therefore your product.

Assigning proper tasks

This is probably my most important point and the one I think justifies my answer in the first place, as there are redundancies accross all answers posted:

Another answer by @Ertai87 mentions that correcting all minor mistakes is exhausting, I assume both for the reviewer as well as the reviewee. You also mention there is so much to correct, that the whole exercise somewhat derails. The answer I am referring to then states to focus on the major issues and ignore minor problems.

In my view this is not the correct approach.

When the tasks solved by the developer in question are so laiden with issues that reviewing them turns into an enormous undertaking, then I want to argue these tasks are too large for the developer in question. They are not ready and need to be assigned smaller tasks and get down the minor stuff first. That means, assigning e.g. bugfixes that only come with presumably only a few lines of code, only very minor features and other issues of the sort. Otherwise you will pass a ton of nonsense into your codebase because you are so busy with fixing their major mistakes, that you cannot afford to fix all the minor nonsense. Ultimately this will likely be time spent by other employees, who end up fixing all these things when they in turn work on the same code passages.

You should not expect your junior to be at the same level as everyone else, as the process of improving must be incremental. Still they are an employee, so you can expect that they bring value to the company, even if that value is relatively minor and only comes with and increases over time. So assign them smaller tasks and let them get the basics down first. The better they get, the larger their area of responsibility may become and so their tasks can increase in significance too.

Ask yourself this. With the time spent fixing that developer's code, how much time in comparison would you have spent doing it yourself?

Distributing reviews

As a team lead it is not written in stone that you have to review all code. Reviews can be done by all experienced employees, you have the option to use this tactic. A common way of doing this is to have a set of reviewers and a designated timeslot, e.g. once a week, when reviews are being processed. During that time all members of the set are required to review issues that are awaiting acceptance/rejection.

There are three main advantages to this:

  • Code reviewing is a task that requires a lot of concentration. You can do only so much of it on your own during a day before you start passing mistakes into production. More people on this task means more concentration as a resource.
  • No matter how experienced you are, there are likely some patterns in your code and some mistakes you repeat and are unaware of. This is true for your peers as well. When multiple people review members of your team and each other, at the very least the reviewee gets to see other patterns and other ways of solving problem X. This way knowledge is distributed in your team.
  • The more people do reviews, the less a single person is running the risk of becoming the bad cop.

I will say though, this may depend on the company and the processes in place. Some workplaces may require a team lead to sign off on each and any piece of code and some workplaces may even do so due to a specific qualification that only an expert brings to the table. An example of this could be safety in a medical setting. If there are no such special requirements, but the processes currently require you to personally review all code that goes to production, then this can be raised with management arguing for increased efficiency of the team. Only you will know how things work at your company, use your best judgement whether distributing reviews can be achieved at your workplace.


A personal note: When we started code reviews at our company it was bumpy at first too, because it is hard not to feel criticized when your merge request is rejected with a bunch of stuff to fix. By now the team cherishes code reviews. Personally I have learned a lot from getting my code reviewed and so did my peers.

On defensive behavior

There are some things that can be discussed and some things that do not require a debate. Discussing this or that architecture is not uncommon. When doing so it is important to have a good reason for you want to change implementation X to implementation Y. Just saying "this is better" is insufficient. Of course you can go the authoritative way, but this is likely to demoralize and can show a lack of insight. On the other hand, when your team developed your styleguide I would expect you to have put some thought into why you decided you wanted to do thing X in way Y. These things should not end up in endless debates every single time, at least if the team's concensus on the matter has not changed.

All in all defensive behavior is not that quick or simple a problem to solve in my experience. I suggest doing one-on-one talks from time to time. Akin to performance reviews, but intended to be a non-interrogative talk between two team members, rather than a boss giving their subordinate the business. This is a time where you can share your gripes with how the employee performs by suggesting improvements. It is important to listen to their side as well. Are they content in what they are doing? If not, what are the issues on their mind? How can these be resovled?

That being said - if all such attempts do not bear fruit, then the authoritative way may be all that remains. In this case, explain to the developer that their performance is not satisfactory, as hard as it seems. This is basically a warning shot and at this point I would consider letting that person go.

I understand this may sound harsh, but ultimately every employee needs to bring value to the table eventually. The value of a junior in the beginning may be barely above zero, it may even be an investment into future productivity, without any immediate gain. However if time passes and no improvement is seen, then the company is wasting money and the employee is not the right fit for you.

There are a lot of things to try before this happens though, some mentioned above. You should ask yourself, if you can improve your communication with that employee and go from there. Are you phrasing things that force them into a defensive stance? If the developer turns out to be an asset to the company that was only hindered by poor communication between them and you, then everyone wins once this is recognized and resolved.


Another personal note: I have been working with and teaching quite a view juniors by now in my last couple of companies - mostly students in their bachelor's and master's, doing the first steps coding for real world applications, but also self-taught coders as well as juniors with a different educational background. One thing many students learn after taking this step, is that technical skills, no matter how good you are, are one part of a larger equation. Soft skills are largerly important and need to be caught up on if necessary.

Nowadays we filter candidates by assessing their character rather than their technical skill. They have similar education and we rely on this fact. Personality compatibility however is highly important, because one bad apple can poison the whole basket. So far, primarily by promoting a very welcoming company culture, we have been able to integrate all of our students and every single one of the became an asset eventually, but we take our time with them and don't assign someone who is learning the ropes giant tasks. As said - progress is incremental.

I hope this wall of text helps you in one way or the other. Good luck!

| improve this answer | |
  • 2
    Excellent answer! I would also recommend this book, Code Complete 2, to help with making checklists and style guides. amazon.com/Code-Complete-Practical-Handbook-Construction/dp/… – Stephan Branczyk Jul 15 at 0:25
  • 1
    +1 well thought out and points out how the team as whole is important, skill can be improved easily, personality is much harder. – morbo Jul 15 at 11:15
  • 3
    On the subject of workplaces where a lead is required to review: It's usually still feasible to have someone else on the team review it first, for general compliance and quality, and then once it's up to an acceptable standard have the lead review for whatever their specific expertise is. In some cases, that might still involve reviewing everything in detail, but it would at least reduce the workload on the lead. – Bobson Jul 15 at 13:21
  • 3
    Having dealt with this issue myself on multiple occasions, I especially support the idea of properly assigning tasks. Someone who doesn't yet understand how the team organizes code, local standards, formatting conventions, etc... really shouldn't be writing (for instances) entire models and classes. Or they should be doing so with the understanding that they need to be mimicking the patterns and habits shown elsewhere, because when it comes to code, the most important thing is consistency. – Conor Mancone Jul 15 at 13:37
  • 2
    A style guide need no justification beyond, "We made an arbitrary choice and are sticking with it to maintain a consistent code base." Of course, having a proper justification is preferable, but a substantial part of the motivation for having a style guide is to make the code consistent. – Brian Jul 15 at 14:18
48

If there are that many mistakes in the code, maybe a code review is too late to catch them. Maybe you need to take a step back. There are some alternative approaches you could take:

  1. Training. Doesn't have to be a course. Could be a book, a video series, an exercise site.

  2. Personalized guidance. Instead of repeatedly pointing out the same mistakes in code reviews, maybe take him aside and explain the most common ones in more detail.

  3. Pair programming. Let him shadow a few of the other devs. It's the quickest way to pick up the in-house code style.

  4. Mentoring. Officially assign another dev as a mentor to help out with code reviews. Ideally, this should be something both parties agree to.

| improve this answer | |
  • 4
    1. requires the individual to want to improve, which doesn't seem to be the case here. 2. seems to have been done excessively. 3. May be an option but expensive. 4. again - needs cooperation. And you forgot 5. : Let him go. – Fildor Jul 15 at 7:47
  • 23
    @Fildor The idea that pair programming is somehow expensive is a huge misconception. Making software is not a production line where everyone needs to be busy and maximally utilized. It is a creative design work. And best designs come from collaboration and communication. – Euphoric Jul 15 at 8:22
  • @Euphoric I know what you mean. I've done pp before. I sometimes do it occasionally. It is expensive. That has nothing to do with if those expenses are justified or create a ROI later on. Also, in this instance, pp wouldn't have the benefits nor even the intent you describe. It would be merely babysitting. Expensive babysitting. – Fildor Jul 15 at 8:26
  • 10
    @Fildor, There's already expensive babysitting happening: it's just happening during code review when the dev thinks they are "done" and are defensive of their decisions. In my experience, the earlier in the development process feedback is given, the more likely it is to be taken well, so switching to pair programming for a while could potentially yield improved outcomes without using up much more time. – Parker Coates Jul 15 at 12:40
  • @ParkerCoates Didn't argue against that. May be worth trying. – Fildor Jul 15 at 15:14
33

The code reviewer is supposed to be the "bad cop". That's your job. If you feel like a "bad cop", that's a good thing and you should embrace it. That said...

  1. Junior developers make a lot of mistakes. Pointing them all out is exhausting, frustrating, and demoralizing. If they e.g. name a variable wrong, or they use a linear search instead of a binary search for a sufficiently small dataset, or didn't write a unit test for a piece of code that you believe works properly, that's probably not worth discussing. Save your energy for serious issues, at least on the first pass...

  2. Do multiple passes. In the first pass, look at only the most critical issues. Then let the developer fix those, and move onto the next most serious issues. You may want to do 3-4 passes on a PR to get all the issues ironed out. Too many issues is demoralizing and confusing to read.

  3. Point out when the developer does something cool that you like. You can be encouraging in your code review as well if you throw in a comment like "oh, that's a cool way to do that good job!" once in a while.

  4. Reconsider if maybe your coding practices are too strict. If you have something like "every int variable has to end with the word Int", maybe that's a dumb restriction that you should consider relaxing. How many of your rules are industry-standard, and how many are esoteric?

  5. If all else fails, sometimes you have to put your foot down. You are the code reviewer. The code doesn't get merged without your say-so. You're also the senior on the team, he's the junior. There does come a point at which you simply say "I'm better than you, do what I say". Try not to pull the seniority card too often or it will get toxic and demoralizing, but you can pull it once in a while when you feel you need to. If you're going to pull the seniority card, make sure you are 100% sure you are absolutely right. If you pull the seniority card and you turn out to be wrong, you're going to lose a lot of face, both with this developer and also your boss and team. Nobody likes the guy who whines and complains and then when he gets his way it causes production to crash.

| improve this answer | |
  • 2
    For that matter, misnaming a variable could be even worse if it misleads a maintainer about what's happening in the system. – chrylis -cautiouslyoptimistic- Jul 15 at 1:45
  • 3
    @Kevin But using a linear search for a sufficiently small dataset is not a problem... It's easier to implement, easier to verify, easier to understand (for reviewer and future readers), and might not need to be tested. You've traded some marginal efficiency which might not even matter for a ton of time saved. $$$. Economics. Besides, it might possibly be a faster solution for some n < N. These are things that an experienced code reviewer should think about before spending $$$ and pressing that "reject" button. – Mateen Ulhaq Jul 15 at 5:07
  • 11
    One thing: I would recommend taking a step back on the 'do multiple passes', and first find out if the junior has a problem with many remarks at once making them feel overwhelmed, or if the multiple passes would give junior the feeling that 'it's never good enough' or annoyed at having to go back and forth so much (I definitely fall in the latter category, I'd be massively annoyed if I found out someone already knew something was wrong yet saved it for after 3 back-and-forths while I could've fixed it the first time too). It's a good strategy for the first sort of people, not for the latter. – Tinkeringbell Jul 15 at 8:54
25

The answer is kinda mean, but... everything's lining up on the "go all out on enforcement" boat, as much as I hate to look at it that way.

I mean, you've said:

  • He "produces low-quality code" (even apart from style differences)
  • The things you've already let slide have cost your other developers unneeded time.
  • He's "very inexperienced".
  • He's opinionated and not receptive to changing.

The reason I point these things out is... what if you suddenly just said, "You know what? This guy doesn't get to move any of their code to production until the code completely conforms to our standards."

It's not like the developer is churning out loads of amazingly productive code and that your standards would be seen as niggling and holding back the company's bottom line. It's not like the developer is receptive to non-forced change, and that this issue goes away after another several months. It's not like the developer is putting out code that doesn't cost your other developer's unnecessary maintenance time due to standards deviations.

... and as sad as it is? It's not like the developer is an extremely valuable asset to the company. That's just what happens when you combine "Inexperienced Junior" with "Unwilling to Learn or Change".

Because of all this, your best bet is probably just draw a line and say, "You don't get to promote code unless it completely conforms to the standards. Period. You'll need to either start following standards when you compose your code, or start budgeting time to rewrite it before you can get it put into production." And don't let anything slip.

The dev's likely going to hate that. Maybe they'll end up improving and writing quality code. Maybe they won't. But... that's the saddest part of it. An inexperienced junior that refuses to learn or change deciding to leave your group isn't all that terrible of an outcome.

EDIT: Oh, something else I forgot to add: they're a "very inexperienced" junior. While I'm definitely not going to say, "Never listen to the junior because they won't have anything to contribute", there's nothing wrong with saying, "Listen, I know a lot more about this, and I'm telling you: this is the way our group operates, and this is what the standard is. You need to change your code to match," and then moving on to the next issue on the code review.

| improve this answer | |
7

How many of these style rules are actually written down?

My organization (sometimes) does code review, but one of the issues is that we do not follow any meaningful rules regarding the authorship of code. You can get entirely different (and frequently contradictory) feedback depending on who does the reviewing. It also does not help that most of the code was written before anyone on the current team arrived, meaning that none of it aligns either and using past code as an example did not work.

For code review about style/organization to work, clear rules need to exist. It is incredibly frustrating to try and adhere to rules which are quasi "just known" within the team. Imagine trying to replicate a painting while viewing it through fog.

In the case of your junior developer, you could just tell him that the code must "adhere to the style guide" and send it back to him instead of making a barrage of repeated comments.

You should not stop the code reviews, but you should also be sure that the new developer is in a reasonable position to know what the rules are.

| improve this answer | |
  • 1
    This is fine if the code review issues are "use camel case" or "indent using tabs instead of spaces". But if the developer has lots of bugs or code that doesn't include error checking, that can't be covered by a style guide. – DaveG Jul 14 at 20:39
  • 1
    @DaveG bugs wouldn't be, but unit tests to check for the bugs would. Error handling rules would also be in such a guide. Code style guides are probably more accurately called development guidelines. – Matthew Gaiser Jul 14 at 20:57
  • 1
    The next step would be to use automated tools to analyse that code follows the standards (e.g. a linter). Being able to run something that produces a list of issues in some code is a lot easier than finding those issues yourself based on what a 10-page document says. Some such tools can also identify some bugs. – Bernhard Barker Jul 15 at 4:54
  • 1
    "Write everything down" is why legal systems are so complicated and prone to loop holes. (Should I write down "variable names must be in English"? How about comments? Oops, now we have a loophole where function names can still be in Klingon, or someone has named a variable very_long_variable_name_definitely_in_English.) That's why we have common law, also known as previously written code. If no one on your team follows previously written code and all give contradictory feedback, you don't have any law--you just have anarchy, and that's a different problem. – user3067860 Jul 15 at 18:54
  • 1
    @user3067860 legal systems have to handle people actively trying to break them. Assming a good faith effort on the part of developers, development guidelines need not be that long. – Matthew Gaiser Jul 15 at 19:08
3

I also tried one-on-one feedback sessions to avoid public reviews, but it was fruitless, as the developer was trying to justify every piece of feedback to the point of derailing the session.

It sounds like your working styles are incompatible: You want him to work in a particular way (openness to feedback, high-quality code, focus on maintainability, ...), and he wants to work differently (let's call it "lone cowboy coding"). That's frustrating for both sides.

To borrow from role-playing terminology: You need a session zero. Sit down, explain what is expected of him in the long term (openness to feedback, higher quality code, willingness to change, etc.), and determine whether this is something he even wants.

  • If he does... explain that you are here to help him become that future self that is a good fit for your company, and that a lot of learning and change will be required. He needs to commit to that goal and accept that code reviews are a tool to get him there. The more feedback he gets on code reviews, the more he can improve and reach that goal.

  • If he doesn't... well, it might be better for both sides to amicably part ways. Programmers are currently in high demand, so he should not have a problem finding a company where a less structured approach to software development is appreciated (there are lots of questions here on The Workplace.SE complaining about such companies).

| improve this answer | |
  • I think you need to have a bit of experience before your "coding style" can reasonably be considered when measured against an existing codebase. Even considering style here is really putting the cart before the horse, considering the question is about a junior dev who's basically refusing to learn. – kungphu Jul 20 at 9:04
  • @kungphu: I did not mean "coding style" in the sense of "where to put your braces", I mean the more general "software development style", let's say "lone cowboy coding" vs. a development process with a focus on code maintainability and a strong feedback culture. The junior seems to prefer the first one (whether out of inexperience or out of conviction does not matter, if he does not want to change), whereas the OP's company requires the latter. I agree that the phrase "coding style" is misleading here, I've changed that. – Heinzi Jul 22 at 7:30
  • OK, that makes sense. Though it seems like this situation is less about the new dev preferring anything in particular than it is about being fundamentally ignorant of a lot of things experienced developers (and anyone who's successfully worked in teams) would take for granted... to me, the main takeaway from this question was that they hired a kid but weren't prepared to raise one. I'd hope the company/department takes this opportunity to get that right with this hire if they intend to keep hiring junior devs, rather than firing him for something they really should've seen coming. – kungphu Jul 24 at 3:22
2

There are loads of good answers to this question, I'm going to add a few thoughts that I haven't seen in the other answers.

  • Does your coding standards deviate much from the standards of the language? If so it will be harder to get developers to follow it, especially new developers that have a hard time to just understand code.
  • If your coding standards doesn't deviate much from the language standards you can point to it being the language standards, and it will be the same for most companies.
  • Do you use tools to do as much of the review as possible automatically? Formatting templates in the editor solves a lot. Static code analysis helps with a lot more.
  • Code reviews are to improve the code now and in the future. You should make sure it's possible for the developer to learn. One way is to give credit when something is good. Another to let the developer review others code, that way it's possible to see good code. Note that I don't necessarily suggest that the junior developer should be the only reviewer.
  • Most people straight out of university/whatever don't know how much they don't know, and think they know everything. While this can be frustrating it's just the way it is, and it will be better the more they learn that they don't know. The attitude will improve at the same time.
  • I think you have to expect that some code isn't up to all of your standards, especially for a junior developer. Concentrate on getting the important parts up to standards, and add additional comments when appropriate. That way the developer won't feel like nothing is good enough and give up.
| improve this answer | |
  • OP Here: Our code standards are pretty much derived from what the big industry players use. The deviations are mostly in weird things that can't be caught by linters and other tools. I honestly see this as an attempt at malicious compliance. – mtw Jul 16 at 9:36
1

Put him on a Performance Improvement Plan.

It sounds like at the moment he is producing negative value for the company - he's being paid a salary to waste the time of other, more experienced developers. Obviously, this isn't a viable position for him to be in for the business, and something needs to change. As a result, it might be a good idea to formalize this with a Performance Improvement Plan that includes concrete milestones and goals for him to reach, so that he can either improve his performance to be net benefit for the business, or be fired with cause so that he's no longer a net detriment to its performance.

| improve this answer | |
  • 7
    This would be much more reasonable if he weren't a new, "very inexperienced" hire. Whoever hired him knew he was inexperienced. The question reads much more like the hiring process resulted in onboarding a very junior developer without providing the appropriate context/training/materials necessary for them to understand standards and expectations. A PIP can be discouraging and demoralizing even for someone experienced; for a new employee it could ruin any good will, enthusiasm, or rapport. – kungphu Jul 15 at 8:02
  • 3
    I have yet to see a single instance where a PIP provides any actual value to an employee. It is nothing more than a way to try to get out of paying unemployment when an employee is terminated. – Bardicer Jul 15 at 14:49
1

I'd say you give him a small task, and then you review the result, and let him re-work what he's done until you are happy with it. If he tries to argue, and he's wrong (that's important obviously), then you tell him that he should know what is wrong, and ask him why he thinks he has to defend the undefendable. If there are coding styles that everyone adheres to, tell him to adhere to them. Be careful there: I have some coding habits because they are better, some because consistency is important in some cases, and other coding habits that are just habits.

| improve this answer | |
1

In the Philokalia, it is said that such-and-such can help people with such and such deficiency, and such and such can help people with only such and such deficiency, "but only God can help the proud."

Pride is, besides being a sin, a weakness that puts an iron guard around other weaknesses (cf. Chesterton). Someone who is humble and inexperienced can make steady progress learning. Someone who looks down on you and exempts himself from every correction has a higher pay grade of a problem than just someone who is good, old-fashioned inexperienced.

You need a humble programmer. Put him on a performance improvement plan, as a last measure of mercy instead of just terminating him (which is justified).

| improve this answer | |
0

I haven't seen this option put out there anywhere... but if you do not have something like automatic linting/stylecop enforcement as part of your development process, this would be a great first step as it will catch a large portion of issues without anyone needing to feel like a "bad cop".

Put it in the code as part of a build - if any of the rules are violated, like maybe you expect a space with an if, i.e. if (...) instead of if(...) or if a variable shouldn't have an underscore in it and should be camelCase instead of PascalCase and that breaks the build if violated... then if they violate the rule and it blows up on them, they will know what they did wrong and what they need to do to fix it without having to impact anyone else's time.

With this in place, nobody's feelings or pride are being injured needlessly because their minor issues are being caught by the style enforcement library and not another person. This will also leave you to focus on the code smells and bigger issues.

When it comes time to lay actual eyeballs on their code, if something isn't right, call this out as well as an explanation as to WHY it is done incorrectly. Expect some push back, and that is ok if they can give a valid reason (performance, maintainability, etc.) why they did it a better way. Keep an open mind about it. If they start getting overly defensive and bristly, call that out as well but in a non-combative way such as "Hey, we're a team, we sink or swim together. I'm not trying to make you feel bad, I'm trying to help you avoid pitfalls that I fell into myself."

When someone has to be "bad cop", try to push that off on the emotionless code as much as possible, as it doesn't care whether someone likes it or not. When you have to assume that role, be a "good cop" giving "tough love" instead of an outright "bad cop".

| improve this answer | |
  • 1
    Exactly. And most code quality scanning tools (like Sonarqube, Codacy, LGTM, etc) will give you a numeric count of different types of problems -- style problems, code smells, vulnerabilities, etc. -- which acts as a "score". The dev can work on his own to improve his score, before submitting code for a review. – workerjoe Jul 15 at 16:01
-1

Problem is: when I review his code, I have to leave too many comments to the point feel like I'm overdoing it. A few times I did let some issues slide, but this always ends up costing another developer's time (or mine).

It means that the first problem to fix is you and the company. How? Why?

If the company is at least minimally professional, there should be formal written coding guidelines, design guidelines etc. In these guidelines, everything should be ranked: mandatory, almost mandatory (can be skipped e.g. only if there are very good reasons and the entire team agrees), optional. These guidelines must be mandatory and followed by everyone.

If the above is implemented and works, the new guy can just review his work according to the rules, before you review. A lot of the stuff would get fixed "by itself".

Since we talk about software, using a static analysis tool (agreed and configured by the company) should be mandatory. That tool will catch a good number of things (hopefully), and the new guy can do a lot of work independently before talking with anyone else.


Reviews - with a twist

Not only that his colleagues should review his work, but he should review the work of his colleagues too. He will learn how reviews really work, and he will learn why some things are important.

He will remember his criticism he provided for someone else's work when his work will be criticized.


  • If I were the new guy, and
  • if there would be no formal guidelines,

then I would have a much stronger stance against your reviews:

Why is your opinion better than my opinion?

or even better:

Why? Where is it written?

Update: the guy is already doing it!! (I missed this info when I read the question initially)

I also tried one-on-one feedback sessions to avoid public reviews, but it was fruitless, as the developer was trying to justify every piece of feedback to the point of derailing the session.

Lacking a reference point / system, any opinion is as good as any other opinion.

| improve this answer | |
  • 1
    @Michael: Thank you for the comment. You are right, I will update the answer. I had in mind more like "it will catch everything it can catch". – virolino Jul 15 at 10:38
-2

his code style diverges too much from the team

It's unclear to me whether you think style is only/major problem with his code, or whether it's also poor in other ways, but this one is easily solvable.

The solution is to make it impossible to check in code which deviates from your expectations. You don't have to be the bad guy. Let the computer be the bad guy.

The first step is to codify your expectations. Checkstyle (a Java tool) uses an XML style format. It's extensible for custom checks too. I've found it quite good. You should also have a human-readable version, preferably with examples.

Next you want to make sure your build will fail if a developer deviates from the expected style (you do have an automated build, don't you?). We use the Maven Checkstyle plugin for our projects.

It is easier to fix problems locally than wait until the automated build fails, so I find it advisable to set up pre-commit hook to execute the same build plugin, so that it's not possible (without deliberately circumventing the hook) to push something which deviates from the expected style.

The last step is to ensure that your developers' lives aren't consumed with trying to adhere to your style; you need to distribute resources to help them do it. This step is important to make sure they don't hate you for the previous 3 steps. Set up your IDE so that it matches your style guide and export the settings, so that your team can import them. Put the settings file in a repo that everyone has access to. If different members of the team prefer different IDEs, consider EditorConfig which does this in an IDE-agnostic way. Once you've done this, adhering to the style should just be as easy as hitting a keyboard shortcut.

Also look into IDE plugins which match the tool you're using as part of the build. We use a checkstyle plugin for IntelliJ which can load a custom style XML file and will show violations in-line, just like a syntax error.

For as many violations as possible, automate the fixes. I find auto-formatting the entire code to often give some bizarre results. For some cases, like unused import statements, a computer can easily fix those itself with low risk.

| improve this answer | |

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .