65
dr-ant
84d

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
  • 19
    "No you cannot push this. It's vile. Start again!"
  • 9
    @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.
  • 8
    @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.
  • 8
    @dr-ant Sounds like bubble sort.

    Also, if code reviews don't allow rejecting PRs, what is the point?
  • 5
    @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.
  • 14
    @dr-ant My eyes want to die after reading this. LIKE WTF ? WHY ?

    Managers can be sometimes dumb AF. Yeah sure lets push that shitty code in because we need to have it done really fast. The shitty part of the code gets touched on after multiple years and changelog "HUGE improvement in speed"
  • 2
    Going to wash my brain with bleach now. Laters!
  • 5
    @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
  • 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
    I've done something similar recently. I just joined the two lists and used LINQ to do a comparison and got a combined list of all the data I needed from both lists with only the data I needed.
  • 1
    @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
    @dr-ant I just saw keeping two lists in sync. I guess it depends on how the two lists are generated. Sure if somewhere up in the code a single list can be generated there would be no need to join. In case my lists came from two separate data sources which had to be called separately then I had to intersect them. Guess seeing the code world be easier to understand the implementation.
  • 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.
  • 0
    @dr-ant what language? In C# I can store multiple types in an anomymous IEnumerable

    var updates = list.Join(customersList, l => l["CustID"], c => c?["id"]?.ToString(), (item, customerList) => new { item, customerList });

    Where list is a Dictionary<string, string> and custimersList is a <JToken>

    updates end up being IEnumerable<Dictionary<string, string>, JToken>

    Then just access via updates.item["CustID"] or updates.customerList["id"] (and any other data in either list.
  • 2
    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++
  • 0
    @dr-ant ah ok now I understand.
  • 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
  • 0
    @dr-ant What's the Big O for that atrocity? O(pow(n, n)? 😭🤮
Your Job Suck?
Get a Better Job
Add Comment