7 ms·

This is great advice and isn't followed often enough, especially when reviewing code written by people new to an organization/team: > If you see something nice

• siquick 7y ago
  > I've seen cases where people got hundreds of comments (many of them minor, nitpicky) from more experienced developers and were discouraged by the sheer number of them. That most new developers naturally suffer from imposter syndrome is not helped at all by 100% critical code reviews. This, combined with a large portion of developers lacking social empathy, poor communication skills, and (unfortunately) a desire to appear to the the smartest person in the room, can lead to severe demotivation and stress for both experienced and new team members.
  • hckr1292 7y ago
    Social empathy is a really important skill, especially when delivering feedback. However, the best way to improve as a junior developer is to welcome feedback, not panic. I've seen junior developers welcome feedback or run from it, and I think that has as much to do with the reviewer as the author of the code. On the flip side, I've learned almost as much from reviewing more senior engineer's code and just asking dumb questions or looking up functions in documentation that I didn't know about. I think there's huge potential to learn as the code reviewer.
    • siquick 7y ago
      I'm sure we've all worked in places where a senior would not like it if a junior reviewed their code, regardless of the juniors intent. But yes, always a tonne to learn. IMO the first few code reviews should be done face to face so some rapport can be built. Receiving feedback from a 'human' is far easier to process than a faceless Github profile picture.
      • dgellow 7y ago
        The face to face is definitely a good approach for first reviews. Also it’s worth starting with lower expectations and less critical feedbacks then increase over time. It takes time for people to feel comfortable receiving direct critics from persons they aren’t familiar to.
  • vonseel 7y ago
    > hundreds of comments (many of them minor, nitpicky) This is one reason I liked Phabricator’s review system, which allowed drafting comments on an entire PR before submitting the comments. This allows you to be as nit-picky as you want when reading the PR, and then delete or modify any of them at the end. Instant submission of line-level comments, on the other hand... I think reviewers should have the awareness to understand not to over-burden the person whose code they are reviewing. For example, if I’m reading some very junior code and finding lots of issues, maybe I should delete all my comments about minor things like style and only leave the comments requesting major fixes like code design or potential bugs? When the developer re-submits the code after fixing the main issues, maybe it will be less frustrating to get a review back with “this looks great, can you just fix these small issues that don’t conform to our style guide and it’s good to go.”?
    • pitaj 7y ago
      GitHub features the same draft system you described.
      • vonseel 7y ago
        I thought theirs was immediate, but I was referring mostly to Bitbucket Server's system from previous experience with that.
        • dgellow 7y ago
          GitHub has both. You can add an immediate comment, or add it to a draft that you post as one unit.
    • tbodt 7y ago
      Critique and Gerrit (code review systems at Google) also do this.
  • rajangdavis 7y ago
    100's of comments should raise a red flag. I think there are 1 of 3 scenarios at play: 1. The commit/change is too large. 2. The reviewer is nit-picky. 3. The person that wrote the code made a lot of mistakes. I really feel like if a reviewer is making 100 comments on a change, they're doing something wrong even if there are a lot of mistakes. That reviewer should really reach out to the person that wrote the code and talk to them human to human.
    • LandR 7y ago
      As someone who did a code review for a colleague and left over 100 comments... I would say that leaving those comments is handy as it's a single place I can check to see what comments I left and the state of the fix, or if it's not fixed yet. If I just ended up talking to the guy, I'd have no record and he would have no record (unless he took notes) of all the places he has to fix. The reason for 100+ comments was a junior developers code and far too large a changeset.
      • rajangdavis 7y ago
        I feel as though leaving 100 comments only addresses the code but not the coder. Telling someone that the changes they made are too much for a commit would probably go a lot farther. Identifying common architectural problems would also reduce what you need to connunicate. Maybe they made a lot of mistakes but I would find it difficult to believe there were 100 unique mistakes.
