Ask HN: How to avoid giving a scathing code review? | Hacker News Reader

Ask HN: How to avoid giving a scathing code review?

66 pointsreinhardt67 comments

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?

View on HN

What's wrong with giving exactly that feedback? "The main logic is too monolithic and deeply nested. Please refactor in a more maintainable version, split into several simpler methods".

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.

> What's wrong with giving exactly that feedback?

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.

"Reprove with sharpness, then show afterwards an increase of love."

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.

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

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.

My favorite and severely underrated code review trick is positive reinforcement:

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

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

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.

> What if he's a fresh college grad or a junior developer?

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.

> Based on the OP's description, we're talking about someone who isn't capable of producing quality work.

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.

That's BS. Coding isn't some kind of magical incantation, in spite of the jokes in SICP. Anyone that can understand basic algebra can learn to code.

At some point, there needs to be some sense of "if you're not going to do it right, don't do it at all". Anybody could give a haircut, but do you want a bad haircut from someone who does not want to improve?

> you're making waves

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

If the code is that far off the deep end, then your job at this point needs to shift from code reviewer to teacher.

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.

This might be a good time to do a pair programming exercise. Go through the code with them so they can see your process. For example, review the requirements, write test cases to cover those reqs, then refactor the code to work or even start from scratch if it's not that much code.

The best way for a developer to get better is to be shown a better way to do things.

> teach him

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.

The techniques for writing well-structured code may seem obvious to someone who has been practicing them for a long time, but they may be not at all obvious to someone who is just starting out. After all, it took a few decades in the history of programming and some very smart people like Dijkstra to convince professional programmers that code structure was important. The principles of refactoring weren't well known until much later. So I think that expecting some junior programmer to come up with these ideas by trial and error is setting them up for failure.

this is a very good point.

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.

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.

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.

I mostly agree, especially with the emphasis on teaching.

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!

This sounds like a mentoring rather than a code review situation to me. Code review is a look at code, judging its general applicability (usually a pass, in this case apparently not), commenting on style and/or obvious bugs, test coverage, suitability for deployment and so on. You try to stay civil and constructive, guiding the submitter towards what you would have liked to receive in the first place, you gently reject the PR with a list of 'tofix' items.

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.

Totally agree.

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.

responsibility for the failure does belong to management, certainly, but taking away all responsibility from the employee for producing poor work is amazingly populist, but not particularly helpful to the reality that the person is unable to do the job assigned.

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.

The guy simply should not have been in a position to send this PR, that's not populist, it's reality. So you fix the process and you try to salvage what was produced in a way that has the least overhead and the maximum possibility for success.

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.

You could simply go ahead and give the scathing code review, if you wanted. I used to work with a few senior engineers like that who were extremely sharp and, shall we say, point-blank with their feedback. But their feedback was always about the product, not myself, and they often provided guidance outside of reviews. After some initial butthurt, it teaches the young engineer to learn to separate their identity from the product they've produced.

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.

Or you could avoid the fallacy of the excluded middle: list in a clear and objective way exactly what is wrong and (if applicable) points to fix it. "Sucks" is a subjective comment and not very useful feedback. I know my code sucks but not why it sucks - that is partly why it is being code reviewed.

> fallacy of the excluded middle

wordy synonym for false dichotomy?

No.

   X --- excluded middle --- Y

Versus

   excluded left bit --- X --- excluded middle --- Y --- excluded right bit

+1 Fantastic. Its hard to receive these reviews as a young engineer, and as a slightly more experienced engineer, it is also hard to give them. But its an important part of the job. Its fine, in my opinion, to give the hard review, as long as you walk over to your coworkers desk, and helpfully guide them through this as a learning experience. Having this stuff written down is also helpful for them to reason.

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>"

room for improvement is still pointing out the failure to hit the mark. Much better to stay focused on positives in code, like you focus on the positives in a relationship. 'I wish you would...' is so much different from 'stop doing XYZ, it annoys me'

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.

> trophy for trying

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.

This sounds like a list of excuses to be lazy and rude.

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.

This is why pair programming is so great, especially when the duo is somebody seasoned and a new hire. Pairing is continuous code review so you address problems like 400LOC methods before they start. It also reduces the newbie's sensitivity/fear/hostility to feedback when the code is your shared work product. "How could we make this better?" feels a million times better than "You should have done it this other better way".

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.

I wouldn't deliver a scathing code review even though it would probably make you feel better, at least for a little bit. Instead, I'd approach the person and see if they misunderstood the task, were overwhelmed etc. Approach it from a teachable moment first, if a pattern of behavior and bad code continues then address it with increasingly direct feedback.

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.

tl;dr Don't try to fix the code, try to fix the process that produced the code. This is an opportunity to encourage and teach, berating will produce the opposite result.

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!

