Office Life

Terrible code review culture

Bloomberg std::
Mar 11

Have you ever had to deal with code reviewers that have absolutely no idea how to do it and instead use the chance to go on an ego trip?

My company has no formal code review process, just that it has to be "reviewed" by somebody, which makes it very political and bureaucratic to get some code in. "Reviewers" which are your peers would block your changeset for days on complete nonsense like:

1. "your code is not wrong, but this is how I would do it, can you please change?"

2. "I don't like this variable named "statusCode", can you name it "statusCodeInt"?"

3. "I know this is not your code but can you also make some changes to it as well?"

4. "passing int by value is too slow, can you pass it by reference?"

5. "I see you need to return a pointer, can you take a double pointer and make it a void function?"

6. "I know this is existing code but I don't understand it, can you add more comments here?"

7. "can you refactor this whole codebase while you're at it?"

8. "what if `new Object()` returns null? can you add a null check?"

If you don't do as they say then your changeset will never get merged. How should I do the needful and revert back properly in this case?

comments

Add a comment
  • New / EngpHXp44
    Have standards that are agreed upon. If people agree that integer variables should have “Int” as a suffix, make that a rule. Better yet, make it a rule that a linter can verify automatically. Same goes for returning pointer vs. double pointer or even pass by value vs. pass by reference. Don’t let things devolve into opinion debates. The specific decision is less important than whether or not you can have an agreed upon rule that everyone follows. If you do that, code reviews will be much more productive.
    Mar 110
  • Oracle pzd
    This is what I use: what you are saying makes sense, but it seems orthogonal to the problem I am trying to solve. How about we circle back to this in a later PR?
    Mar 113
    • New qnq
      If they actually care they will tell you that it will never end up getting fixed then. What do you say in that case?
      Mar 11
    • New / EngpHXp44
      You create a chore and put into the backlog for the next sprint. Change the culture on the team. If you don’t have the sway, get a more senior member of the team on your side to help present the plan.
      Mar 11
    • Microsoft TqoN12
      Tech debt
      Mar 11
  • Google hfch
    > 4. "passing int by value is too slow, can you pass it by reference?"

    Lol 😂
    Mar 113
    • Bloomberg std::
      OP
      I personally think 8 is most moronic. And yes, that guy is now a manager.
      Mar 11
    • Amazon donut-
      It has been a while since I did C++, but I recall the possibility of getting null from a new call could be worth checking for, esp if you have a valid retry strategy. But otherwise, yeah, I guess it’s pointless whether you catch and throw an exception vs having the program throw and crash.
      Mar 11
    • Snapchat gqkO66
      Maybe he wanted you to catch std::bad_alloc
      Mar 11
  • Google come2daddy
    Google has concept of readability. Only people with a language readability can do formal code reviews. And getting readability is months long process as one is expected to read many many many pages of style guide and books .

    This helps in maintaining consistency in reviews. Maybe it is time to start selling consistent coding style across Bloomberg. Easiest would be to adopt existing standards and then adapt it through a council.
    Mar 111
    • Bloomberg / Eng
      Fork()

      BloombergEng

      PRE
      Amazon
      Fork()more
      BBG officially has that as a suggested policy, complete with tools to help out. There is an internal flavor or clang-format to help out for the truly lazy.
      Mar 11
  • Rubrik PartyStick
    It’s a cultural thing. Don’t be a fracking racist.
    Mar 110
  • Bloomberg iVX372
    7) is an example of major scope creep, and should not be necessary. Simply state that it's out of scope for the ticket, and create a follow-up ticket to refactor
    Mar 113
    • New qnq
      Who do you assign the ticket to? The gal/guy who pointed it out? 😂
      Mar 11
    • Bloomberg iVX372
      No need to assign it to anyone, but you could assign it to them if they're particularly insistent.

      "Great idea! I've just assigned you this ticket to spearhead the refactoring initiative."
      Mar 11
    • Microsoft / EngbebK16
      If you can do small refactorings as you go, you can avoid big refactoring. Sometimes the ask really is out of scope, in which case a "won't fix" response is perfectly acceptable. But sometimes it's totally worth doing, "This test class is now tens of thousands of lines long with hundreds of methods. Move your new methods either to a more appropriate test class or create a new one. And then take a minute to look at other related tests and cut & paste then while you're at it."
      Mar 11
  • Mode DvVM00
    i would leave honestly. this is a broken culture and nearly impossible to fix as an IC complaining on blind.
    Mar 112
    • Amazon / Eng
      yQoH82

      AmazonEng

      PRE
      IBM
      yQoH82more
      Why leave? You can give multiple company wide talks or write blog and make people notice it. The content is so good. You have to tell people how nonsensical they are, at least next time they will think twice before giving the review comments.
      Mar 11
    • Mode DvVM00
      i hope you’re trolling
      Mar 11
  • Chase blahBumbu
    Good time start a set of code guidelines suitable for your group

    Add //TODO to address all points you can’t in the time that you need to.

    This is not a reason to leave the firm. It’s a time to man up and help fix it. It’s not a hard problem to sort out.
    Mar 120
  • Bloomberg / EngUFEn11
    Some of my excuses:
    1. Outside the scope of this PR.(PULL REQUEST).
    2. File a work item for future work, assign it to team lead to prioritize. Mark created by field to the reviewer who suggested it.
    3. Be vocal when someone is steam rolling you, and get everyone in the team to discuss. With the new beehive cubicle arrangements that's the easiest to do.

    Having said that, I have definitely seen some so called Champs going on ego trips.
    Mar 120
  • VMware nsxlife
    Good time for you to start creating the formal code review process you want for your team.
    Tell your team that code reviews are taking too long if that’s the case with everyone.
    Define the standards, document it, take inputs and share.
    Mar 112
    • Bloomberg std::
      OP
      yes it's the same for everyone. But people seem to thrive on bureaucracy here so multiple attempts to formalize the process were struck down in the past.
      Mar 11
    • Facebook / Eng
      FastFinger

      FacebookEng

      PRE
      Facebook
      FastFingermore
      Sounds like some of the companies I've been at. Try your best to change things. If you don't have the motivation to, just do what is barely enough while you look for a better job.
      Mar 11
  • Oracle pzd
    Exactly. Reframe the argument this way: this is a reasonable tech debt to take on, in order to ship the product / bug fix sooner. There must be someone in your company who wants the product to ship sooner rather than later. (It may be your boss or a project manager.) Make an ally of him / her.
    Mar 111
    • Oracle pzd
      Sorry I was speaking in reply to what @tqon12 said above.
      Mar 11
  • Google / Project
    toothfairy

    GoogleProject

    PRE
    500 Startups
    toothfairymore
    Oh that's Google 100%. Overly pedantic and subjective posturing. Kind of sad the way some people are enabled there just to get attention.
    Mar 120
  • Bloomberg / Eng
    Fork()

    BloombergEng

    PRE
    Amazon
    Fork()more
    1. Ask to explain why.
    2. If that is your team's/orgs convention, follow it
    3. Sure, why note if it's not risky. Leave the code better than you found it.
    4. Probably NBD, but probably no harm.
    5. Not sure about this one. Can someone from BDE chime in?
    6. Good way of learning. Commenting code you are unfamiliar with is a common and easy entry point.
    7. Wut?
    8. Welcome to C++.
    Mar 110
  • Amazon / Eng
    yQoH82

    AmazonEng

    PRE
    IBM
    yQoH82more
    This is hilarious!!! Condolences man!
    Mar 110
  • VMware yooooooolo
    Write good code in the first place
    Mar 110
  • Synchrony ————
    God damn. Null check on new object. Btw no. 1 is saying your code isn’t optimal in a polite way. Unless you have an argument the suggested change is not better, just do it.
    Mar 110
  • Morgan Stanley hotChicken
    Omfg 5 lol
    Mar 123
    • Mode DvVM00
      to be fair we don’t know the internal style guide. they might like to only return error values. maybe it’s part of a class where every other method does that. etc.
      Mar 13
    • Bloomberg std::
      OP
      there is no style guide, the guide is whatever people claim it to be. That's why people go out of their way and claim they are the best.
      We do have a basic style checker tool that runs as part of jenkins and it doesn't say anything on this.
      Mar 13
    • Mode DvVM00
      again i personally would leave. there’s lots of jobs. this one isn’t worth it. who wants that kind of grief?
      Mar 13
  • Bloomberg Sgvp47
    I have had so many instances like this. Reviewers want the code to be like how they would have written it. Sometimes it will take up like 3-4 weeks to get a simple code review done.

    I have given up now, i would suggest to just copy paste from the review and get it over with.
    Mar 131
    • New
      EMVH32

      New

      PRE
      Amazon
      EMVH32more
      I know! And it’s a damn terrible way to develop a product.
      Mar 13
  • Two Sigma zxcasdq
    LOL, love some of OP’s examples. Hope they are exaggerating.
    Mar 121
    • Bloomberg std::
      OP
      the sad part is I was not...
      Mar 12
  • Bloomberg / Eng
    WPslayer

    BloombergEng

    PRE
    Fidessa
    WPslayermore
    Lol @ 4.

    In my department, the cod reviews aren’t like that. I discuss strategy with teammates prior to embarking on the code change. The times I’ve been asked to change things, the changes have been constructive.

    Perhaps you should bring these concerns about code reviews up with your manager or skip levels during 1:1 with some concrete examples of unconstructive reviews on how they prevented you from being more productive.
    Mar 121
    • Bloomberg / EngUFEn11
      I have seen instances where managers themselves will tell you to be not afraid to push back during code reviews, since people tend to hold others to a higher bar, compared to themselves.

      At the same time managers/TLs will refrain from getting involved in heated debates on reviews, rather than putting a foot down and establishing standards.
      Mar 12
  • New
    EMVH32

    New

    PRE
    Amazon
    EMVH32more
    I have commented on this in other questions. Part of code review is building a skill that lets you gracefully deal with difficult people.

    You will also find that a team that “democratizes” code reviews sabotage good work. Because it only take one dissenter to prevent a pull request from being accepted.
    Mar 120
  • Two Sigma / Engurethra
    *shudder*
    Mar 120
  • Two Sigma bingshuh
    The worst I've ever had in code reviews is fundamental design disagreements between me and a reviewer; eventually those just are resolved by polling the team, going with whoever has more experience with the area, and maybe manager fiat. They are annoying but the process is necessary to produce good code.

    On the other hand, I've done code reviews where the author is super stubborn about every little change, and seems to think their code is perfect from the start, and has to be merged instantly, with no time for revisions. I dunno OP, if those examples are how you view most code review feedback, maybe you need to be a little less defensive about your code. Are you just posting to get justification for your refusal to change anything in code reviews?
    Mar 120
  • Chase fxJk28
    This is all very reasonable you should just do what’s said 😉😂
    Mar 120
  • Amazon / EngfHUg57
    The problem isn't limited to smaller companies. As prestigious as it is, Amazon has the same problem. Egos are bigger than you think. I have one team member who has double standards and argues for everything and eventually flip the argument and make you look like a fool. She doesn't add test cases at all and always creates a ticket to add test cases later, which she'll never get to.
    Mar 120
  • Amazon OTdS88
    Raise it to your manager and have them explain why their bullshit suggestions are blocking real work from getting done
    Mar 110
  • Bloomberg iVX372
    My last company had this problem. Some teams at Bloomberg are better than others in this regard.

    Definitely agree that it can be annoying when excessive. It blocks delivery and makes meeting Bloomberg's "timeliness" evaluation criterion highly political, when it shouldn't be.
    Mar 110

Download the app for more exclusive content.