Ask HN: How to avoid giving a scathing code review?
For the past several hours I've been struggling to unravel a pull request from a newly hired coworker. It was apparent from a quick glance that it needed some work but I underestimated how bad it was. The main logic is in a monolithic ~400LOC function (in a dynamic language, not verbose Java or C) consisting of a deeply nested if/else spaghetti code. I wouldn't dare touching it with a 10 foot pole without tests but fortunately there are some and the original test coverage isn't bad. So I've cut it down to almost half the size so far but there's still way to go until it reaches a somewhat maintainable state. I've also found and fixed a few bugs along the way so strict refactoring is not even a goal at this point.
So I'm wondering how to go about this. A few code review comments here and there won't cut it. If I was to be brutally honest, I'd outright reject the PR and tell him to rewrite it from scratch in a clean way but that's probably not the ideal response. How do you deal with situations like this?
69 comments
[ 4.9 ms ] story [ 137 ms ] threadIt can be difficult. Somes some people aren't meant to be developers, but ultimately you need to foster a good culture of software craftsmanship.
If you have the same manager, I'd see about talking to them first about how to approach it.
If you don't, you are in trouble.
Hopefully, this person wants to learn how to write better code.
Now, the opposite is also true, it sucks to be on the other end, and sometimes the person reviewing is on a high-horse and possibly wrong as well. A 400 line function does seem wrong, but just because you're not using someone's favorite Ruby function or not writing all 2-line methods doesn't mean that is wrong either.
Code is intensely personal - engineering rigor is required,b but personal style and creativity are vital to enjoying the job as well. This can be very hard in group contexts.
Perhaps it would be possible, if you're on good terms with this person, to set up a pair refactoring session where you start to unwind it?
I think it's always easier to do things while things are being designed, rather than to do something at review stage, where it more feels like things are being judged.
I always like to encourage folks to talk about code as it's being written when I can.
I state this because I have done the opposite and it can backfire in ways you don't initially think. So I have learned it is always best to start of with an inquisitive mind on why he/she wrote it this way and what were their instructions and then use it as a teaching moment to help them be better.
I have seen companies where people got rewarded for the most convoluted stupid ass code you can imagine, generally in larger enterprises. As an example, I all but stopped hiring financial services programmers in the late 90's early 2000's in my area because we found we had to get them out of so many bad habits. Not saying all were bad, but our screening of someone anytime I saw financial services (especially big banks) on their resume as a developer was quite a bit tougher. Its not quite the same today, but I still know teams that I wouldn't hire from without really screening someone heavily.
Also, I guess one other point. If this person was hired at a senior level vs a junior or mid level, I would still approach it the same but be more critical (and direct) why they would write code that increases the likelihood of defects.
I'd definitely give it a try, at least for onboarding new people. There's no better way to transmit knowledge about the code and expectations for how to work with it.
Until then, you need to be friendly and patient, for a number of reasons. Don't betray shock, and don't betray disbelief in their incompetence, but do firmly require them to sit through a lengthy code refactoring with you or someone you trust, so that they can see what is required AND how far their code is from what is required. They need to feel your pain, but not by you being testy or edgy or frustrated.
Or, as the good writers say: show them, don't tell them.
I am assuming 2 to 4 years training or education prior to this. If you haven't learned to subdivide long / deeply nested routines into smaller functions with defined inputs and outputs after 2 years, you probably just don't get it, and may never.
That seems harsh, I suppose. But I've been doing this for a living for most of the last 30 years. When I started, I was 2 years into my CS degree. I've met too many code monkeys who might know some new API, but just congenitally cannot organize code to save their lives and write stuff I would not even have written as a 20 year old part timer going to school.
Spend a few weeks to see if the guy can learn. If not, start documenting a "performance improvement plan" (there's a euphemism) so you can get rid of him.
Everyone is different, and I certainly don't know all of the right words to say, so I'll simply say what I think should be mentioned no matter what: point out the problems with the code, not the person. "What if we broke this apart in two right here? It looks like it'll be hard to test this in isolation otherwise." "What are your thoughts on some sort of pattern matching / switch statement here instead of using if/else blocks? Any downsides? Might end up being a little cleaner."
You're trying to make the case the code can be better, but not that it's horribly bad. I mean, if it works, but it's ugly, you have a base. They got the job done. Now it's time for you to help them make it better.
- Don't phrase your feedback in such a way that you wouldn't feel comfortable saying to this person's face, or have read out in court. In other words, be professional.
- Mark each piece of feedback as "Blocker/Not-a-Blocker", to cover issues which (in your opinion), need addressed before using or are simply opinion points which are at the author's discretion on whether to use.
- If you know of a better way to write it, include a snippet or some pseudocode if possible. If it's more complex than a snippet and it's a work situation, then write your point succinctly and then approach in person and offer your assistance in pairing if they would like.
Ask your boss / their boss if they also expect those standards or if you are wasting your time or if this was a good use of your time to spend hours/minutes rewriting this employees work. Ask if that employee should be spending 2x/3x/4x the amount of time on their code to get to that level.
edit: What I mean is, it doesn't sound scathing if it's helpful. Scathing does not have to be in the vocab. Just be helpful to everyone, coworkers and superiors. Everyone wants to do the right thing.
If you insist on refactoring and fixing all the PRs you end up being the bottle-neck, and your co-workers will not learn as much from the experience as they could.
I'd still argue that's probably a good bottleneck to have though, and certainly better than letting the code quality slip. It depends on the timelines and priorities, but I'd rather work somewhere that invests time up front to save time later.
I strongly disagree that the co-workers would not learn from said refactoring. That's very pessimistic on either the coworkers reading comprehension, or pessimistic on their attitude or willingness to engage with the changes without being forced to do so? I don't know I find it amazing when someone offers a refactor suggestion, whatever amount of "doing it" they do for me, I find I learn a lot from learning what the changes actually are. If they do it themselves I get a really good feel for their style and what I can learn from it.
edit: And on the flip side, I find it's sometimes faster to just make a small refactor change than to explain it in person --- but this depends on the physical locality of teammates and schedule conflicts. In other cases it's easier to swivel the chair around and poke them on the shoulder and explain the refactor than to make it. Which is easier depends on priorities, chain of command, schedules and all that.
This seems like a great teaching moment on how to break up 400 LOC into smaller, more understandable modules. I would encourage you to block out some time with the dev to pair program and refactor it. If you refactor it yourself and then show them, they'll never learn how to do a better job in the future.
No need to be that pessimistic is all I mean.
Pairing would be better.
If the employee does not have time to pair, I would still do the refactor and ask the boss how much time they will be given to review the changes (I would assume/expect they would be given many hours to do that, if it took many hours to refactor) ....
Wholesale refactoring (especially by you, really, do not refactor someone else's pull requests, teach them what you want to see instead!) and/or wholesale rejection quickly degenerates into an impromptu performance review or even a question of suitability for the position. If you plan on keeping this person around I'd argue for assigning a more experienced 'buddy' to this person who will be on the receiving end of the reject (as the more senior person) after which they together will engage in a do-over according to your local practices with respect to all the things you feel are wrong and/or questionable.
This will take some time. Essentially this person should not have been in a position to submit this PR so the failure is a process and a management one, not one of that particular employee.
I'm assuming such this is some of that good old low-current class warfare you get in tech circles... all managers are stupid and evil, all engineers are pure and virtuous. that's the same sort of thing that leads to Aaron Swartz being lionized for breaking the law, because the prosecutor sought a ridiculous punishment.
a failure in process does not have the side effect of absolving individual responsibility.
If you feel like pushing the blame on the employee you can do that but then you risk it happening over and over again, in the end the employee owns the code he produced but the process that put him in that position is 100% a management issue.
The trick is not to either define this is good old low-current class warfare or as stupid or evil managers and virtuous engineers, that's a total strawman. You simply need to apportion your effort there where you can fix what went wrong and to avoid a repetition. Aaron Swartz need not be brought in to it, that has nothing to do with any of this.
I'll spare the details, however a long time ago a junior developer asked me to do a code review which happened to be on a critical part of the system which handles money -- not only was the code bad, it was incorrect as well.
I decided to skip some meetings and I spend a few hours with him. We did a pair programming session, cleaned up the code and we discussed the merits of each approach while we were doing it.
He didn't say anything at the time, however I found a blog post from him a few years later where he described the experience as invaluable and a driver to help him to try and improve his craft.
You need to patiently explain not only what is wrong, but also why it's wrong, and how to fix it, with examples. You might want to even sit down and refactor the code with him.
You need to explain the code design concepts and principles that he has missed. In short, you need to GUIDE him.
It's either that or get rid of him, because until SOMEONE teaches him, he will continue doing what he's doing and remain oblivious.
The best way for a developer to get better is to be shown a better way to do things.
I teach (not CS), but the OP should break down a small amount of code with a small number of actionable principles. He can't teach everything at once, simultaneously, without overwhelming the person he's teaching. If he has the time, he should do a small-ish amount of teaching every day, perhaps starting with the same code base and giving appropriate assignments each day.
The way I typically deal with this situation (and it's one I encounter almost as a matter of course with new people, and they all grow beyond this) is to have a high-level, in-person conversation about the code. Many professional programmers are actually quite good (whatever you think of this particular pull request) and the conversation is a chance to simultaneously de-escalate (whereas a nit-picky code review can escalate) and give them a chance to see their code anew from your perspective.
Good luck!
it's more important that he learns how to find better ways to do things himself. I would ask him if he thinks it's actually check-in worthy. I would continue asking until his doubt about it got the better of him, so that he views his code with a more skeptical eye. Then I would ask him to come up with 3-5 succinct ways to improve this. If they were good, I'd have him implement them.
You don't do the work for them, and you don't show them. The point of the college degree is that you've learned how to teach yourself without handholding. You need to be able to craft creativity yourself from blocks of wood.
If the 3-5 improvement ideas were bad, ask him to try again. If he still screws up [which is unlikely IMO] (and is really trying to come up with good stuff), give him one idea, ask him what he thinks of it, why, etc; have him implement it, give him another, etc.
my own experience writing C was basically that all the fancy terms in the book didn't help me any, because they translated poorly to tacit knowledge. what helped me was running into the problems myself, coming up with the solution myself, and then realizing someone had already done it.
I'm very slow at tacit acquisition though. I needed more practice. For someone like me in the first year and a half, I would just give them the info. However, now that I have 3 years of C and C#, I can rapidly digest new design paradigms, and write much better code. If the guy purported to have 5-10 years of experience and is still writing crap, I would take issue with telling him the problems himself as he's probably one of those rote-memorization dudes that China or India pumps out with no tacit understanding.
What I have learned is that having automated tools that enforce your code style and measure code quality (e.g. Code Climate, rubocop in the Ruby world, jshint etc.), integrated in your test suite or CI, goes a long way in improving the quality of your PRs (and, of course, code in general).
There's a cognitive difference between reading a code review written by a colleague and having a set of rules that are simply enforced automatically. This might matter a lot for new hires who have yet to learn that code reviews are about getting better and learning new things (and not about highlighting mistakes). Of course this wont replace code reviews, but it might remove some friction.
Telling someone they are stupid is the most effective way to have them not listen to you. I would approach the situation along the lines of this:
1) This is a really good first pass. Thanks! I like how you... 2) Lines x to y seem a little messy, do you think there is a more elegant way of approaching this? (Note: y - x < 20) 2b) Maybe highlight another area for improvement? 3) What happens if the feature requirements change? Can you show me how we would adapt to that? (Response: Oh, I see. Nice. Hmm...I wonder if we treated this in a more abstract way, would it require less maintenance?) 4) I'm really excited to have your enthusiasm (or other personal trait) as part of the team. Let's go get this!
Why I would use that approach: 1) Sandwich method is a classic strategy for giving negative feedback (positive then negative then positive). It minimizes defensive and emotional reactions. 2) Do not attack the person in any way. They will get defensive and angry and insulted. They will not hear "this is how I can improve" but rather "they are out to get me and not to be trusted". Humans need a sense of safety. 3) It's very possible that they are overwhelmed and scared at this new job. Insulting them will not build confidence, but rather destroy it. 4) It's also very possible that they've been trained to write crap as fast as possible, commit, and move on. If this company has a different style, they deserve to know that.
I would also want to know how did such poor architecture get implemented. It might be good to have design reviews, before coding starts, to discuss how everything should function. It sounds like the bad code is partially a result of improper communication with the employee.
Great question!
from personal experience, biggest reason
"Before I accepted this pull request, I would ask you to do a lot of refactoring. However, it's too complicated to explain in comments on the diff. Why don't we sit down and fix it together?"
Then explain your reasoning for the overall changes and each smaller change face to face in a pairing session (or series thereof).
I have to say that I find it odd that you spent any time at all re-writing this person's code. If I was your manager, that is not what I would want you to do.
Further, reviewing code over 400 LOC _total_ is generally a waste of time (never mind a single method larger than that.)
Or you could couch your feedback in mamby-pamby happy-land that said programmer still deserves a trophy for trying. I personally am fine with someone saying "this block of code sucks" if there's an unarguable set of reasons/metrics attached to that statement. Usually better if the reasons come before the conclusion. Avoid style preferences or bringing general identity accusations like "all your code sucks" into the review.
Keep the focus entirely on the product not the producer and that's fair game. Maybe tell the newly hired coworker a story about your code once used to suck, and how you overcame that.
wordy synonym for false dichotomy?
It's quite possible to be extremely critical while remaining respectful. Of course, you avoid criticizing the programmer: that goes without saying. But saying "it sucks" changes the tone of the discussion to a confrontation and that is rarely the best way to handle a corrective situation.
Point out the errors, introduce the dev to the quality standard expected by the team, offer help if needed. But treat people as peers, not as cretins that your manager forces you to work with.
That being said, I dont think I'd ever say, "This code sucks." I might say something like "I think there is significant room for improvement here if you were to <xyz>"
I have only barely begun to understand let alone implement the relationship side, so I'm sorry I don't have more examples of how to handle this in code review.
trying should always be rewarded. It's the only way you get better. Raising children, chapter 7 [nods]. You're confusing 'trophy for trying' with 'trophy for half-assed effort'. Like with any child, you need to know the person well enough to know how hard they're trying. If you go by the value of the work they produce, you're a cold heartless father.
If not, then that ought to be a priority, so that you can point people at that in the future, when rejecting pulls.
Unless there is some politik behind your question, I see no reason to say exactly what you feel. I have given (and received!) similar reviews many times. Btw I don't think is useful to do the rewrite yourself in the review: you are robbing the original author of a chance to redeem him/her self. I also think that refactoring a complex function is not a "rewrite from scratch", and even a rewrite from scratch on a second iteration is seldom as time consuming/complicated as the first iteration.
If you or your coworkers shy away from giving honest feedback in code reviews you have a big problem. Lowering the bar leads to technical debt grow, which someone will have to deal with later. You are missing a great opportunity to educate (and learn from) your peers.
If your managers pressure you or your coworkers to "go easy" on the quality (ie. "It works, let him check it in!") then again you have a big problem. You trade immediate achievement for later pain. It may be because is their immediate achievement but somebody else's pain later, and in such a case you're in a bad environment and not much you can do.
If you personally want to avoid the friction of a review that is asking for a major rewrite, you can sugar coat it, but ultimately giving negative review (when warranted!) is part of your daily job. As long as is objective and arguably leads to better code (and behavior) it should offend none.
Keep in mind that we all often write the code in iterations, and once is working we feel an urge to push it out without taking a break and looking over it again, but you the code reviewer see directly the end result. If is not pretty, is not necessarily because the original author is a bad programmer. It may simply require to take a break and have a look again, and he may see the same problem are obvious to you.
Giving feedback on a code review is really meant to improve small bits here and there. Incremental improvement. If someone doesn't know a new language construct, a better way of doing things, etc.
If someone simply writes crap, telling them that they wrote crap isn't helpful ... because they're clearly incompetent and are only capable of writing crap.
> If your managers pressure you or your coworkers to "go easy" on the quality (ie. "It works, let him check it in!") then again you have a big problem.
This is really the issue. First, you'll piss off the incompetent coder. Second, you'll stall the project. Third, you'll piss off your boss, since he's probably the guy who hired the incompetent coder.
All the while, you're "making waves". All based on what is essentially your opinion on what clean code should look like. Because it does run.
I would just take it slow and incrementally target some of the worst offenses. You'll never make the guy into a competent developer, but you can at least ameliorate some of the worst offenses and improve the quality of the code.
If you're actually working on the codebase as well, instead of just reviewing it ... I would talk to management. Explain the situation. They're either going to fire the new guy or you're going to need to find another job.
One of the reasons I like freelancing is that I don't have to face these dilemmas. The next client is just around the corner.
as long as nobody makes waves, we'll have a peaceful experience until the shitstorm of the reality of unmaintainable code hits.
this is one reason why companies need to die, and not be bailed out
I think you can't reach such a conclusion without knowing the circumstances. What if he's a fresh college grad or a junior developer? I think you're coming from a start-up environment where everyone around you is supposed to be an expert in their field. Most Big Cos hire fresh college grads and mentor them. So mistakes like this are common.
Now considering he's a junior, Remus's advice is spot on. I helped a dev on our team in a similar way who went on to write some really complicated distributed systems code that you use today. Also Remus comes from MSFT where this advice would apply.
Even when I was in college, I didn't write crap. Obviously my code wasn't great, but it wasn't crap.
And there is a lot of crap out there, I see it every day. Based on the OP's description, we're talking about someone who isn't capable of producing quality work.
You can either write code or you can't and it is very obvious by seeing the code.
If the person can write code, but needs a few pointers, that's what code reviews are for. If they can't write code, they need to move on to a different profession ... or move up to management.
Ehh - I've seen coders who can produce quality work, but sometimes choose not to because of (perceived) time pressure or (perceived) lack of value.
Their mental model for code generation is to generate crap, then identify the patterns, then refactor until it's not crap - where the last two steps are "optional".
Time pressure: A new hire likely feels some pressure to prove they can be productive.
Value: New grads may not be in the habit of maintaining code (having written lots of throwaway assignments), not so new programmers may be used to writing throwaway prototypes, or "code ownership" situations where readability by coworkers isn't valued.
Heck, it's quite possible they realize their current code is crap, and may even plan to clean it up in a subsequent changelist. I do similar on solo projects & private branches for the sake of keeping 1 changelist ~ 1 change parity, as it makes it easier to figure out refactoring mistakes. That said, those changes should generally be either done before the code review request, or noted as intended followup - and some pushback is warranted.
"Oh nice, a cleanly separated MakeSnafucated function! I'm working on document level snafucation, and just being able to call this will save me a lot of time! Thanks!"
If the coder has any potential, not only will they write more modular code like that in the future, but they'll do so happily rather than complain to management about your so-called perfectionist nitpicking.
Telling a coder he needs to break down a function and get rid of deeply nested function isn't calling it crap, it's giving him a way forward to improve his code. As a new hire, he deserves that feedback. If he follows it, compliment him and give positive feedback.
Whether a person can become a good coder isn't determined by how he writes code, it's how he reacts to the feedback he gets. If he can take correction, he can become a good coder.
Quoted for emphasis.
My first recursive postgresql function was crap. I recently learned that it became a DoS vector at 3 layers deep because I had horribly flawed logic. I spent an hour the other night refactoring it.
The cruelest feedback comes not from mortals, but from the harsh production environments which do not tell you what is wrong, but merely go offline.
Teach a man to code, he can write crappy code. Fix it for him, he still writes crappy code. Make him fix it himself, he learns (hopefully).
Also, as a code reviewer myself, this is really difficult, but the best way to fix this kind of thing in the future is to get involved in the code much earlier on. Watch the commits as they get made, stop by occasionally and go over the code while it's in progress. If the guy spends a week working on something and you send it back, he basically wasted a week. If you can spot a problem within a few hours, then you saved both of you a week...
1. Reject the PR. It needs to be clear that this isn't acceptable. 2. Realize that stopping with rejection doesn't help the problem at all. :) 3. Start with some light comments and refactoring a couple of sections (usually it's the same bad patterns repeated). 4. Only provide the refactors as comments. Try not to fix other people's PR and then submit. It makes it hard for that person to improve. 5. Talk with the individual about it. Ask about the issues they were having and offer fixes. 6. Ask them to try again.
If this cycle happens a few times, you either need to break down tasks into simpler, smaller bites, or let them go. This can quickly turn into negative productivity for the team.
Next, realize that if it works, the developer may consider it done. Many organizations don't see maintainability as an important goal and stop at functionality.
So, with those two things in mind, I'd suggest to the developer to keep functions small (do you have a coding standard?). Show him how the logic can be refactored (a surprising number of good developers never think about refactoring).
Next, consider that you may be imposing your personal preferences on him. Does your team as a whole generally follow the practices that you are telling him? I find that this can be a difficult topic in code review: of course I want everyone to do things the way I do them, but I have to stop short of trying to create little "mini-me's"
tl;dr: be nice, point out how the rest of the team codes, suggest a rewrite based on the concepts you bring up.
BTW from my experience teaching people to avoid nested control flow is one of the simplest things to do - just show them how many different code paths are possible (and how many tests would be required to cover each one) in a function with 4 nested if's.
A good way to communicate is to keep it informal and provide direct feedback, w/o involving anyone else, so you avoid making the developer feel inferior or feel that there is a political element involved. Ideally, you would share screen and code the suggested improvements together, and it will not only help you accomplish what you set out to do, it can also build a bond/trust/respect between you and your new colleague.
During the conversation, I would try to understand the following: Is there any legacy, currently working code, that has been implemented in this monolithic approach? Maybe the developer realized it was ok, for the initial iteration. Is the developer under a tight deadline? To meet the deadline she prefers solving problems in this way, for the initial iteration. In both situations above, the developer is aware of a better way, and your help to get it there in time will be appreciated. If not, some coaching is required (hopefully you have coding standards to refer to): Is she interested to learn a better way? What are her reservations and counter arguments?
For smaller teams, daily team code reviews are very helpful.
Have fun!