TE
TechEcho
Home24h TopNewestBestAskShowJobs
GitHubTwitter
Home

TechEcho

A tech news platform built with Next.js, providing global tech news and discussions.

GitHubTwitter

Home

HomeNewestBestAskShowJobs

Resources

HackerNews APIOriginal HackerNewsNext.js

© 2025 TechEcho. All rights reserved.

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

66 pointsby reinhardtabout 10 years ago
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?

34 comments

rusanuabout 10 years ago
What&#x27;s wrong with giving exactly that feedback? &quot;The main logic is too monolithic and deeply nested. Please refactor in a more maintainable version, split into several simpler methods&quot;.<p>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&#x27;t think is useful to do the rewrite yourself in the review: you are robbing the original author of a chance to redeem him&#x2F;her self. I also think that refactoring a complex function is not a &quot;rewrite from scratch&quot;, and even a rewrite from scratch on a second iteration is seldom as time consuming&#x2F;complicated as the first iteration.<p>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.<p>If your managers pressure you or your coworkers to &quot;go easy&quot; on the quality (ie. &quot;It works, let him check it in!&quot;) 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&#x27;s pain later, and in such a case you&#x27;re in a bad environment and not much you can do.<p>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.<p>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.
评论 #9477925 未加载
kstenerudabout 10 years ago
If the code is that far off the deep end, then your job at this point needs to shift from code reviewer to teacher.<p>You need to patiently explain not only what is wrong, but also why it&#x27;s wrong, and how to fix it, with examples. You might want to even sit down and refactor the code with him.<p>You need to explain the code design concepts and principles that he has missed. In short, you need to GUIDE him.<p>It&#x27;s either that or get rid of him, because until SOMEONE teaches him, he will continue doing what he&#x27;s doing and remain oblivious.
评论 #9477689 未加载
评论 #9478152 未加载
评论 #9477745 未加载
评论 #9477788 未加载
jacquesmabout 10 years ago
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&#x2F;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 &#x27;tofix&#x27; items.<p>Wholesale refactoring (especially by you, really, do not refactor someone else&#x27;s pull requests, teach them what you want to see instead!) and&#x2F;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&#x27;d argue for assigning a more experienced &#x27;buddy&#x27; 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&#x2F;or questionable.<p>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.
评论 #9478041 未加载
评论 #9477861 未加载
kirseabout 10 years ago
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&#x27;ve produced.<p>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 &quot;this block of code sucks&quot; if there&#x27;s an unarguable set of reasons&#x2F;metrics attached to that statement. Usually better if the reasons come before the conclusion. Avoid style preferences or bringing general identity accusations like &quot;all <i>your</i> code sucks&quot; into the review.<p>Keep the focus entirely on the product not the producer and that&#x27;s fair game. Maybe tell the newly hired coworker a story about your code once used to suck, and how you overcame that.
评论 #9477816 未加载
评论 #9478097 未加载
评论 #9478173 未加载
评论 #9477855 未加载
jurassicabout 10 years ago
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&#x27;s sensitivity&#x2F;fear&#x2F;hostility to feedback when the code is your shared work product. &quot;How could we make this better?&quot; feels a million times better than &quot;You should have done it this other better way&quot;.<p>I&#x27;d definitely give it a try, at least for onboarding new people. There&#x27;s no better way to transmit knowledge about the code and expectations for how to work with it.
davismwflabout 10 years ago
I wouldn&#x27;t deliver a scathing code review even though it would probably make you feel better, at least for a little bit. Instead, I&#x27;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.<p>I state this because I have done the opposite and it can backfire in ways you don&#x27;t initially think. So I have learned it is always best to start of with an inquisitive mind on why he&#x2F;she wrote it this way and what were their instructions and then use it as a teaching moment to help them be better.<p>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&#x27;s early 2000&#x27;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&#x27;t hire from without really screening someone heavily.<p>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.
abanninabout 10 years ago
tl;dr Don&#x27;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.<p>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:<p>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 &lt; 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&#x27;m really excited to have your enthusiasm (or other personal trait) as part of the team. Let&#x27;s go get this!<p>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 &quot;this is how I can improve&quot; but rather &quot;they are out to get me and not to be trusted&quot;. Humans need a sense of safety. 3) It&#x27;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&#x27;s also very possible that they&#x27;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.<p>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.<p>Great question!
评论 #9478288 未加载
cauterizedabout 10 years ago
I&#x27;d basically say:<p>&quot;Before I accepted this pull request, I would ask you to do a lot of refactoring. However, it&#x27;s too complicated to explain in comments on the diff. Why don&#x27;t we sit down and fix it together?&quot;<p>Then explain your reasoning for the overall changes and each smaller change face to face in a pairing session (or series thereof).
评论 #9477720 未加载
geoelectricabout 10 years ago
Others have mentioned it incidentally, but I&#x27;ll highlight it:<p>When you&#x27;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&#x27;t effectively review it and you think there must be a better approach. Comprehensibility is always a code review concern, so you&#x27;re on solid ground there.<p>The big thing here is that there&#x27;s a point where the honest code review will basically be &quot;wow, you screwed this up big time,&quot; 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&#x2F;praise publicly thing. It&#x27;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.<p>Unlike others, I&#x27;ll say that you don&#x27;t have to turn this into a teaching or mentoring moment unless you&#x27;re the lead and that&#x27;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.<p>Your best outcome here is a followup comment <i>from your coworker</i> saying &quot;after offline conversation with reinhardt, I decided to rework the code and will resubmit.&quot;
uptownJimmyabout 10 years ago
You gotta find a way to figure out if this person is CAPABLE of delivering better code. That&#x27;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&#x27;t&#x2F;won&#x27;t get much better, at which point you unload them.<p>Until then, you need to be friendly and patient, for a number of reasons. Don&#x27;t betray shock, and don&#x27;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.<p>Or, as the good writers say: show them, don&#x27;t tell them.
评论 #9477973 未加载
Jemaclusabout 10 years ago
As the code <i>reviewer</i> you should probably not be doing any writing. Instead, you should <i>send back</i> the code with notes. An in-person meeting would be better to go over the changes.<p>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).<p>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&#x27;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...
pfgabout 10 years ago
This is not a direct answer to your question, but might be relevant nonetheless.<p>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).<p>There&#x27;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.
blitiabout 10 years ago
This is a great opportunity to pair program with this person. You have the advantage of knowing the codebase and the company style&#x2F;nuances. Sit down and show him. Don&#x27;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&#x27;s the worst feeling in the world when someone says you wrote shit code. Don&#x27;t make him feel shitty. Make him feel like he is part of a growing team.
tobzabout 10 years ago
You can be honest with them without bringing up things that aren&#x27;t relevant. There&#x27;s a reason you don&#x27;t like the PR: it&#x27;s too big, it has a lot of conditional logic, etc etc. There&#x27;s a bit of personal dislike with it, but that dislike is also founded. There&#x27;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 &quot;ughhh&quot; when you look at the code, but there&#x27;s an objective component to it that, if you take the time to, you should be able to get across.<p>Everyone is different, and I certainly don&#x27;t know all of the right words to say, so I&#x27;ll simply say what I think should be mentioned no matter what: point out the problems with the code, not the person. &quot;What if we broke this apart in two right here? It looks like it&#x27;ll be hard to test this in isolation otherwise.&quot; &quot;What are your thoughts on some sort of pattern matching &#x2F; switch statement here instead of using if&#x2F;else blocks? Any downsides? Might end up being a little cleaner.&quot;<p>You&#x27;re trying to make the case the code can be better, but not that it&#x27;s horribly bad. I mean, if it works, but it&#x27;s ugly, you have a base. They got the job done. Now it&#x27;s time for you to help them make it better.
binarymaxabout 10 years ago
I don&#x27;t have enough information to give much advice.<p><pre><code> - Is the programmer Jr or Sr level? - Are they new to the language? - Are they new to the architecture&#x2F;style? </code></pre> 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.
mpdehaan2about 10 years ago
I&#x27;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.<p>It can be difficult. Somes some people aren&#x27;t <i>meant</i> to be developers, but ultimately you need to foster a good culture of software craftsmanship.<p>If you have the same manager, I&#x27;d see about talking to them first about how to approach it.<p>If you don&#x27;t, you are in trouble.<p>Hopefully, this person wants to learn how to write better code.<p>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 <i>wrong</i> as well. A 400 line function does seem wrong, but just because you&#x27;re not using someone&#x27;s favorite Ruby function or not writing all 2-line methods doesn&#x27;t mean that is wrong either.<p>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.<p>Perhaps it would be possible, if you&#x27;re on good terms with this person, to set up a pair refactoring session where you start to unwind it?<p>I think it&#x27;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.<p>I always like to encourage folks to talk about code as it&#x27;s being written when I can.
评论 #9477581 未加载
sleazebreezeabout 10 years ago
Code that I&#x27;ve seen like that is usually an organic method that grew to meet additional requirements and edge cases.<p>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&#x27;ll never learn how to do a better job in the future.
评论 #9477636 未加载
MalcolmDiggsabout 10 years ago
I&#x27;d focus on the distant goal: Getting this coworker to make higher-quality contributions in the future. (Which helps him&#x2F;her in their career just as much as it helps your project). In that light, I think it&#x27;s doing them a favor to be honest. Refactoring their code for them (cleaning up their mess) isn&#x27;t really teaching them much, or setting yourself up for anything but a repeat of these events. In your position, I&#x27;d probably reject the pull request and give them a handful of pointers&#x2F;things-to-work-on.<p>You don&#x27;t have to mean about it, just be straightforward and make sure you&#x27;re only addressing the inadequacies in the code itself, not attacking them as a person. I&#x27;d try to convey this message (in your own words): &quot;I appreciate the hard work you&#x27;ve done. That being said, I think you&#x27;re a very capable programmer, so I&#x27;d like to see you push yourself even harder. If you were less-talented I would accept the pull-request, but I&#x27;m betting that we can get even higher quality output from you.&quot;
HeyLaughingBoyabout 10 years ago
First, be polite. Actually, I&#x27;ve found this to be a useful mantra for all things business-related.<p>Next, realize that if it works, the developer may consider it done. Many organizations don&#x27;t see maintainability as an important goal and stop at functionality.<p>So, with those two things in mind, I&#x27;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).<p>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 &quot;mini-me&#x27;s&quot;<p>tl;dr: be nice, point out how the rest of the team codes, suggest a rewrite based on the concepts you bring up.
pedalpeteabout 10 years ago
I&#x27;d recommend ignoring Maratd&#x27;s advice.<p>I used to be a horrible coder. I used to have just the issues you describe here. I wrote spaghetti code, I didn&#x27;t understand objects and classes properly, I probably had every bad habit in the book.<p>I kept getting noticed as a &#x27;good programmer&#x27; because I did stuff other people didn&#x27;t get. I was good at figuring out the puzzles how to make pieces fit, but I was, quite frankly, a bad coder.<p>I got a contract for a big company, and I wrote the app they wanted. The code was awful, but I didn&#x27;t know that. When I finished the contract and passed the app over to their IT division, it went through a &#x27;code review&#x27; process.<p>I&#x27;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&#x27;t do that.<p>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&#x27;t, so instead, I gradually, over the course of a few years, learned what it meant to write good code.<p>I probably lost my first coding job because my code wasn&#x27;t great, but it could have been, had somebody taken the time to explain to me why it wasn&#x27;t good.<p>So, I hope you can look at this not as a time for a &#x27;scathing code review&#x27;, but rather as an opportunity to help a struggling coder to maybe become excellent.<p>I think the most important thing may be to NOT do the work for him. Stop at what you&#x27;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.<p>Then let him take another shot at it. If he doesn&#x27;t want to redo it, or thinks he can&#x27;t, then you&#x27;ll know he&#x27;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&#x27;ll know that he might have a shot at this.<p>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&#x27;t stuck with him if he isn&#x27;t able to adapt. But if this is approached properly, it may be the best gift you could give the new hire.
vkjvabout 10 years ago
I work with a lot of contractors and so I end up doing this frequently. Here&#x27;s the pattern I follow:<p>1. Reject the PR. It needs to be clear that this isn&#x27;t acceptable. 2. Realize that stopping with rejection doesn&#x27;t help the problem at all. :) 3. Start with some light comments and refactoring a couple of sections (usually it&#x27;s the same bad patterns repeated). 4. Only provide the refactors as comments. Try not to fix other people&#x27;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.<p>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.
ThrustVectoringabout 10 years ago
Try separating out the factual observations from how you feel about them. &quot;The main logic is in a single ~400 LOC function&quot; is a simple observation and less likely to make your co-worker feel attacked. I think the word &quot;monolithic&quot; is counter-productive.<p>&quot;I noticed that the main logic is in a single ~400 LOC function. I&#x27;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?&quot;<p>I think that &quot;I wouldn&#x27;t dare touch it with a 10-foot pole&quot; is something that I&#x27;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.
drallisonabout 10 years ago
<a href="http:&#x2F;&#x2F;xkcd.com&#x2F;1513&#x2F;" rel="nofollow">http:&#x2F;&#x2F;xkcd.com&#x2F;1513&#x2F;</a><p>Treat this as an education opportunity, one that can be used to mentor the new hire. Just having her&#x2F;him rewrite the code from scratch will not help prevent a repeat of the problem.
proksoupabout 10 years ago
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.<p>Ask your boss &#x2F; 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&#x2F;minutes rewriting this employees work. Ask if that employee should be spending 2x&#x2F;3x&#x2F;4x the amount of time on their code to get to that level.<p>edit: What I mean is, it doesn&#x27;t sound scathing if it&#x27;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.
评论 #9477655 未加载
ac2uabout 10 years ago
I like to follow 3 simple rules.<p>- Don&#x27;t phrase your feedback in such a way that you wouldn&#x27;t feel comfortable saying to this person&#x27;s face, or have read out in court. In other words, be professional.<p>- Mark each piece of feedback as &quot;Blocker&#x2F;Not-a-Blocker&quot;, to cover issues which (in your opinion), need addressed before using or are simply opinion points which are at the author&#x27;s discretion on whether to use.<p>- If you know of a better way to write it, include a snippet or some pseudocode if possible. If it&#x27;s more complex than a snippet and it&#x27;s a work situation, then write your point succinctly and then approach in person and offer your assistance in pairing if they would like.
fishnchipsabout 10 years ago
I never had to give a <i>scathing</i> code review but every time I had to give one with a lot of requests for changes I would always point out why I&#x27;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 &#x27;work&#x27; there.<p>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&#x27;s.
nartzabout 10 years ago
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 &#x2F; teaching - dont come across as an ass.
johan_larsonabout 10 years ago
Reject the pull request, but be civil about it. Explain what is wrong with the code and give some pointers for fixing it.<p>I suggest you do this in person. Rejecting someone&#x27;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.
joshmnabout 10 years ago
Before you do anything, please look into this kid&#x27;s (?) mental state and try to gauge how hypersensitive he may be to being critiqued.<p>As you said, he&#x27;s a newly hired coworker. I think the last thing he wants is to be &quot;outed&quot;.<p>If I were you, I&#x27;d offer some explanation as to why you want to reject it, and then offer to start him on the correct path.
limerabout 10 years ago
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.<p>A good way to communicate is to keep it informal and provide direct feedback, w&#x2F;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&#x2F;trust&#x2F;respect between you and your new colleague.<p>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?<p>For smaller teams, daily team code reviews are very helpful.<p>Have fun!
ryanthejugglerabout 10 years ago
If you&#x27;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.<p>Of course, this is only good about 80% of the time. Sometimes people just don&#x27;t get it.
copsarebastardsabout 10 years ago
&gt; 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.<p>I disagree, that&#x27;s exactly the ideal response.
AndrewDuckerabout 10 years ago
Do you have code standards that state maximum function length, what &quot;clean code&quot; looks like, etc.?<p>If not, then that ought to be a priority, so that you can point people at that in the future, when rejecting pulls.
dustingetzabout 10 years ago
Pair programming pretty much eliminates this issue