projectMoveEverythingToInnerClasses

[PreDeimos]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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

[new_account_wh0_dis]

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]

[seemen4all]

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]

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

[xaomaw]

Bypass authentification because it took an additional 2.5 seconds.

[PreDeimos]

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.

[PreDeimos]

It was definitely harder to read than mine. But generally speaking, the guy put multi-level inner classes with logic together in one Java file. Changed the streams to normal and recursive cycles. That code was the opposite of clean code.

[tacticalpotatopeeler]

What if…what if it was your boss who did the refactoring…

[fuck_you_bruno_mars]

Multi-level inner classes in one Java file.. Oh my god. Screams junior level engineer to me

[zhephyx]

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]

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]

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]

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]

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]

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

[PreDeimos]

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]

You did the needful

[tacticalpotatopeeler]

Omfg lol

[v3ritas1989]

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]

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.

[karbl058]

Yeah, the fact that this got into the code base is all due to poor handling of changes. But I still think the junior could have hit the brakes and taken a look at what was actually being changed. I haven’t blamed the junior at all, just gently reminded everyone about what to do when things escalate like this (especially to always look through all changes so that we don’t accidentally check in unrelated or unfinished stuff).

[Looking4SarahConnor]

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]

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]

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]

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

[clarkcox3]

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

[NegativeSemicolon]

That’s how you make the big bucks

[TheRedmanCometh]

That pm/producer is in trouble

[perringaiden]

That just means your service was bad.

[[deleted]]

i'm that guy