40 ms·

Tips for reviewing code you don’t like

282 points by kesor 7y ago

lmm 7y ago
Most of the advice seems to amount to watering down one's criticism. If we do that, won't the result be accepting less-good code into our projects? Is that the outcome we want?
- dhuramas 7y ago
  I don't think OP wants a project to accept anything "less-good". It's just a post asking to be respectful to the contributor. Attack the idea(in a civilized manner) - not the person.
- gnozzle 7y ago
  I didn't read any of these as watering down. Which ones do you think dilute the message?
  - LanceH 7y ago
    Exactly. I seems like most of these are jumping off points for starting a conversation. I may be wrong but I read the article as a senior reviewing a junior's code, in which case the big win is not just getting better code in now, but getting a better developer. Starting the conversation instead of jumping all over them is important. Even when reviewing a peer, while more terse we tend to phrase things in questions, "Shouldn't this be in a lib instead?" Frequently it's oversight on their part, but sometimes they have their reasons and I learn something in return.
  - lmm 7y ago
    "How do we do X with this change?" is less clear if what you really mean is "This change makes X impossible". "Thank you for your contribution" is misleading when talking about a contribution that simply doesn't meet minimal quality standards. "Please" suggests that you consider something optional, which is misleading if you are actually expressing a requirement. Snide or hyperbolic remarks convey a message; perhaps not a welcome one, but one whose meaning is presumably intentional.
- BurningFrog 7y ago
  This assumes that criticism produces good code. If only it was that easy.
- waldfee 7y ago
  In my experience, this is not the case when both the reviewer and the reviewee are working together and want to find a good solution. Rephrasing criticism is a pretty common technique not exclusive to code reviews and imho helps avoid a defensive stance which makes it easier to be productive.
- jasonlotito 7y ago
  This advice is similar to the Hacker News Guidelines [1] Such as: > Be kind. Don't be snarky. Comments should get more thoughtful and substantive, not less, as a topic gets more divisive. or > Please don't post shallow dismissals, especially of other people's work. A good critical comment teaches us something. and > Please respond to the strongest plausible interpretation of what someone says, not a weaker one that's easier to criticize. Assume good faith. Being kind doesn't mean being a push over. Rather, it makes it easier to achieve the goals you want. None of these suggestions advise you to accept less-good code. Rather, it's all about making it easier to get better code. I could have also just said "Can't you see that you are obviously wrong?" and left it at that. Would that have been better? 1. https://news.ycombinator.com/newsguidelines.html https://news.ycombinator.com/newsguidelines.html
- watwut 7y ago
  Your comment is factual untrue. Most advice is actually removing reviewers emotional reactions in exchange for more factual statements. The only watering down is that it gives benefit of the doubt to reviewed code where difference of opinion may be the issue.
- bluGill 7y ago
  No, for several different reasons. It is often hard to tell the difference between great code and bad code. I've written code that was fast and very flexible, at the expense of being complex - the flexibility turned out not used at all (after 8 years in production I'm confident it never will be), so on hindsight this wasn't great code - but if the flexibility envisioned had been used the complexity would be required and the solution would be great. I've seen code that cleverly handled multi-threaded issues such that can't be understood or modified by anyone without breaking - but it great code because it abstracts the threads so nobody else has to understand them. Second, when you first start on a project you don't understand it all. We want to grow your understanding and bring you in as trusted contributor. Setting the right tone in those first days makes all the difference in the question of will you contribute more code or not. Last, most open source projects are essentially unmaintained. Almost any contribution is better than that, even if the code is bad, if it works at all...
  - reallydontask 7y ago
    > It is often hard to tell the difference between great code and bad code. I've written code that was fast and very flexible, at the expense of being complex - the flexibility turned out not used at all (after 8 years in production I'm confident it never will be), so on hindsight this wasn't great code - but if the flexibility envisioned had been used the complexity would be required and the solution would be great. I've seen code that cleverly handled multi-threaded issues such that can't be understood or modified by anyone without breaking - but it great code because it abstracts the threads so nobody else has to understand them. At my previous company, the lead dev did this (the complex code part with the lack of usage too), unfortunately his view was that it might be used sometime. I think, after 10 years, we were handling about 100 events per second peak load and what I pointed this out he countered with: What if we had to handle 100000 or 1 Million? I gave up after that. For context we were a B2B company with a relatively long sales cycle, so ramp ups were generally very strongly correlated with on-boarding of new customers and we normally had notice of what was coming. In other words, the naive solution would've worked fine ( the 100 events (not web) were across ~ 5 servers (servers did other stuff too)) and we would've been able to scale up while we did improvements to the naive solution.
- nkozyra 7y ago
  The idea is to express the intent without the emotion. I don't see any watering down there; in fact, it's likely the criticism will be clearer, more direct and more palatable.
- AstralStorm 7y ago
  The first comment actually means a strong opinion. Impossible usually really means impossible from technical standpoint and is not used for things that are heavily inconsiderate or inconveniencing future development. The suggested option is Machiavellian and patronizing at the same time, and pushing burden of proof on the author of the code instead of reviewer. Puts them on defensive. Both are evil, but first is clearly an opinion, second is also a personal attack. Proper review really must have "X because Y" form with perhaps exception for trivial syntax fixes. Like form 5. Part two also forces additional burden of proof on the author. Generally for performance changes documentation requirement is the only thing. Preferably runtime numbers or profiles rather than abstract notions. We've seen people going for algorithmic gold plating where it actually list performance to constant factors. Doing 6 in excess is viewed as patronizing. There is no good way around it. And finally, presuming you know the teammates making the review as bland as possible without a bit of color can be the worst thing as reviews will be felt as a chore. They are one still but humor can help. Some people like their reviews to be ultimately super boring instead, and you have to be careful.
