Terrible code review culture

Bloomberg std::
Mar 11 48 Comments

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

Want to comment? LOG IN or SIGN UP
TOP 48 Comments
  • New / Eng pHXp44
    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 11 0
  • 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 11 3
    • 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 / Eng pHXp44
      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 11 3
    • 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 11 1
    • Bloomberg / Eng
      Fork()

      Bloomberg Eng

      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 11 0
  • Bloomberg / Eng 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 11 3
    • New qnq
      Who do you assign the ticket to? The gal/guy who pointed it out? 😂
      Mar 11
    • Bloomberg / Eng 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 / Eng bebK16
      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 11 2
    • Amazon / Eng
      yQoH82

      Amazon Eng

      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
  • 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 13 1
    • New
      EMVH32

      New

      PRE
      Amazon
      EMVH32more
      I know! And it’s a damn terrible way to develop a product.
      Mar 13
  • 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 12 0
  • Bloomberg / Eng UFEn11
    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 12 0
  • 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 11 2
    • 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

      Facebook Eng

      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 11 1
    • Oracle pzd
      Sorry I was speaking in reply to what @tqon12 said above.
      Mar 11
  • Google / Project
    toothfairy

    Google Project

    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 12 0
  • Bloomberg / Eng
    Fork()

    Bloomberg Eng

    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 11 0
  • Amazon / Eng
    yQoH82

    Amazon Eng

    PRE
    IBM
    yQoH82more
    This is hilarious!!! Condolences man!
    Mar 11 0
  • VMware yooooooolo
    Write good code in the first place
    Mar 11 0
  • 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 11 0
  • Morgan Stanley hotChicken
    Omfg 5 lol
    Mar 12 3
    • 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
  • Two Sigma zxcasdq
    LOL, love some of OP’s examples. Hope they are exaggerating.
    Mar 12 1
    • Bloomberg std::
      OP
      the sad part is I was not...
      Mar 12
  • Bloomberg / Eng
    WPslayer

    Bloomberg Eng

    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 12 1
    • Bloomberg / Eng UFEn11
      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