66
dr-ant
6y

Reviewing coworker's code:

Me: I see you're doing a convoluted sort for every element twice to get your two lists in sync... 😐
CoWorker: Yeah. *straight face, no regrets* That's the only way to do this.
Me:... Uh... No? You can just manage one list with a simple struct and then use the the standard sort.
Coworker: Yeah sure I know. But it'll take time. We don't have time.
Me: *aghast* This is embarrassingly bad code!
Coworker: Don't worry, later on I'll use a hashmap for it. But this needs to be pushed now.
Me: *to myself, no you don't need a hashmap*
Okay, you do you but I can't back you on this. It isn't going to take a lot of time to correct it.

Next day.
Coworker: Hey can you review my code again?
Me: You've made the changes already? *in a bored tone, knowing that they wouldn't have changed shit*
Coworker: No this is a different file. Our manager agrees that we can worry about performance later.
Me: Sure. *😀🔨🔨*

Few weeks pass by:
QA: The operation takes absurdly long time to complete even with the smallest data. Ten minutes for X is unacceptable.
Me: Who would've known? ☺️

Comments
  • 17
    "No you cannot push this. It's vile. Start again!"
  • 7
    @Root trust me that was exactly what I implied. But I don't have the power to reject his pull request.
    Besides the manager supported the idea that changing the data structure now will delay the function delivery.

    This isn't the first time this kind of thing has happened. This place is rife with antipatterns and casual "coders". It's like a sewer hole where anyone can take a dump and the sewer people will gobble it up as if it was the most delicious pie ever baked.
  • 7
    @Root if you had seen the code... A loop inside a loop that sorts the lists inside another loop that traverses the list. And this atrocity is invoked every time the user changes his selection. My ass was sweating just looking at it.
  • 6
    @dr-ant Sounds like bubble sort.

    Also, if code reviews don't allow rejecting PRs, what is the point?
  • 4
    @Root it was a bubble sort made worse. List A was bubble sorting for every element of list B. To keep them in sync, the list B was getting rearranged too with every move.
  • 2
    Going to wash my brain with bleach now. Laters!
  • 4
    @Haxk20 I cannot ++ you enough. You get my plight.
    It's not that they made a mistake that hurts me the most. It's that they don't care for it.
    The manager doesn't care as long as anyone higher up doesn't care.
    I'm the only one seething inside.😞
    Even with the big report they'll be like "Yeah we knew. But we had targets to meet." It hurts me that such behaviour is easily justified (even rewarded). No one will question anything. Despite the bloody fact, clear as day, that it wouldn't have taken more than an hour to write it properly.
  • 0
    @magicMirror add vinegar too!
  • 5
    approved with a comment: two weeks down the line customer is going to complain that the effects this atrocity has are unacceptable, but everyone here is too dumb to realize that, so whatever.
  • 4
    I - I can't even...
    Can't you respectfully decline the reviews? Your feedback is ignored anyway and you can use "we have targets to meet" all the same... And as a bonus you can stay kinda sane.
  • 0
    @Midnigh-shcode lol!
  • 0
    @VaderNT I wish I could. We're a small team and I'm not comfortable declining anything. 😅
    I'm still working on it.
  • 0
    @hash-table I don't think it's similar. There's no combining or intersecting any list in this case. Just a lot of unnecessary movements of elements.
  • 0
    @hash-table In this case no intersection is possible because the lists contain different types. They just needed to have their pointers stored in a single struct. No join is needed unless you mean a sql join.
  • 0
    @dr-ant You're taking this too personally. If you really care, do improve it on your personal time, else just fuck it and watch the company burn while you sit and smile. I prefer you do the latter.
  • 1
    LOL. Seems like the code review process is only a facade. CR doesn't mean ish when someones in a hurry to burst the bubble.
  • 0
    @IsLau that's true. It is just a facade.
  • 0
    @hash-table good ol' c++
  • 1
    @phreakyphoenix damn right. I can't just go ahead and change it. I can only change the modules that I am responsible for. So yes, latter is the only option.
    The code part isn't what that bothers me the most like I mentioned earlier. It's the lack of care.
  • 0
    @hash-table yeah 😁
  • 0
    @dr-ant What's the Big O for that atrocity? O(pow(n, n)? 😭🤮
Add Comment