- jsgo 7y ago
  Seems to be advocating more for thoughtful criticism (and meaningful discussion) than anything. Is it going to make things better? To what degree, I don't know, but I'd imagine it helps with people creating pull requests and then disappearing from the project (which I've seen multiple complaints of this behavior over the years). It'd make them feel more of a stake in it I'd imagine if they're not receiving blowback from the maintainer for daring to create a pull request (based on the "bads" in the examples). I've personally never seen anything as bad as the examples given (I've read stuff from Linus that are there, but seems like he's trying to change the atmosphere there now), but if the communication is that prickly, I can't imagine it is going to receive a ton of outside help in the form of pull requests (that could be intentional though).
- m463 7y ago
  I believe the idea is clear communication and constructive criticism. (or possibly to avoid being sheldon in the big bang theory)
wickerman 7y ago
First rule about code reviewing is that you don't tell someone how you would've written it, but rather try to point out pros/cons of current vs proposed approach. It's as easy as that. Every time I see a piece of code I don't like I ask myself: why do I not like it? I've gotten PR reviews with people literally writing "I don't like this" even after I wrote a very detailed technical argument about my reasoning. My only approach to that is to remind them that I really don't care about their personal preferences, because no discussion will come out of them addressing none of my technical arguments and instead relying on what their personal preference is like.
- organsnyder 7y ago
  Agreed. In these cases, the reviewer should always aim for learning as much as teaching (if not more so). I've gone into code reviews where my reaction began with, "ugh, this is terrible", but evolved into, "well, I probably would have done it differently, but this approach handles an edge case I hadn't considered".
  - makapuf 7y ago
    Completely agree with all of this. Some say be hard with the code, soft with the coder. Also don't nitpick and be nice. Simple.
    - filoleg 7y ago
      In cases like this, I explicitly preface it with "nit: <text here>", explain how I would do it myself instead and why, and then (also explicitly) say that this is more of a preference thing and that they should feel free to leave it as is. It might turn out they like my approach more and change the code, but if not, it is still totally cool with me, as they learned of just one more way to do the same thing that might come useful later.
  - just_myles 7y ago
    Same. I've heard it all. From "terrible", "You don't know what you're doing", "does it work?", to "ok, I see why you did it so try this way instead because X". It's honestly who you are working with.
raverbashing 7y ago
I'll just say that if you think Linus' comments are bad, there are much more (needlessly) rude people in LKML. Linux might be incisive but he's often right and it's usually about an impactful issue. Some other people are just gratuitously more rude and/or obnoxious.
- titzer 7y ago
  This is why it doesn't matter if you are right are not, if the attitude that comes from the top is rude and obnoxious, then other "less right" people will follow suit. The culture then degrades as people generally emulate the behavior of elites. The example of behavior should be set at the top. Being technically right (this time) is no license to train others how to be an ass.
  - Ididntdothis 7y ago
    In all fairness Linus often makes pretty detailed comments with a lot of patience. He only blows up from time to time. There aren’t many tech leaders who have the capacity to understand issues and then explain them.
spicyusername 7y ago
I think the spirit of these is applicable to any situation where multiple parties need to come to consensus - i.e. try to focus on positive, respectful, inclusive, and objective modes of communication.
Traster 7y ago
This seems like a pretty good list, I managed a team previously where they would make these mistakes in pretty much every code review - and the result of one engineer being nasty during a code review was very quickly a culture where code reviews were an opportunity to settle scores and showboat rather than an actual functional process. A lot of this advice could be boiled down to "Would you make this comment on reddit, or would you say it to your colleague's face?" The only thing I would add is: >If a submission is too large to be reasonably reviewed, it is okay to let the submitter know right away. Keep moving forward. Doesn't seem right to me - or atleast the focus really should be making sure that no one submits code for review that's too big for review, and that means early feedback about a feature getting too complex and breaking it down. It's much easier to go into a problem saying "I'm going to do X, it's going to be huge, so I'll do A then B then C and submit them separately". If a submission is too big to reivew it's probably non-trivial to go back and break up so that can be quite annoying.
- singron 7y ago
  The number 1 reason why I actually try to make code reviews small is because the few times I made big code reviews, they just sat there. They get a few comments here and there, but the reviewers seem reluctant to review them at all, or just keep doing partial reads without the confidence to approve the whole thing. I even found the relationship non-linear. I.e. if you can break up a big change into smaller changes that have more overall code, you can still probably get those reviewed quicker. I probably would have learned the lesson faster if reviewers had just rejected the changes for being too big, or at least a "next time you could have split it this way".
reallydontask 7y ago
I use to hate receiving comments like this (the bad ones) yet I use to give them myself almost without realising it. Obviously, I was right to give them and wrong to receive them ;-> It took me a while to realise that the same message can be delivered in a far nicer manner, which generally leads to a lot less conflict. Now it almost comes naturally to write nice comments that deliver the same message, namely: the code could be improved
alexandercrohde 7y ago
Honestly, in my opinion, the fact that this type of thing can even happen suggests the format of code-review has a problem. I propose it's better to: 1. Sit at the desk of the person and pair-code review. Helps avoid basic misunderstadnings, and also is more private that a permanent written record where people may feel defensive about their choices. Also it's easier for a lot of people to be rude through the internet. 2. Write in small notes if any must-address concerns arise and are agreed-upon (e.g. "Security?", "duplicates function x") If your devs can't interact face-to-face, then I think you've got major cultural problems or hired the wrong devs.
- smcameron 7y ago
  Or you're working on an open source project.
  - organsnyder 7y ago
    Maybe a shared-screen video call, then? Same for non-colocated colleagues.
    - Crinus 7y ago
      This makes it much harder for people to review stuff since they'll need to schedule for such reviews. Normally reviews happen asynchronously whenever people find time and often code review tools allow for writing a review piece-by-piece (that is, your progress is saved on the server that hosts the review) instead of all-in-one. Introducing any form of friction makes reviews less desirable for everyone involved.
      - organsnyder 7y ago
        Agreed. But for complicated reviews, a high-bandwidth synchronous meeting can be useful. Perhaps start out asynchronous, with the reviewer requesting a live meeting/call if they feel it's necessary? That's always worked well for me.
  - Crinus 7y ago
tomxor 7y ago
> It’s not a political or emotional argument For FOSS at least, ultimately maintainers and authors are not beholden to anyone who wants to submit code, it's perfectly ok to say NO even for political or emotional reasons... so if not in the direct context of a PR then where? I think heated and unconstructive PRs probably arise as a result of pushy and entitled submitters, after a certain point authors will inevitably get fed up and abandon diplomatic approaches.
srfilipek 7y ago
> 9. Say please I actually don't like this. Maybe it's just me, but I always read these as being condescending, written by someone who thinks that I won't take a suggestion without unnecessary pleasantries and/or begging, or they're frustrated and trying to mask it. > “Could you please align these variable definitions so they’re easier to read?” > “Could you align these variable definitions so they’re easier to read?” I like the latter.
- Double_a_92 7y ago
  Agree. I'm not trying to do the reviewer a favor, nor are they asking one from me. Even if it's some personal style thing, I don't perceive "Could you...?" as rude or anything. If anything an exaggerated please could seem more insulting, as in "Could you please just do this simple thing ffs!?"
  - nroach 7y ago
    Tolerance for and expectation of "please" can be cultural. OS code may receive contributions from global contributors. Anecdotally, in my team which spans individuals from each continent except Africa and the poles, US english speakers are the least likely to use 'please', whereas our APAC (asia-pacific) contributors use it liberally.
    - wolco 7y ago
      Canadians will use it liberally along with thanks.
      - sverige 7y ago
        And sorry.
    - marcus_holmes 7y ago
      or SE Asian team members who say "yes boss, sure" and then do nothing... and then you find they disagreed with the thing you proposed but there's no way in their culture that they can disagree with the boss, so they just say yes and do nothing. That one was fun to deal with ;)
raxxorrax 7y ago
I have started to love these "hey programmers, here are some tips for being a nice person for once"-articles. I haven't been in too many code reviews, but my experience is that it is often taken personally if developers aren't friends with each other. I vastly prefer unit and integration tests to measure code quality. I can also understand the reluctance of maintainers to include code of a different style. Here the tips from the article probably help, but I think it happens often enough that code is rejected for non-technical issues.
- gtirloni 7y ago
  Yes, as much as possible should be automated by tests and style guides but they don't cover everything. I'm not aware of automated ways to validate architectural soundness and other abstract features.
Double_a_92 7y ago
The most annoying thing is when the reviewer doesn't recognise the complexity of some problem, and just insists on doing it in some "simple" way that doesn't solve the whole problem. "Oh you spent 2 weeks figuring this solution out? Well here's my 2 minutes of naïve ideas..."
- Ididntdothis 7y ago
  The problem is that the reviewer often doesn’t have time enough time to really understand the problem. So you often end up making trivial comments because you don’t really understand what’s going on.
- lolc 7y ago
  When the reviewer can't understand why the complexity is needed, I add a comment explaining it. This is a very frequent response by me: "Yeah, it's not obvious why the simple approach doesn't work. I've added a comment."
- OJFord 7y ago
  Sometimes I self-review when I can anticipate that, i.e. just leave some (non-code, though it might make me consider that they should be in code) comments with a bit more context. And, of course, commit messages.
- m463 7y ago
  code reviews are for education, which can go both ways.
ravenstine 7y ago
My biggest problem with code reviews isn't so much the phrasing(although it is definitely important), but the fact that they almost never has a "done" criterion. In my opinion, if new code has nothing objectively wrong with it(no security flaws, no unacceptable design patterns, no foreseen bugs, etc.), then the author has the prerogative to merge the code. Reviews shouldn't go on in perpetuity because different reviewers would all have written something differently than the author submitting the code. I've experienced this phenomenon at every company I've worked for at one point or another, and it's basically a way for Waterfall to sneak its way back in to an Agile process. (not that I'm an advocate of Agile) It doesn't happen too often, but I find it more frustrating than just about anything else. Beyond some reasonable adjustments to be made in the first wave or 2 of review comments, any other non-critical changes should be separated into tickets for the next sprint or simply be labeled as DIY or "do it yourself". If you want something done, but there's no actual standard for doing it on your team, then ordering people to write things your way via code review is really just a form of backseat coding. My code is never perfect, but that doesn't mean I want to change variable names, swap out libraries, or use different approaches merely because someone else prefers them that way. You think the variables should have different names? Wonderful! Go do it yourself. I'm not trying to be rude, but I've got other shit to do, and the software already works beautifully.
- jedimastert 7y ago
  > My biggest problem with code reviews isn't so much the phrasing(although it is definitely important), but the fact that they almost never has a "done" criterion. I've actually been experiencing this myself, as I've recently started a job with code reviews, and it's been driving me crazy. > In my opinion, if new code has nothing objectively wrong with it(no security flaws, no unacceptable design patterns, no foreseen bugs, etc.), then the author has the prerogative to merge the code. One of my biggest personal issues currently is that I have a fear of hitting the GO button, doing one more look-over just in case. I suppose it's just a lack of confidence in my own abilities, or maybe being anxious that my code isn't up to the standards of everyone else and the code base as a whole.
