Just refactored 100+ lines of madness to 5 (FIVE!) lines of code. It's pleasing me and shitting me off at the same time.

Some people still get paid per line of code, I'm convinced.

Hang on, really?
Like no joke, 100+ lines to 5?
No fucking way I genuinely cannot believe that.

If it wasn't company policy I would have gladly shown you the code. Long story short, an API endpoint should return an array of simple objects (Guid key and string value). Now, this was implemented using the CQS pattern with a query class, query handler class, some conversion and dictionary lookups, multiple interfaces with implementations before eventually returning what was simply stored as static objects in a constants class.

I changed that to just returning the objects directly in the endpoint, and you have to be pretty fanatic about patterns to opt for the original solution.

@devdiddydog
What happens when that code moves to be configurable?

Simple: you refactor it - and it will be dead easy. Implementing solutions to cater for all sorts of unknown (and often unlikely) future scenarios is a huge problem. I have seen it everywhere, and was even taught that myself back when I started out as a dev. Dangerous, dangerous, dangerous..

@devdiddydog
If the whole system is in CQRS already, then that refactor would require someone to redo the scaffolding work that you removed.

That sounds a lot like creating technical debt.

It's not a full blown event sourced CQRS system, it's using a flavour of the command / query pattern (CQS).

Either way, an endpoint for retrieving a couple of values stored as constants does not justify several levels of abstraction. I want minimal logic in my controllers and could have possibly created a query handler invoked through my dispatcher. This would have just added a few more lines of code, but I hardly consider returning an array of constants "logic" so I decided against it.

The main problem with the original code was how a simple request went through 5 classes and 9 or 10 methods to retrieve the result.

Controllers shouldn't hold any logic at all. They should be the gateway towards internal services providing the logic. That's the only sane way to do It imho.

Most likely there was an end point requirement. 😛
I've also written stuff that pissed me off because one guy using the api had ridiculous requirements.

I just can’t believe how 100+ lines of code can be reduced to 5 without breaking something.

@no-oxygen controllers in what context? In mvc, they're literally the logic of the system.

@NoMad controllers as in "api controller". I've been educated in keeping them as simple as possibile, delegating all logic to a service that should be on a one on one relationship with the controller itself. This makes the controllers stupid and stupidly simple to change later on. Over time I started to get to like this way of writing api controllers and that's what I was suggesting above...

Mind you, I'm no senior, but that's what I think

@no-oxygen API controllers are usually kept minimal also due to security 😜 just saying! It is still part of the logic tho.

@NoMad that's well said friend! 👻

@spocksocks if most of it is unnecessary abstractions its easy to grow a small code to massive size.

I have some similar solutions where a whole class is created to just check a condition. And if they had gone the whole mile and made it dynamic so that adding another class would add another condition to all the right places fine, but unfortunately its just the abstraction that is done, not any usage of it.

Every place any if the classes are used you have to manually manually run a method from the class with the right parameters, oh and the classes are static with a single method each, so no injections or helpers.

If all methods had been one liners in the same static class you could remove 90% of the lines AND have the benefit of having all conditions listed by intellisence instead of having to know which classes exists.

A clear example if someone who got half of the pattern but not the important half.

And also of someone who is not literally payed by LOC but thinks large commits with much code is more impressive.

If that is the case here I can easily see a 20 fold reduction of code size with no loss of function.

And if its old code that has been there for a while its unlikely there will suddenly be a requirement for the extra abstractions.

The real danger with unused and unnecessary abstractions is that its more code that can contain subtle errors and new developers risk miss understanding it.

Like a former colleague that found one example in js using setTimeout to avoid having the gui wait for a callback, but never really understod why it was used.

Half a year later we found nested setTimeouts 6 levels deep with no reason beyond the first one, and it was causing lots of race conditions that just went away when we dropped all but the first setTimeout in the hierarchy.

I once had to fix similar, with a function that spanned maybe 1000 lines, split down the middle by an if statement and one tiny change in the 500 line blocks. The whole thing reduced down to around 50 of lines of code.
The guy whose code I was refactoring left in a huff not long before, so I'm reasonably certain he did it to piss off our respective boss.

@luret Don't get me started on unneccessary IF statements and nesting... :|

@Voxera
The thing that usually burns me is people trashing highly distributed, late-bound code because they didn't understand the abstraction. As an example, someone broke the tenant-custom module loader in a .net app I wrote some years back. I was called to take a look because someone I knew who was still there couldn't figure it out.

It felt like he'd taken all the IDE's mindless suggestions, made the loader context builder static, and ended up serving the same context data for everything. I was a little impressed at how badly he'd fucked it, because he had even replaced the transient DI registration for the builder with a factory to serve the same static instance.

Some people really shouldn't be allowed to touch necessarily complex systems.

@SortOfTested So true yes ;)

If you check some other posts you know I had such a colleague.

He was in a way a very fast learner of the purely syntactical code but never on the thought behind it.

So whenever he learned something new he could very quickly replicate it and get things running, but he rarely understood why or how and made stupid mistakes or used solutions for all the wrong reasons and almost always used overly complex or repetitive code.

And personally I love good abstractions. But he often used abstractions as a jack of all trades, more for consistent code syntax than for consistent functionality.

So if an abstraction was used in one class within a section of the program he might use it in the next class, not because it was needed or added anything to the class but because it made the code lists look more similar :/

Combine this with an often total lack of understanding of what the abstraction was solving you got some very hard to read, bug ridden code.

Or mistakes like using the secret key to generate the session token in client side js :P, just because the example was in js (for node).

@Voxera
Paint by numbers coding is the biggest enemy to quality anymore.

@SortOfTested if it even was numbers, in this case it was more like ignoring the numbers and drawing with stamps right over them :/

100 to fucking 5. 😒😒😒 who you kidding.