• mieseratte 7y ago
  Why is it that people feel so discouraged by loads of review, especially early on? I always had a good bit of imposter syndrome early on, but never considered quitting. I always assumed that you have a lot to learn, that it’s expected you’re going to suck at some level.
  • compiler-guy 7y ago
    People have their own internal stories about how good they are, and all that feedback can be very painful. It doesn't help that many comments are nitpicks and pedantry, made by people without much social empathy. This is especially true when sending a patch for a high-level review of your proof of concept, and the next thing you know people are complaining about your formatting.
    • hckr1292 7y ago
      The best critical code reviews I've had focused on the design and completely ignored things like formatting. On balance, formatting is an important part of code health. Hopefully, using auto-formating tools (gofmt and friends) should hopefully make code formatting issues not take up time in code review.
    • Arnavion 7y ago
      It can be hard to review badly formatted code. The reviewer has to put in effort to understand your change, but they have to put in more effort to parse the change if the formatting is bad. "Why is this function's return value not being checked? Oh, turns out this person used a yoda conditional even though the rest of the code doesn't do that." "Why is this block of code running even when the condition is false? Oh, turns out the else branch already ended and this part of the code is just indented wrong." And so on. Even something as minor as `foo ()` vs `foo()` can stand out and act as a constant stream of mental speedbumps. Claiming your change is high-level / only looking for feedback to the overall design doesn't change that. You're asking for a code review because you want the reviewer to read your code, but reading it is exactly the part that they're finding hard to do. And it's not excusable, but the reviewer might be insulted that the reviewee is wasting their time and their feedback may be ruder / snappier as a result. After all, the reviewee could easily have put in the effort to run the auto-formatter, follow the existing code's style, etc.
• ereyes01 7y ago
  I once worked on a team that specialized in very long littanies of code review comments... but they were able to bake this into their culture in fundamental ways such that it ended up being one of the most positive experiences in my software engineering career. The basics of how they accomplished this was: - The obvious- no personal / destructive attacks or insults, no cussing, no comments on any person's abilities. - Having your code picked apart is a badge of honor, and you were expected / required to do the same to the most senior of your teammates when they submit code. - Collective ownership- it's never "your" code, it's the team's code. - Constant acknowledgement that our system is hard and complex, and it is really important that it works as expected / promised. That last part was an ingredient I never found anywhere else. The opposite seems to be more common- every other team I've worked on tended to underestimate or even trivialize the difficulty and complexity of the systems they work on. By acknowledging that this work is hard for everyone involved, and that despite this, it must be well-done and function properly, the code review and the ensuing discussions became a very welcome and encouraged part of this process. It also helped defeat newcomers' imposter syndrome because this mentality was effective at making everyone feel like they had an important role to play, and that even the most senior folks often felt like a noob when they screwed up.
  • sailfast 7y ago
    Sure, but “Hey I really appreciated how you did this thing here because it’s tidy / does thing X really well / takes into account future whatever” never hurts to throw in either!
    • ereyes01 7y ago
      Yup, I failed to emphasize how much I agree with that as well!
  • throwamay1241 7y ago
    I found that really informal comments cut less deep 'hey man this needs double checking, read up on $x and then reconsider this block' (gender aside) is far better received than 'This is missing fundamental concepts around $x read up on them then rewrite this block'
• dahart 7y ago
  Agreed. I find this extra important when working remotely or with other people who work remotely. If you’re not getting any face-time, critique has to be balanced with positivity. In my experience it’s true even with very experienced team members, and even with the very serious & stodgy people who avoid chit-chat and claim to not be bothered by feelings & opinions.
• Ididntdothis 7y ago
  Agreed. You don’t need fake praise but you should know whether something you did is just adequate or really good. You rarely have discussions about why something is good.
• civicsquid 7y ago
  Something I've started doing that I picked up was prefixing my nitpicky comments with "Nit: ..." so that it's clear that certain comments are just minor suggestions, not that anything is necessarily wrong. I'm usually okay with preemptively accepting code with only nit comments too just to signal that those comments are not too big of an issue (if at all).
  • sailfast 7y ago
    The guide goes into this as well with the “LGTM/Comments” approval flow. It is something we’ve done organically but nice to see written up as well.
  • daxelrod 7y ago
    My favorite way to do that has been to indicate “Not a blocker” to differentiate nits that I’m picking from things that would make me not approve the review.