- BurningFrog 7y ago
  Code reviews can be much better, faster, and friendlier if done in person. It's also harder to ask people to spend half a day doing extra work on a whim in person.
stunt 7y ago
I found three different reasons for people doing a bad job at code review. - Not having the required soft skills to do it. - Cultural differences. Because, What is considered offensive, rude, or negative in many cultures, might be just direct in some cultures. But I do believe cultural awareness is also a soft skill. - Having a bad mentor/peer/environment. Many junior developers write reviews with the same style as they receive reviews.
- pbhjpbhj 7y ago
  Comment review: you have to put a line space [extra blank line] in order for HN not to collapse the text into a single line. - first - second instead of -first -second https://ibb.co/jLtZTFS https://ibb.co/jLtZTFS - image shows editor pre-submission ;o) HN should use [limited] markdown or something IMO.
  - m463 7y ago
    I wish there was a way for a bullet list. Then again, simple text might mean better minimalist communication.
docker_up 7y ago
First thing before any code review is that your team/company needs to have a coding standard. This is the first rule of engagement and eliminates most problems. Without a coding standard, code reviews quickly devolve into crap. Once you have a coding standard, then the reviewer needs to differentiate between opinion and fact. If you don't like the variable name but it conforms to the coding standard then it's not a valid review comment. The coding standard should give some guidance as to what a variable name or function name should be. If there's a bug or if there are potential for issues then that's what I focus on.
agapon 7y ago
I think that this is a very good article on code reviews: https://jml.io/pages/your-code-sucks-and-i-hate-you.html https://jml.io/pages/your-code-sucks-and-i-hate-you.html
pkamb 7y ago
The problem I have with most similar articles/comments is that the only two options for Code Review now seem to be: - point out strict violations of the team's enshrined code style guidelines - accept any and all code as-is, so long as there are no "bugs" or performance issues Which, at that point, why are we even doing PRs? Unit test and lint... When I receive a Code Review I want to know about the function that reduces my 10 lines to 1. I want someone to notice that my git mistake introduced a bunch of unneeded diff noise. I want to change variable names that are confusing to the first reviewer, because that means they'll be confusing to me when I read the code again next month. Give me the development "human factors" in the code review. Some of that will be subjective and that's ok.
- JoeAltmaier 7y ago
  I know the pain - but its no use reducing it to a dichotomy (All or Nothing). The strong reactions are due to pedants who block the source base doorway and pick at everything going by. It can sink a project. So folks push back, say things like "Only comment if its a real bug!" and so on. There's lots of middle ground in a company source process. For instance, point out real bugs if present. But for anything else, comment and approve, respecting the contributing Engineer to do what they can in the time available.
- skybrian 7y ago
  I think you might have to ask for that sort of feedback since people differ in how much criticism they can take. (Sometimes based on how busy they are.) But the tips in this article work for subjective comments too. Rephrasing comments as questions is nearly always applicable.
- tomjakubowski 7y ago
  Code review at least ensures that more than one person has seen and read the code. Hopefully they've at least kind of understood it, too. That's valuable in itself against the bus factor or "WTF is this?" surprises.
thanatos519 7y ago
I was expecting a way to receive "tips" (small cash payments) for reviewing code ... :/
- civilian 7y ago
  lol, I was expecting to receive advice I wouldn't like for reviewing code.