> It's very possible that they are overwhelmed and scared at this new job.

from personal experience, biggest reason

I'd basically say:

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

Agreed: this is now an in-person conversation where you have an opportunity to help your colleague grow.

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

Others have mentioned it incidentally, but I'll highlight it:

When you're at this point, talk to them offline, either face to face, video conference, but somewhere privately. The core message should be that the code is sufficiently convoluted that you can't effectively review it and you think there must be a better approach. Comprehensibility is always a code review concern, so you're on solid ground there.

The big thing here is that there's a point where the honest code review will basically be "wow, you screwed this up big time," at which point doing it in front of other people will make things worse and not better--your coworker will probably dig in their heels and will definitely feel humiliated enough to not really be open to constructive criticism. Comes down to the whole criticize privately/praise publicly thing. It's not about politics, just human nature. And as much as people should be able to take a critical code review, taking one that effectively says you bungled the entire thing requires rather superhuman objectivity.

Unlike others, I'll say that you don't have to turn this into a teaching or mentoring moment unless you're the lead and that's your job. If you want to do so, great, but that can be a lot of load to add to what should be a straightforward task. It would be good for you to be able to articulate in neutral terms some of the issues that make the code incomprehensible. You can even get a sanity check and maybe a few ideas from a trusted colleague.

Your best outcome here is a followup comment from your coworker saying "after offline conversation with reinhardt, I decided to rework the code and will resubmit."

You gotta find a way to figure out if this person is CAPABLE of delivering better code. That's gonna hinge on their talent, and their willingness to learn. At some point, some folks make it clear, one way or another, that they can't/won't get much better, at which point you unload them.

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.

That was my worry: the guy who writes 400 line functions/methods/subs is always gonna do just that when nobody is looking.

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.

As the code reviewer you should probably not be doing any writing. Instead, you should send back the code with notes. An in-person meeting would be better to go over the changes.

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

This is not a direct answer to your question, but might be relevant nonetheless.

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.

This is a great opportunity to pair program with this person. You have the advantage of knowing the codebase and the company style/nuances. Sit down and show him. Don't point the things that are wrong. Make a list if all the functionality required and then read line by line or chunk by chunk. Write code and tests as you move along. Let him write it but you take lead on how. Ultimately, step on his shoes. It's the worst feeling in the world when someone says you wrote shit code. Don't make him feel shitty. Make him feel like he is part of a growing team.

You can be honest with them without bringing up things that aren't relevant. There's a reason you don't like the PR: it's too big, it has a lot of conditional logic, etc etc. There's a bit of personal dislike with it, but that dislike is also founded. There's increased complexity, which makes it harder for others to understand, and most likely harder to test. It comes out as a personal feeling of "ughhh" when you look at the code, but there's an objective component to it that, if you take the time to, you should be able to get across.

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.

I don't have enough information to give much advice.

    - Is the programmer Jr or Sr level?
    - Are they new to the language?
    - Are they new to the architecture/style?

Obviously spaghetti is not acceptable. But if they are a beginner, then you have an opportunity to be generous in your review and help teach them (as you should for beginners). If they are senior level and they should know the language then it may be a sign that it was not a good hire.

I've had this happen before, and the code review was met with resistance -- ultimately the person on the team least able to design modular architecture was given perhaps one of the most critical architectures to design, and it was more difficult because management gave him the task because everybody else was busy with other tasks.

It 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've been on the receiving end of several code reviews that could be summarized as "You didn't write this exactly the same way as if I had written it." This was at places that did not have uniform written standards uniformly applied. It was just the boss swinging his dick.

Code that I've seen like that is usually an organic method that grew to meet additional requirements and edge cases.

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.

I think refactor and show them could help them learn.

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

I'd focus on the distant goal: Getting this coworker to make higher-quality contributions in the future. (Which helps him/her in their career just as much as it helps your project). In that light, I think it's doing them a favor to be honest. Refactoring their code for them (cleaning up their mess) isn't really teaching them much, or setting yourself up for anything but a repeat of these events. In your position, I'd probably reject the pull request and give them a handful of pointers/things-to-work-on.

You don't have to mean about it, just be straightforward and make sure you're only addressing the inadequacies in the code itself, not attacking them as a person. I'd try to convey this message (in your own words): "I appreciate the hard work you've done. That being said, I think you're a very capable programmer, so I'd like to see you push yourself even harder. If you were less-talented I would accept the pull-request, but I'm betting that we can get even higher quality output from you."

First, be polite. Actually, I've found this to be a useful mantra for all things business-related.

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.

I'd recommend ignoring Maratd's advice.

I used to be a horrible coder. I used to have just the issues you describe here. I wrote spaghetti code, I didn't understand objects and classes properly, I probably had every bad habit in the book.