• amxpd 7y ago
  At Amazon, all code is reviewed. Comments are constructive and straight to the point. Standard courtesy applies, but anything more than that is left out. Unless there's something extremely interesting, overly cautious phrasing and "positive" comments are mostly noise. Also, people tend to specify when comments are nits.
  • sanderjd 7y ago
    Why the scare quotes around "positive"? I have lots of positive thoughts when doing code reviews. I don't write them into comments all that often, but sometimes I do. "Oh I didn't know about this API, nice find!" "Ah, nice approach, this is a big improvement." "Thanks for improving the test coverage!" Maybe this seems like unactionable "noise", but it isn't, it encourages future actions of the kind being positively reinforced. Plus it's free, it takes no time to write and no time to read comments like this, all it does is make the author's experience of reading the review a bit more pleasant. This is all upside, no downside. It would be crazy to discourage such a thing.
    • aledalgrande 7y ago
      Agree, I love to point out a nice approach or a TIL, and I like that Google put it down in writing. If Amazon really considers it noise, it is very descriptive of their culture.
    • UK-Al05 7y ago
      Depends how it's come across. If your not careful it can come across as patronising.
      • n_ 7y ago
        Full disclosure, I work at Google, and I actually find positive comments mostly rare, but code review culture is very different team to team, so I can hardly speak for the entire company. Personally I try to make them often, but I also try to be thoughtful about making them as to not come off patronizing like you mentioned. Though I’ve never felt like a positive comment ever made me feel patronized in any situation, I think a good way to not come off patronizing is to to make the comment more personal than general, ie: “This is a cool way to use the rest operator (or whatever thing), Ima steal this”. Obviously it needs to be an authentic comment, but I think it’s a significant difference vs “Great use of the rest operator!” When the interpretation on the other end might be “oh so this person is surprised I know these basic language constructs”. I learn things from doing code reviews all the time, and I like letting my coworkers know that I feel like I’ve gotten better at my job as a result of reviewing their contributions. I think it’d be a shame if I didn’t let them know.
• kyrra 7y ago
  Want to know a great way to handle a subset of these? Good automated code linters. Google has a ton of formatters and linters that either auto correct or suggest changes. To paraphrase a smarter Googler than me: People tend to just do what the bots say and don't care too much about. The best way to impose your code ideas on others is via a linter. Automated tools here have also drastically reduced the number of comments I get on CLs at Google, as people don't bicker about the little things as much anymore.
• matwood 7y ago
  > I've seen cases where people got hundreds of comments (many of them minor, nitpicky) from more experienced developers and were discouraged by the sheer number of them. Unfortunately many people stink at communication. The best thing a reviewer can do during a code review is ask questions. A reviewer who comes in and just says 'this is wrong' misses an opportunity to either learn themselves or actually teach the submitter. A much better approach is to ask the submitter why they did something certain way. What were their thoughts, and what did they see. IME, asking questions leads to much better outcomes. Sometimes the submitter will just admit they were not thinking about anything and see their own errors. Upside, they found the issue themselves. Other times they'll explain some complexity or use case only they could see while deep in the code. Upside, I just learned something. In general the best teachers ask questions. It doesn't put people's egos on the defensive, and tends to help people learn more effectively (the goal if any review IMO). I've seen this work on me in other scenarios like Jiu-Jitsu. My teacher will ask mid-roll or afterwards why I did something, no matter how stupid it was. Explaining my thought process helps me to better recognize the error, and I'm not immediately on the defensive. I'm then much more receptive when the teacher goes "that wasn't a terrible idea, but if you did this...".
• patthebunny 7y ago
  I did a pretty reasonable chunk of C++ code once years ago. I was junior level. Wanted a good review. I got 21 comments to remove blank lines and none about the code. Thanks. Really useful. This was the team of "experts" on the code base. Then a principle engineer reviewed some other code and oh look, actual useful comments. Nitpicking can be worse than useless. Obfuscates or ignore real issues in the code. Seems to usually be a sypmtom of the reviewing not being competent enough to actually review the code properly.
  • mytailorisrich 7y ago
    Sometimes people don't want to actually review but feel they have to leave comments. Trivial comments are a sign of that.