JamesBarney 7y ago
I feel like code reviews comments should be of several types. 1. There is a bug in this code(most important) 2. This violates the norms on the project 3. This is less maintainable because it can cause a future bug(two pieces of code in different that need to be consistent) 4. I personally had trouble understanding this code, here's what took me a while to understand. Would you mind adding a comment or temp variable to speed this up. 5. Here's a cleaner way to do it(totally in writers court whether or not they implement this unless they're a jr. dev. I make a lot less code review comments than I used to because I've seen too many bugs introduced by fairly innocuous looking code review changes and the amount of bikeshedding that can go into code reviews comments that will likely never materially effect the application.
- goostavos 7y ago
  >code review comments that will likely never materially effect the application I think that is actually a good bar for a CR comment. Issue of maintainability affect the application. Bugs affect the application. A lack of general good software architecture has long term effects on the application. All other comments I either tag on an "as an option, you could..." or force myself to shut down the "that's not how I would have done it!" voice in my head and just leave no comment. On the receiving end, few things are more obnoxious than "deck chair rearranging" comments which have no bearing on performance, maintainability, style, or correctness, but just boil down to you satisfying the benign preferences of whoever is looking at your code.
fjfaase 7y ago
I have noticed that when doing a code review, I ask a lot of why questions. I want to understand why the coder has chosen for the given solution and if any other solutions were taking into consideration. I also want to know if the coder things that all use cases are covered. I find that I often happy with the answers, when I realize that the coder thought about the solution. Usually, there are multiple ways to implement something and that there is no clear choice which is better. Trying to make into a black/white problem, is not the best way to deal with the inherently grayness of software engineering. Make a fuss about coding standards, for example, is often not productive.
MattyRad 7y ago
> 1. Rephrase your objection as a question Careful with this one. In my experience, you aren't fooling anyone when you ask someone a question to which you yourself know the answer. It can quickly come off as condescending. One helpful practice that we have is that we make a distinction between blocking and non-blocking comments. Nonblocking comments are desired changes, but aren't required by the submitter to fix (formatting, code simplification, naming, etc). This steers people in the right direction without preventing them from getting work done. Blocking changes are critical things which must be resolved before the code gets merged (security risks, performance concerns, complexity concerns). And of course, every comment should be followed with a suggestion for how to resolve it (even if it's meeting in person). "This is bad" obviously doesn't fly.
- watwut 7y ago
  What helps here is to then actually listen to answer and try to learn from it. As in, it becomes less condescending when the answer is listened to and actively acted and reacted upon in follow up conversation.
watwut 7y ago
The most infuriating code rewiev was the one where I ended up with code that I considered worst then my original one. The second most infuriating one was series where I had to rewrite code because rewiever did not knew syntax I used amd claimed it is difficult to read. Three weeks later he was forcing me to use that same syntax - because he found some blog where he learned it so it became necessary. Like what the hell.
TheCapn 7y ago
I'm kind of shocked at how many recommendations are merely, "have tact and don't be a dick to your coworkers." How many code reviews are shitslinging mudfests where you guys work?? Now admittedly I mostly work on projects where I'm the sole developer so structured code reviews aren't part of my daily routine, but during the period where I managed interns we'd do code reviews before every merge which mostly involved sitting down and going through the functions. I'd point out deficiencies and work to teach the student developers new tools/libraries/resources they weren't aware of. At no point did we exchange shit like "This change sucks", "Can’t you see that this is obviously wrong?", or "This is stupid code written by a stupid person."... all examples from an article which is also toting the "Avoid hyperbole" mantra. Even a single one of those snide remarks would be grounds for discipline or potentially termination if it were consistent. My tip? Separate your ego from your code. If you can't discuss criticism objectively and professionally you shouldn't be involved in a professional career. Hostile workplaces are shit to be part of and I will never stand for that among my peers.
- itronitron 7y ago
  Yeah, several of those 'bad' comments are completely inappropriate. I can't imagine how the author even thought of those in the first place.
  - JetezLeLogin 7y ago
    Some classic rants and raves of the recently trying-to-do-better Linus Torvalds come to mind!
- civilian 7y ago
  After 2 years of developing at a scrappy design&dev shop, I got a new job that was a big step up at a hip SAAS company. I was in my mid-20s and I still hadn't worked through a lot of my anxiety issues, and I started getting code review from these two intimidating brothers-- one an engineering manager and one a principal dev. Their review style was very terse, and they called out _every instance_ of a syntax 'mistake' I made, and it just made me feel like absolute shit. It wasn't just me, several of my coworkers felt the same about just getting hammered on by them. This has kind of permanently changed my approach to code review. I always call out the things I like in a batch of code. I acknowledge that there are some code styles preferences that are arbitrary, but that I have a preference in and it's good to have a consistent codebase. If the person is working for me, or if a coworker took a nasty task that nobody wanted, I thank them for the code with the LGTM.
gwbas1c 7y ago
I review a LOT of code, and honestly, I didn't find this article helpful. It's common sense on how to behave in a civil manner, but it doesn't really help with having effective code reviews. Code reviews accomplish a few things: - They are primarily a discussion about code changes. ("Why did you do this?" - They help onboard new members of the team, both in terms of how to work most effectively with the code, and with style. (No style guide is 100% exhaustive, "We normally name variables for that fooBar, not barFoo.") - They help team members learn from each other. ("That's a cool new function in the new version of XYZ library!") - They help enforce good engineering practice. ("I'm not merging that until you write tests.") - They help make code more readable. ("Please add a comment here, and rename that method to DoSomething.") - They also quickly identify people who shouldn't be part of the team. Everyone sees that someone moves slowly in a pull request, or that someone resists cleaning up sloppy code. The thing with people who resist cleaning up their sloppy code is that it shows to management. This happens when everyone else in the team gets their pull requests merged quickly, and one person's pull requests pile up with lots of minor comments.
- efitz 7y ago
  > It’s common sense on how to behave in a civil manner I am frequently surprised by how lacking many technical professionals are in EQ/soft skills/civility -often I observe people who apparently believe that their skill is such that they don’t need to treat their peers with respect. This isn’t anomalous; it’s widespread. [Conjecture/opinion] I believe that many people who enter software development and related fields are high on the autism scale- eg aspergers, etc., and that many such people are unaware of how badly that they’re treating others. It’s not “common sense” for many.
  - wolfgke 7y ago
    > I am frequently surprised by how lacking many technical professionals are in EQ/soft skills/civility -often I observe people who apparently believe that their skill is such that they don’t need to treat their peers with respect. This isn’t anomalous; it’s widespread. I personally think that terseness is actually polite since it wastes a lot less time for both sides.
tawat987 7y ago
I disagree with the basic premise of the article that maintainers are obliged to merge code changes by default. One reason is that the maintenance burden falls on more than just the author, so there needs to be some consideration on whether it is maintainable and whether the people involved are willing and able to maintain it. Particularly if the change adds a public API or behavior, it may have to be maintained for the life of the codebase. Also, some projects have very high reliability expectations and you have to assume that code is broken until proven otherwise. I won't merge code until the author has demonstrated via tests, comments and clearly-written code that the change does as expected and doesn't introduce potential issues. The attitude in the article isn't particularly conducive to the long-term health and quality of a project, since you get a bunch of authors ramming through their changes based on their agenda and end up with an inconsistent mess that nobody wants to maintain.
- m463 7y ago
  I was mentally thinking about refuting your argument, but then I started wondering about systemd (which I do not like, although I do have concrete criticisms).
kazinator 7y ago
This article is complete garbage. :) We absolutely must consider the non-technical aspects of a change, and it behooves us to consider them before other aspects. Changes are often driven by new requirements. If we approve the change, we are approving the adoption of those requirements. There isn't always a technical reason for such an adoption other than "someone wants it that way". Based on this article, I mustn't reject, for instance, GPL-ed code being added to a BSD-licensed program. There is no technical argument against it. We can just switch everything to the GPL license and march forward; what's the problem? Can we put religion aside and just discuss the code? There also no technical reason why a compiler shouldn't have a --tetris option for playing a little game on the text console. No wait, I didn't say anything about text; we can just link in SDL and have it graphical! Anyway, can we just discuss the best way to do this, and not emotional debates about whether or not to do it? So, "someone wants it that way"? Whooptee doo, so effin' what? Let them fork the code.
deleted 7y ago
[deleted]

282 points by kesor 7y ago

lmm 7y ago

Most of the advice seems to amount to watering down one's criticism. If we do that, won't the result be accepting less-good code into our projects? Is that the outcome we want?

dhuramas 7y ago
I don't think OP wants a project to accept anything "less-good". It's just a post asking to be respectful to the contributor. Attack the idea(in a civilized manner) - not the person.
gnozzle 7y ago
I didn't read any of these as watering down. Which ones do you think dilute the message?
- LanceH 7y ago
  Exactly. I seems like most of these are jumping off points for starting a conversation. I may be wrong but I read the article as a senior reviewing a junior's code, in which case the big win is not just getting better code in now, but getting a better developer. Starting the conversation instead of jumping all over them is important. Even when reviewing a peer, while more terse we tend to phrase things in questions, "Shouldn't this be in a lib instead?" Frequently it's oversight on their part, but sometimes they have their reasons and I learn something in return.
- lmm 7y ago
  "How do we do X with this change?" is less clear if what you really mean is "This change makes X impossible". "Thank you for your contribution" is misleading when talking about a contribution that simply doesn't meet minimal quality standards. "Please" suggests that you consider something optional, which is misleading if you are actually expressing a requirement. Snide or hyperbolic remarks convey a message; perhaps not a welcome one, but one whose meaning is presumably intentional.
BurningFrog 7y ago
This assumes that criticism produces good code. If only it was that easy.
waldfee 7y ago
In my experience, this is not the case when both the reviewer and the reviewee are working together and want to find a good solution. Rephrasing criticism is a pretty common technique not exclusive to code reviews and imho helps avoid a defensive stance which makes it easier to be productive.
jasonlotito 7y ago
This advice is similar to the Hacker News Guidelines [1] Such as: > Be kind. Don't be snarky. Comments should get more thoughtful and substantive, not less, as a topic gets more divisive. or > Please don't post shallow dismissals, especially of other people's work. A good critical comment teaches us something. and > Please respond to the strongest plausible interpretation of what someone says, not a weaker one that's easier to criticize. Assume good faith. Being kind doesn't mean being a push over. Rather, it makes it easier to achieve the goals you want. None of these suggestions advise you to accept less-good code. Rather, it's all about making it easier to get better code. I could have also just said "Can't you see that you are obviously wrong?" and left it at that. Would that have been better? 1. https://news.ycombinator.com/newsguidelines.html https://news.ycombinator.com/newsguidelines.html
watwut 7y ago
Your comment is factual untrue. Most advice is actually removing reviewers emotional reactions in exchange for more factual statements. The only watering down is that it gives benefit of the doubt to reviewed code where difference of opinion may be the issue.
bluGill 7y ago
No, for several different reasons. It is often hard to tell the difference between great code and bad code. I've written code that was fast and very flexible, at the expense of being complex - the flexibility turned out not used at all (after 8 years in production I'm confident it never will be), so on hindsight this wasn't great code - but if the flexibility envisioned had been used the complexity would be required and the solution would be great. I've seen code that cleverly handled multi-threaded issues such that can't be understood or modified by anyone without breaking - but it great code because it abstracts the threads so nobody else has to understand them. Second, when you first start on a project you don't understand it all. We want to grow your understanding and bring you in as trusted contributor. Setting the right tone in those first days makes all the difference in the question of will you contribute more code or not. Last, most open source projects are essentially unmaintained. Almost any contribution is better than that, even if the code is bad, if it works at all...
- reallydontask 7y ago
  > It is often hard to tell the difference between great code and bad code. I've written code that was fast and very flexible, at the expense of being complex - the flexibility turned out not used at all (after 8 years in production I'm confident it never will be), so on hindsight this wasn't great code - but if the flexibility envisioned had been used the complexity would be required and the solution would be great. I've seen code that cleverly handled multi-threaded issues such that can't be understood or modified by anyone without breaking - but it great code because it abstracts the threads so nobody else has to understand them. At my previous company, the lead dev did this (the complex code part with the lack of usage too), unfortunately his view was that it might be used sometime. I think, after 10 years, we were handling about 100 events per second peak load and what I pointed this out he countered with: What if we had to handle 100000 or 1 Million? I gave up after that. For context we were a B2B company with a relatively long sales cycle, so ramp ups were generally very strongly correlated with on-boarding of new customers and we normally had notice of what was coming. In other words, the naive solution would've worked fine ( the 100 events (not web) were across ~ 5 servers (servers did other stuff too)) and we would've been able to scale up while we did improvements to the naive solution.
nkozyra 7y ago
The idea is to express the intent without the emotion. I don't see any watering down there; in fact, it's likely the criticism will be clearer, more direct and more palatable.
AstralStorm 7y ago
The first comment actually means a strong opinion. Impossible usually really means impossible from technical standpoint and is not used for things that are heavily inconsiderate or inconveniencing future development. The suggested option is Machiavellian and patronizing at the same time, and pushing burden of proof on the author of the code instead of reviewer. Puts them on defensive. Both are evil, but first is clearly an opinion, second is also a personal attack. Proper review really must have "X because Y" form with perhaps exception for trivial syntax fixes. Like form 5. Part two also forces additional burden of proof on the author. Generally for performance changes documentation requirement is the only thing. Preferably runtime numbers or profiles rather than abstract notions. We've seen people going for algorithmic gold plating where it actually list performance to constant factors. Doing 6 in excess is viewed as patronizing. There is no good way around it. And finally, presuming you know the teammates making the review as bland as possible without a bit of color can be the worst thing as reviews will be felt as a chore. They are one still but humor can help. Some people like their reviews to be ultimately super boring instead, and you have to be careful.
jsgo 7y ago
Seems to be advocating more for thoughtful criticism (and meaningful discussion) than anything. Is it going to make things better? To what degree, I don't know, but I'd imagine it helps with people creating pull requests and then disappearing from the project (which I've seen multiple complaints of this behavior over the years). It'd make them feel more of a stake in it I'd imagine if they're not receiving blowback from the maintainer for daring to create a pull request (based on the "bads" in the examples). I've personally never seen anything as bad as the examples given (I've read stuff from Linus that are there, but seems like he's trying to change the atmosphere there now), but if the communication is that prickly, I can't imagine it is going to receive a ton of outside help in the form of pull requests (that could be intentional though).
m463 7y ago
I believe the idea is clear communication and constructive criticism. (or possibly to avoid being sheldon in the big bang theory)

wickerman 7y ago

First rule about code reviewing is that you don't tell someone how you would've written it, but rather try to point out pros/cons of current vs proposed approach. It's as easy as that. Every time I see a piece of code I don't like I ask myself: why do I not like it? I've gotten PR reviews with people literally writing "I don't like this" even after I wrote a very detailed technical argument about my reasoning. My only approach to that is to remind them that I really don't care about their personal preferences, because no discussion will come out of them addressing none of my technical arguments and instead relying on what their personal preference is like.

organsnyder 7y ago
Agreed. In these cases, the reviewer should always aim for learning as much as teaching (if not more so). I've gone into code reviews where my reaction began with, "ugh, this is terrible", but evolved into, "well, I probably would have done it differently, but this approach handles an edge case I hadn't considered".
- makapuf 7y ago
  Completely agree with all of this. Some say be hard with the code, soft with the coder. Also don't nitpick and be nice. Simple.
  - filoleg 7y ago
    In cases like this, I explicitly preface it with "nit: <text here>", explain how I would do it myself instead and why, and then (also explicitly) say that this is more of a preference thing and that they should feel free to leave it as is. It might turn out they like my approach more and change the code, but if not, it is still totally cool with me, as they learned of just one more way to do the same thing that might come useful later.
- just_myles 7y ago
  Same. I've heard it all. From "terrible", "You don't know what you're doing", "does it work?", to "ok, I see why you did it so try this way instead because X". It's honestly who you are working with.

raverbashing 7y ago

I'll just say that if you think Linus' comments are bad, there are much more (needlessly) rude people in LKML. Linux might be incisive but he's often right and it's usually about an impactful issue. Some other people are just gratuitously more rude and/or obnoxious.

titzer 7y ago
This is why it doesn't matter if you are right are not, if the attitude that comes from the top is rude and obnoxious, then other "less right" people will follow suit. The culture then degrades as people generally emulate the behavior of elites. The example of behavior should be set at the top. Being technically right (this time) is no license to train others how to be an ass.
- Ididntdothis 7y ago
  In all fairness Linus often makes pretty detailed comments with a lot of patience. He only blows up from time to time. There aren’t many tech leaders who have the capacity to understand issues and then explain them.

spicyusername 7y ago

I think the spirit of these is applicable to any situation where multiple parties need to come to consensus - i.e. try to focus on positive, respectful, inclusive, and objective modes of communication.

Traster 7y ago

This seems like a pretty good list, I managed a team previously where they would make these mistakes in pretty much every code review - and the result of one engineer being nasty during a code review was very quickly a culture where code reviews were an opportunity to settle scores and showboat rather than an actual functional process. A lot of this advice could be boiled down to "Would you make this comment on reddit, or would you say it to your colleague's face?" The only thing I would add is: >If a submission is too large to be reasonably reviewed, it is okay to let the submitter know right away. Keep moving forward. Doesn't seem right to me - or atleast the focus really should be making sure that no one submits code for review that's too big for review, and that means early feedback about a feature getting too complex and breaking it down. It's much easier to go into a problem saying "I'm going to do X, it's going to be huge, so I'll do A then B then C and submit them separately". If a submission is too big to reivew it's probably non-trivial to go back and break up so that can be quite annoying.

singron 7y ago
The number 1 reason why I actually try to make code reviews small is because the few times I made big code reviews, they just sat there. They get a few comments here and there, but the reviewers seem reluctant to review them at all, or just keep doing partial reads without the confidence to approve the whole thing. I even found the relationship non-linear. I.e. if you can break up a big change into smaller changes that have more overall code, you can still probably get those reviewed quicker. I probably would have learned the lesson faster if reviewers had just rejected the changes for being too big, or at least a "next time you could have split it this way".

reallydontask 7y ago

I use to hate receiving comments like this (the bad ones) yet I use to give them myself almost without realising it. Obviously, I was right to give them and wrong to receive them ;-> It took me a while to realise that the same message can be delivered in a far nicer manner, which generally leads to a lot less conflict. Now it almost comes naturally to write nice comments that deliver the same message, namely: the code could be improved

alexandercrohde 7y ago

Honestly, in my opinion, the fact that this type of thing can even happen suggests the format of code-review has a problem. I propose it's better to: 1. Sit at the desk of the person and pair-code review. Helps avoid basic misunderstadnings, and also is more private that a permanent written record where people may feel defensive about their choices. Also it's easier for a lot of people to be rude through the internet. 2. Write in small notes if any must-address concerns arise and are agreed-upon (e.g. "Security?", "duplicates function x") If your devs can't interact face-to-face, then I think you've got major cultural problems or hired the wrong devs.

smcameron 7y ago
Or you're working on an open source project.
- organsnyder 7y ago
  Maybe a shared-screen video call, then? Same for non-colocated colleagues.
  - Crinus 7y ago
    This makes it much harder for people to review stuff since they'll need to schedule for such reviews. Normally reviews happen asynchronously whenever people find time and often code review tools allow for writing a review piece-by-piece (that is, your progress is saved on the server that hosts the review) instead of all-in-one. Introducing any form of friction makes reviews less desirable for everyone involved.
    - organsnyder 7y ago
      Agreed. But for complicated reviews, a high-bandwidth synchronous meeting can be useful. Perhaps start out asynchronous, with the reviewer requesting a live meeting/call if they feel it's necessary? That's always worked well for me.
- Crinus 7y ago

tomxor 7y ago

> It’s not a political or emotional argument For FOSS at least, ultimately maintainers and authors are not beholden to anyone who wants to submit code, it's perfectly ok to say NO even for political or emotional reasons... so if not in the direct context of a PR then where? I think heated and unconstructive PRs probably arise as a result of pushy and entitled submitters, after a certain point authors will inevitably get fed up and abandon diplomatic approaches.

srfilipek 7y ago

> 9. Say please I actually don't like this. Maybe it's just me, but I always read these as being condescending, written by someone who thinks that I won't take a suggestion without unnecessary pleasantries and/or begging, or they're frustrated and trying to mask it. > “Could you please align these variable definitions so they’re easier to read?” > “Could you align these variable definitions so they’re easier to read?” I like the latter.

Double_a_92 7y ago
Agree. I'm not trying to do the reviewer a favor, nor are they asking one from me. Even if it's some personal style thing, I don't perceive "Could you...?" as rude or anything. If anything an exaggerated please could seem more insulting, as in "Could you please just do this simple thing ffs!?"
- nroach 7y ago
  Tolerance for and expectation of "please" can be cultural. OS code may receive contributions from global contributors. Anecdotally, in my team which spans individuals from each continent except Africa and the poles, US english speakers are the least likely to use 'please', whereas our APAC (asia-pacific) contributors use it liberally.
  - wolco 7y ago
    Canadians will use it liberally along with thanks.
    - sverige 7y ago
      And sorry.
  - marcus_holmes 7y ago
    or SE Asian team members who say "yes boss, sure" and then do nothing... and then you find they disagreed with the thing you proposed but there's no way in their culture that they can disagree with the boss, so they just say yes and do nothing. That one was fun to deal with ;)

raxxorrax 7y ago

I have started to love these "hey programmers, here are some tips for being a nice person for once"-articles. I haven't been in too many code reviews, but my experience is that it is often taken personally if developers aren't friends with each other. I vastly prefer unit and integration tests to measure code quality. I can also understand the reluctance of maintainers to include code of a different style. Here the tips from the article probably help, but I think it happens often enough that code is rejected for non-technical issues.

gtirloni 7y ago
Yes, as much as possible should be automated by tests and style guides but they don't cover everything. I'm not aware of automated ways to validate architectural soundness and other abstract features.

Double_a_92 7y ago

The most annoying thing is when the reviewer doesn't recognise the complexity of some problem, and just insists on doing it in some "simple" way that doesn't solve the whole problem. "Oh you spent 2 weeks figuring this solution out? Well here's my 2 minutes of naïve ideas..."

Ididntdothis 7y ago
The problem is that the reviewer often doesn’t have time enough time to really understand the problem. So you often end up making trivial comments because you don’t really understand what’s going on.
lolc 7y ago
When the reviewer can't understand why the complexity is needed, I add a comment explaining it. This is a very frequent response by me: "Yeah, it's not obvious why the simple approach doesn't work. I've added a comment."
OJFord 7y ago
Sometimes I self-review when I can anticipate that, i.e. just leave some (non-code, though it might make me consider that they should be in code) comments with a bit more context. And, of course, commit messages.
m463 7y ago
code reviews are for education, which can go both ways.

ravenstine 7y ago

My biggest problem with code reviews isn't so much the phrasing(although it is definitely important), but the fact that they almost never has a "done" criterion. In my opinion, if new code has nothing objectively wrong with it(no security flaws, no unacceptable design patterns, no foreseen bugs, etc.), then the author has the prerogative to merge the code. Reviews shouldn't go on in perpetuity because different reviewers would all have written something differently than the author submitting the code. I've experienced this phenomenon at every company I've worked for at one point or another, and it's basically a way for Waterfall to sneak its way back in to an Agile process. (not that I'm an advocate of Agile) It doesn't happen too often, but I find it more frustrating than just about anything else. Beyond some reasonable adjustments to be made in the first wave or 2 of review comments, any other non-critical changes should be separated into tickets for the next sprint or simply be labeled as DIY or "do it yourself". If you want something done, but there's no actual standard for doing it on your team, then ordering people to write things your way via code review is really just a form of backseat coding. My code is never perfect, but that doesn't mean I want to change variable names, swap out libraries, or use different approaches merely because someone else prefers them that way. You think the variables should have different names? Wonderful! Go do it yourself. I'm not trying to be rude, but I've got other shit to do, and the software already works beautifully.

jedimastert 7y ago
> My biggest problem with code reviews isn't so much the phrasing(although it is definitely important), but the fact that they almost never has a "done" criterion. I've actually been experiencing this myself, as I've recently started a job with code reviews, and it's been driving me crazy. > In my opinion, if new code has nothing objectively wrong with it(no security flaws, no unacceptable design patterns, no foreseen bugs, etc.), then the author has the prerogative to merge the code. One of my biggest personal issues currently is that I have a fear of hitting the GO button, doing one more look-over just in case. I suppose it's just a lack of confidence in my own abilities, or maybe being anxious that my code isn't up to the standards of everyone else and the code base as a whole.
BurningFrog 7y ago
Code reviews can be much better, faster, and friendlier if done in person. It's also harder to ask people to spend half a day doing extra work on a whim in person.

stunt 7y ago

I found three different reasons for people doing a bad job at code review. - Not having the required soft skills to do it. - Cultural differences. Because, What is considered offensive, rude, or negative in many cultures, might be just direct in some cultures. But I do believe cultural awareness is also a soft skill. - Having a bad mentor/peer/environment. Many junior developers write reviews with the same style as they receive reviews.

pbhjpbhj 7y ago
Comment review: you have to put a line space [extra blank line] in order for HN not to collapse the text into a single line. - first - second instead of -first -second https://ibb.co/jLtZTFS https://ibb.co/jLtZTFS - image shows editor pre-submission ;o) HN should use [limited] markdown or something IMO.
- m463 7y ago
  I wish there was a way for a bullet list. Then again, simple text might mean better minimalist communication.