I kept getting noticed as a 'good programmer' because I did stuff other people didn't get. I was good at figuring out the puzzles how to make pieces fit, but I was, quite frankly, a bad coder.

I got a contract for a big company, and I wrote the app they wanted. The code was awful, but I didn't know that. When I finished the contract and passed the app over to their IT division, it went through a 'code review' process.

I'd like to be able to tell you that the CTO pulled me aside and told me what was wrong with my code. Had he done that, I would have taken back all my work and refactored it for them so it was well written. But, of course, he didn't do that.

If the CTO had taken the time to explain to me why my code was bad, I would be very grateful for the learning experience. I would look back on that time as a defining moment which sent me on my course as a professional software engineer. But he didn't, so instead, I gradually, over the course of a few years, learned what it meant to write good code.

I probably lost my first coding job because my code wasn't great, but it could have been, had somebody taken the time to explain to me why it wasn't good.

So, I hope you can look at this not as a time for a 'scathing code review', but rather as an opportunity to help a struggling coder to maybe become excellent.

I think the most important thing may be to NOT do the work for him. Stop at what you've got so far and you can use it as a comparison, so he sees what the difference is between what you did and what he did.

Then let him take another shot at it. If he doesn't want to redo it, or thinks he can't, then you'll know he's a bad employee and a likely not going to grow to become a good coder. If he jumps at the chance and is eager to learn, you'll know that he might have a shot at this.

Do make sure you go through your HR process (if you have one) of recording the poor performance though. But let him know that you have to do that just so that the company isn't stuck with him if he isn't able to adapt. But if this is approached properly, it may be the best gift you could give the new hire.

I work with a lot of contractors and so I end up doing this frequently. Here's the pattern I follow:

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.

Try separating out the factual observations from how you feel about them. "The main logic is in a single ~400 LOC function" is a simple observation and less likely to make your co-worker feel attacked. I think the word "monolithic" is counter-productive.

"I noticed that the main logic is in a single ~400 LOC function. I'm concerned because I may need to make updates to this in the future, and larger chunks of code are harder to work with. Would you be willing to pull it apart into smaller functions?"

I think that "I wouldn't dare touch it with a 10-foot pole" is something that I'd leave out of the PR comment. The observation that it is complicated, and the concern that future work on it would be difficult, that can definitely be included.

http://xkcd.com/1513/

Treat this as an education opportunity, one that can be used to mentor the new hire. Just having her/him rewrite the code from scratch will not help prevent a repeat of the problem.

Keep doing what you are doing in refactoring and fixing it. Spend hours on it. Tell the person you spent hours on it. Tell them you expect them it will take them longer, but these are the standards you expect of yourself.

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.

I'd really counsel against refactoring and fixing it, that is not what code review is for, you review, comment and return and leave the refactoring and fixing to the submitter (see above for a more comprehensive solution).

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 can see the point on being a bottleneck.

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.

I like to follow 3 simple rules.

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

I never had to give a scathing code review but every time I had to give one with a lot of requests for changes I would always point out why I'm making each request - with links to style guides, articles about code quality, code smells, best practices etc. There are also automated tools that can do some of that for you (codeclimate.com comes to mind) so that you can delegate some of your 'work' there.

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.

New hires need to be onboarded properly - specifically, you need to set expectations quickly. Having a place (like confluence, or other wiki) where these are documented is a good start (so you can later reference them) - but simply put, you should sit down next to this person, and help them to understand how, but more importantly, WHY you would suggest certain changes. Take the time to build trust with this person. Make sure, at all costs, that you come from a place of helping / teaching - dont come across as an ass.

Reject the pull request, but be civil about it. Explain what is wrong with the code and give some pointers for fixing it.

I suggest you do this in person. Rejecting someone's work can easy come across as maligning their skills in general, particularly in print. It will be easier to make the distinction face to face. This will also allow for more discussion of what changes are needed, if your coworker has questions.

Before you do anything, please look into this kid's (?) mental state and try to gauge how hypersensitive he may be to being critiqued.

As you said, he's a newly hired coworker. I think the last thing he wants is to be "outed".

If I were you, I'd offer some explanation as to why you want to reject it, and then offer to start him on the correct path.

You are already going about it the right way, by doing two things: 1. you are specific 2. you are investing time to offer a better way.

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!

If you're already weeding through the code, why not invite the coworker to sit down next to you? In my experience, sometimes all someone needs is a pattern to adhere to, and once you show that pattern things might work out.

Of course, this is only good about 80% of the time. Sometimes people just don't get it.

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

I disagree, that's exactly the ideal response.

Do you have code standards that state maximum function length, what "clean code" looks like, etc.?

If not, then that ought to be a priority, so that you can point people at that in the future, when rejecting pulls.

Pair programming pretty much eliminates this issue

Reply on news.ycombinator.com