
Ask HN: How to avoid giving a scathing code review? - reinhardt
For the past several hours I&#x27;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&#x2F;else spaghetti code. I wouldn&#x27;t dare touching it with a 10 foot pole without tests but fortunately there are some and the original test coverage isn&#x27;t bad. So I&#x27;ve cut it down to almost half the size so far but there&#x27;s still way to go until it reaches a somewhat maintainable state. I&#x27;ve also found and fixed a few bugs along the way so strict refactoring is not even a goal at this point.<p>So I&#x27;m wondering how to go about this. A few code review comments here and there won&#x27;t cut it. If I was to be brutally honest, I&#x27;d outright reject the PR and tell him to rewrite it from scratch in a clean way but that&#x27;s probably not the ideal response. How do you deal with situations like this?
======
rusanu
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.

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

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

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

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

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

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

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

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

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

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

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

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

~~~
rancur
> fallacy of the excluded middle

wordy synonym for false dichotomy?

~~~
jacquesm
No.

    
    
       X --- excluded middle --- Y
    

Versus

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

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

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

------
abannin
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!

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

from personal experience, biggest reason

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

------
drallison
[http://xkcd.com/1513/](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.

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

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

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

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

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

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

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

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

------
limer
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!

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

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

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

------
dustingetz
Pair programming pretty much eliminates this issue