docker_up 7y ago

First thing before any code review is that your team/company needs to have a coding standard. This is the first rule of engagement and eliminates most problems. Without a coding standard, code reviews quickly devolve into crap. Once you have a coding standard, then the reviewer needs to differentiate between opinion and fact. If you don't like the variable name but it conforms to the coding standard then it's not a valid review comment. The coding standard should give some guidance as to what a variable name or function name should be. If there's a bug or if there are potential for issues then that's what I focus on.

agapon 7y ago

I think that this is a very good article on code reviews: https://jml.io/pages/your-code-sucks-and-i-hate-you.html https://jml.io/pages/your-code-sucks-and-i-hate-you.html

pkamb 7y ago

The problem I have with most similar articles/comments is that the only two options for Code Review now seem to be: - point out strict violations of the team's enshrined code style guidelines - accept any and all code as-is, so long as there are no "bugs" or performance issues Which, at that point, why are we even doing PRs? Unit test and lint... When I receive a Code Review I want to know about the function that reduces my 10 lines to 1. I want someone to notice that my git mistake introduced a bunch of unneeded diff noise. I want to change variable names that are confusing to the first reviewer, because that means they'll be confusing to me when I read the code again next month. Give me the development "human factors" in the code review. Some of that will be subjective and that's ok.

JoeAltmaier 7y ago
I know the pain - but its no use reducing it to a dichotomy (All or Nothing). The strong reactions are due to pedants who block the source base doorway and pick at everything going by. It can sink a project. So folks push back, say things like "Only comment if its a real bug!" and so on. There's lots of middle ground in a company source process. For instance, point out real bugs if present. But for anything else, comment and approve, respecting the contributing Engineer to do what they can in the time available.
skybrian 7y ago
I think you might have to ask for that sort of feedback since people differ in how much criticism they can take. (Sometimes based on how busy they are.) But the tips in this article work for subjective comments too. Rephrasing comments as questions is nearly always applicable.
tomjakubowski 7y ago
Code review at least ensures that more than one person has seen and read the code. Hopefully they've at least kind of understood it, too. That's valuable in itself against the bus factor or "WTF is this?" surprises.

thanatos519 7y ago

I was expecting a way to receive "tips" (small cash payments) for reviewing code ... :/

civilian 7y ago
lol, I was expecting to receive advice I wouldn't like for reviewing code.

JamesBarney 7y ago

