The Workplace Asked by user119561 on October 21, 2021
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.
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".
Answered by Bardicer on October 21, 2021
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).
Answered by Christos Hayward on October 21, 2021
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.
Answered by Polygorial on October 21, 2021
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.
Answered by Michael on October 21, 2021
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.
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.
Answered by virolino on October 21, 2021
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).
Answered by Heinzi on October 21, 2021
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.
Answered by gnasher729 on October 21, 2021
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.
Answered by nick012000 on October 21, 2021
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:
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.
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?
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:
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.
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!
Answered by Koenigsberg on October 21, 2021
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.
Answered by Matthew Gaiser on October 21, 2021
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:
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.
Answered by Kevin on October 21, 2021
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:
Training. Doesn't have to be a course. Could be a book, a video series, an exercise site.
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.
Pair programming. Let him shadow a few of the other devs. It's the quickest way to pick up the in-house code style.
Mentoring. Officially assign another dev as a mentor to help out with code reviews. Ideally, this should be something both parties agree to.
Answered by Llewellyn on October 21, 2021
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...
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...
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.
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.
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?
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.
Answered by Ertai87 on October 21, 2021
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP