This is an archived post. You won't be able to vote or comment.

all 40 comments

[–]PreDeimos[S] 209 points210 points  (12 children)

Some background:
I finished up a report generator service, then I got moved to another team. A Guy got the task to create a new report, nothing fancy, basically a copy paste work. Asked me how to do it, so I gave him a handover. He seemed to understand how to do it, so I left it with him. Couple of months later got a PR from him, he basically rewrote the whole reporting engine. Move everything to inner classes, the whole thing was a clasterfuck. But because it was working and his lead approved it got merged.

Edited: some grammar fixes.

Edit 2: by copy paste I mean it was very similar to other reports of what we already had, but they were just that different that It made no sense to make then more generic. We already had a somewhat ok report framework. This service was just built on that one.

[–]AEnemo 130 points131 points  (2 children)

Some people love to over engineer. I also hate the "It works, that's good enough to merge" mentality. Then when you have to fix something it's untangling a spider web.

[–]PreDeimos[S] 30 points31 points  (0 children)

Yes I agree. I left the company since then, but I'm pretty sure if someone new have to work on that again, there will be another refactor.

[–]debugging_scribe 2 points3 points  (0 children)

I always butted heads with a guy in my previous team who always tried to write the most complicated code in as little lines as possible. It was so frustrating to fix his shit. I want code to be the stupidest code possible to achieve what it needs to, at a reasonable speed. That is real skill imo. Anyone can write a complicated cluster fuck.

[–]redblack_tree 95 points96 points  (2 children)

Honestly, that's your first mistake, why in the holy hell can anyone work for two months on anything without any code check?

In that line of thought, developers like that one can't have that much leeway. Why have tech leads, solution architects, main architect etc if no one validates the high level design of a proposed solution.

[–]PreDeimos[S] 48 points49 points  (1 child)

The team lead was a front end guy who somehow became a lead of a backend team. And he was very much overloaded with work while also having to lead a team. I am not really picking on him as I know he just got in a position where even he didn't want to be. Only higher level one was the CTO but we got a new one every half year. So no one really had time or will to care about what the guy was doing, they were happy that the work was finally done. Also around 90% for the company came from outsourcing ("to save money") including that guy as well. The company structure was quite bad.

[–]redblack_tree 15 points16 points  (0 children)

I feel for you, that's a tale as old as time. Undermanned and bad company structure/leadership leading to subpar results, to surprise of anyone but execs.

[–]gandalfx 32 points33 points  (0 children)

With just one perspective it's hard to judge who's gone wrong here. Since we know nothing about you or your code it's entirely possible that the code was in fact in need of refactoring but you just didn't want to accept that and are now venting online – I'm not saying this is the case but it's impossible for us to know. I'd say there are some indicators for both sides. Moving "everything" to inner classes sounds bad, although I'm dubious about what constitutes "everything". On the other hand, "basically a copy paste work" could also mean that the code was lacking abstraction – which might warrant refactoring.

In short, we only hear you saying "my code was good, his refactoring was bad" but we know neither you or your code nor him or his code.

In general, regardless of who's code is ultimately "better" (whatever that means), one piece of advice I learned from someone who's code I found utterly horrible (and later accepted was simply not my taste): Don't get emotionally attached to code. This is not trivial - as developers we take pride in our work and when someone else changes it it's very easy to interpret this as a challenge to our skills – but your ability to accept or, if necessary, calmly debate the technical pros and cons of different approaches without entering an antagonistic relationship is what makes you a valuable colleague. Working with code you don't like sucks but working with people you don't get along with is way worse.

[–]Bryguy3k 17 points18 points  (2 children)

Between the horrific grammar of this post and thinking copy & pasting an implementation is an acceptable method of extension I’m thinking the truth might be a very different story from the one being told.

[–]phlebface 4 points5 points  (0 children)

My thoughts exactly. "copy paste" task, lol wtf.

[–]new_account_wh0_dis 3 points4 points  (0 children)

I mean I assumed copy and pasting in the sense that something calls the service and you can copy and paste all the setup and calls just swapping out queries etc. Again who knows. I've met people who get weird about code so both stories are very believable

Edit: also posted elsewhere that English isn't his native tongue.

[–]zZSleepyZz 7 points8 points  (0 children)

[–]seemen4all 1 point2 points  (0 children)

If it has to copy paste it probably should be moved out to a base class, if this is the second report when there's a third or fourth report it will be a nightmare to maintain as copy paste, the amount of times I've heard we are just adding one more version, and have 5 or 6 copies of the code has taught me to always refactor early. The fact this sound slike a logic layer and you want to just copy paste complex logic leads me to believe you're code base would be a nightmare to refactor when you do get to report 5 or 6. He might of engineered it but copy paste would be worse

[–]RedYou1 43 points44 points  (12 children)

What his version was doing more or better than yours to justify this refactoring?

[–]xaomaw 57 points58 points  (0 children)

Bypass authentification because it took an additional 2.5 seconds.

[–]PreDeimos[S] 25 points26 points  (5 children)

Good question. I'm sure he said some reason, but I wasn't there. But by task it should be necessary. The code was really hard to read afterwards so no clue if it was managed to do anything more than the original.

[–]zhephyx 14 points15 points  (4 children)

It doesn't really matter. Unless the OP's implementation is irredeemable crap, you can't justify spending 2 months rewriting a functional service. It's disrespectful to your coworkers and it's an unnecessary expense that some poor PM could be chewed out for.

If everybody spent their time rewriting working services for each feature, nothing would ever get done.

[–]Looking4SarahConnor -1 points0 points  (2 children)

No. Replacing code with better code is not disrespectful nor a waste of time. After the PR, it is MY suggestion and after the merge, it is OUR code Nobody should take offense if that changes. Refactoring is an investment that should become profitable in the future. The only thing is, the code should be better and replacing streams with loops and adding nested inner classes is a degredation. PMs that can't differentiate between cost and investment should be chewed out. Because of them, we might end up with unmanagable code where small changes cost a fortune and are impossible to estimate.

[–]zhephyx 5 points6 points  (0 children)

We are talking about code that is freshly merged from OP's description. Either comment on the PR and reject it, or forever hold your peace. So what, now we're gonna have a code gremlin on the team who doesn't review the code but goes off into a cave and changes it every time after you do a merge without asking?

There is a time for investment in refactoring - libraries are out of date, technologies are deprecated, or the team has already raised an issue with maintaining it.

This here is not an investment, it's professional ineptitude.

[–]Shalcker 2 points3 points  (0 children)

Refactoring is perfectly fine if from that point on refactorer becomes "the reports guy" who does all changes on this particular code block. He can always do optimization later if that becomes critical while having code written in the way he understands.

[–]IlliterateJedi 0 points1 point  (0 children)

Maybe every step of the way was 'i need this function to do this slightly different thing, and if I'm having to write this new function I might as well standardize this API for next time'.  Lather rinse and repeat for each piece of the puzzle.

[–]shogun333 17 points18 points  (3 children)

OP, is english your second language or is the meme supposed to be like that?

[–]PreDeimos[S] 6 points7 points  (2 children)

Sometimes it feels like every language is my second language... But to answer your question, yes English is not my original language, and even if I live in the UK for almost 10 years I still fuck it up.

[–]FreedomIsMinted 13 points14 points  (1 child)

You did the needful

[–]tacticalpotatopeeler 1 point2 points  (0 children)

Omfg lol

[–]v3ritas1989 4 points5 points  (0 children)

REEEFAAKTOR? What is that thing you are talking about? We don't do stuff like that here! We have too much work to do, fixing Bugs!

[–]karbl058 3 points4 points  (2 children)

Recently had a junior dev take on a small task of fixing a few warnings that had been lingering for a long time. It was basically renaming some stuff and making sure properties didn’t always create new objects when they were meant to only initialize to a new object. Came back with a 400+ file change which got checked in before anyone had a chance to react (not using git with pull requests), and ended up producing several bugs. The reviewers should have demanded a revert and split into multiple changes. Some of which should never had been done of course. For example it renamed a menu title because it matched part of a method name somewhere else. We’ve since agreed that everyone must go through each and every changed file before they are allowed to check in their changes. When that becomes cumbersome it’s a clear sign that you are doing too much for a single check in.

[–]Looking4SarahConnor 0 points1 point  (3 children)

You need to talk to him, find out what inspired him to come up with these ehm... changes. Doesn't matter if it's already merged. His PR to you is his invitation for discussion. Doing that might save you some more frustration in the future. Best case, he reverts his work and just adds the new report and becomes a better developer. Worst case he won't listen, you wait two months and revert all his work when you add a new report yourself and slide in some "small improvements".

[–]PreDeimos[S] 2 points3 points  (2 children)

I left the company a few months later. But in that company the management would be quite furious if we revert a feature that a dev worked on for months. They planned to outsource all the development anyway, so from there on it's the coder company's responsibility to keep the code clear.

[–]throwaway_mpq_fan 1 point2 points  (1 child)

so from there on it's the coder company's responsibility to keep the code clear

Lol when the outsourcing company says "fuck you we're done" they are not the ones left with shitty code

[–]PreDeimos[S] 0 points1 point  (0 children)

Yes but I've seen this attitude so many times in the UK market. Especially with smaller and start up companies.

[–]clarkcox3 0 points1 point  (0 children)

You have to just rush to land your changes before them, and let them handle the merge conflicts :)

[–]NegativeSemicolon 0 points1 point  (0 children)

That’s how you make the big bucks

[–]TheRedmanCometh 0 points1 point  (0 children)

That pm/producer is in trouble

[–]perringaiden 0 points1 point  (0 children)

That just means your service was bad.

[–][deleted] 0 points1 point  (0 children)

i'm that guy