I feel like code reviews comments should be of several types. 1. There is a bug in this code(most important) 2. This violates the norms on the project 3. This is less maintainable because it can cause a future bug(two pieces of code in different that need to be consistent) 4. I personally had trouble understanding this code, here's what took me a while to understand. Would you mind adding a comment or temp variable to speed this up. 5. Here's a cleaner way to do it(totally in writers court whether or not they implement this unless they're a jr. dev. I make a lot less code review comments than I used to because I've seen too many bugs introduced by fairly innocuous looking code review changes and the amount of bikeshedding that can go into code reviews comments that will likely never materially effect the application.

goostavos 7y ago
>code review comments that will likely never materially effect the application I think that is actually a good bar for a CR comment. Issue of maintainability affect the application. Bugs affect the application. A lack of general good software architecture has long term effects on the application. All other comments I either tag on an "as an option, you could..." or force myself to shut down the "that's not how I would have done it!" voice in my head and just leave no comment. On the receiving end, few things are more obnoxious than "deck chair rearranging" comments which have no bearing on performance, maintainability, style, or correctness, but just boil down to you satisfying the benign preferences of whoever is looking at your code.

fjfaase 7y ago

I have noticed that when doing a code review, I ask a lot of why questions. I want to understand why the coder has chosen for the given solution and if any other solutions were taking into consideration. I also want to know if the coder things that all use cases are covered. I find that I often happy with the answers, when I realize that the coder thought about the solution. Usually, there are multiple ways to implement something and that there is no clear choice which is better. Trying to make into a black/white problem, is not the best way to deal with the inherently grayness of software engineering. Make a fuss about coding standards, for example, is often not productive.

MattyRad 7y ago

> 1. Rephrase your objection as a question Careful with this one. In my experience, you aren't fooling anyone when you ask someone a question to which you yourself know the answer. It can quickly come off as condescending. One helpful practice that we have is that we make a distinction between blocking and non-blocking comments. Nonblocking comments are desired changes, but aren't required by the submitter to fix (formatting, code simplification, naming, etc). This steers people in the right direction without preventing them from getting work done. Blocking changes are critical things which must be resolved before the code gets merged (security risks, performance concerns, complexity concerns). And of course, every comment should be followed with a suggestion for how to resolve it (even if it's meeting in person). "This is bad" obviously doesn't fly.

watwut 7y ago
What helps here is to then actually listen to answer and try to learn from it. As in, it becomes less condescending when the answer is listened to and actively acted and reacted upon in follow up conversation.

watwut 7y ago

The most infuriating code rewiev was the one where I ended up with code that I considered worst then my original one. The second most infuriating one was series where I had to rewrite code because rewiever did not knew syntax I used amd claimed it is difficult to read. Three weeks later he was forcing me to use that same syntax - because he found some blog where he learned it so it became necessary. Like what the hell.

TheCapn 7y ago

I'm kind of shocked at how many recommendations are merely, "have tact and don't be a dick to your coworkers." How many code reviews are shitslinging mudfests where you guys work?? Now admittedly I mostly work on projects where I'm the sole developer so structured code reviews aren't part of my daily routine, but during the period where I managed interns we'd do code reviews before every merge which mostly involved sitting down and going through the functions. I'd point out deficiencies and work to teach the student developers new tools/libraries/resources they weren't aware of. At no point did we exchange shit like "This change sucks", "Can’t you see that this is obviously wrong?", or "This is stupid code written by a stupid person."... all examples from an article which is also toting the "Avoid hyperbole" mantra. Even a single one of those snide remarks would be grounds for discipline or potentially termination if it were consistent. My tip? Separate your ego from your code. If you can't discuss criticism objectively and professionally you shouldn't be involved in a professional career. Hostile workplaces are shit to be part of and I will never stand for that among my peers.

itronitron 7y ago
Yeah, several of those 'bad' comments are completely inappropriate. I can't imagine how the author even thought of those in the first place.
- JetezLeLogin 7y ago
  Some classic rants and raves of the recently trying-to-do-better Linus Torvalds come to mind!
civilian 7y ago
After 2 years of developing at a scrappy design&dev shop, I got a new job that was a big step up at a hip SAAS company. I was in my mid-20s and I still hadn't worked through a lot of my anxiety issues, and I started getting code review from these two intimidating brothers-- one an engineering manager and one a principal dev. Their review style was very terse, and they called out _every instance_ of a syntax 'mistake' I made, and it just made me feel like absolute shit. It wasn't just me, several of my coworkers felt the same about just getting hammered on by them. This has kind of permanently changed my approach to code review. I always call out the things I like in a batch of code. I acknowledge that there are some code styles preferences that are arbitrary, but that I have a preference in and it's good to have a consistent codebase. If the person is working for me, or if a coworker took a nasty task that nobody wanted, I thank them for the code with the LGTM.

gwbas1c 7y ago

I review a LOT of code, and honestly, I didn't find this article helpful. It's common sense on how to behave in a civil manner, but it doesn't really help with having effective code reviews. Code reviews accomplish a few things: - They are primarily a discussion about code changes. ("Why did you do this?" - They help onboard new members of the team, both in terms of how to work most effectively with the code, and with style. (No style guide is 100% exhaustive, "We normally name variables for that fooBar, not barFoo.") - They help team members learn from each other. ("That's a cool new function in the new version of XYZ library!") - They help enforce good engineering practice. ("I'm not merging that until you write tests.") - They help make code more readable. ("Please add a comment here, and rename that method to DoSomething.") - They also quickly identify people who shouldn't be part of the team. Everyone sees that someone moves slowly in a pull request, or that someone resists cleaning up sloppy code. The thing with people who resist cleaning up their sloppy code is that it shows to management. This happens when everyone else in the team gets their pull requests merged quickly, and one person's pull requests pile up with lots of minor comments.

efitz 7y ago
> It’s common sense on how to behave in a civil manner I am frequently surprised by how lacking many technical professionals are in EQ/soft skills/civility -often I observe people who apparently believe that their skill is such that they don’t need to treat their peers with respect. This isn’t anomalous; it’s widespread. [Conjecture/opinion] I believe that many people who enter software development and related fields are high on the autism scale- eg aspergers, etc., and that many such people are unaware of how badly that they’re treating others. It’s not “common sense” for many.
- wolfgke 7y ago
  > I am frequently surprised by how lacking many technical professionals are in EQ/soft skills/civility -often I observe people who apparently believe that their skill is such that they don’t need to treat their peers with respect. This isn’t anomalous; it’s widespread. I personally think that terseness is actually polite since it wastes a lot less time for both sides.

tawat987 7y ago

I disagree with the basic premise of the article that maintainers are obliged to merge code changes by default. One reason is that the maintenance burden falls on more than just the author, so there needs to be some consideration on whether it is maintainable and whether the people involved are willing and able to maintain it. Particularly if the change adds a public API or behavior, it may have to be maintained for the life of the codebase. Also, some projects have very high reliability expectations and you have to assume that code is broken until proven otherwise. I won't merge code until the author has demonstrated via tests, comments and clearly-written code that the change does as expected and doesn't introduce potential issues. The attitude in the article isn't particularly conducive to the long-term health and quality of a project, since you get a bunch of authors ramming through their changes based on their agenda and end up with an inconsistent mess that nobody wants to maintain.

m463 7y ago
I was mentally thinking about refuting your argument, but then I started wondering about systemd (which I do not like, although I do have concrete criticisms).

kazinator 7y ago

This article is complete garbage. :) We absolutely must consider the non-technical aspects of a change, and it behooves us to consider them before other aspects. Changes are often driven by new requirements. If we approve the change, we are approving the adoption of those requirements. There isn't always a technical reason for such an adoption other than "someone wants it that way". Based on this article, I mustn't reject, for instance, GPL-ed code being added to a BSD-licensed program. There is no technical argument against it. We can just switch everything to the GPL license and march forward; what's the problem? Can we put religion aside and just discuss the code? There also no technical reason why a compiler shouldn't have a --tetris option for playing a little game on the text console. No wait, I didn't say anything about text; we can just link in SDL and have it graphical! Anyway, can we just discuss the best way to do this, and not emotional debates about whether or not to do it? So, "someone wants it that way"? Whooptee doo, so effin' what? Let them fork the code.

deleted 7y ago

[deleted]