[projectileboy]

I find myself somewhat reluctantly agreeing. While I think the SOLID principles and similar things are helpful tools for thinking, I think they should in general take a back seat to the simple principle of Less Code. I’ve seen so many over-complicated code nightmares that were created in the name of “separation of concerns”, or some such, that my spine tingles anytime someone starts talking about some abstract design principles.

[lowercased]

> were created in the name of “separation of concerns”

when I hear "separation of concerns", I now immediately ask (internally or externally) "do we need to be concerned about this at all? right now or even years in the future?"

Recent project: untangling something that was written 2 years ago as "best enterprise engineering practices". That dude is gone, no one else on the team understands it because it's insanely over-engineered. This is literally "send a notification email", and there's about 38 core files, another 25 'helper' files, ~20 templates and a custom template parser/nested template thing, and a few dozen node/npm dependencies. This is a 'microservice' running on lambda. literally, someone needs to be able to send a notification that "X is ready", and it's this behemoth that no one understands. They do understand that all the templates and logic were taken away from each caller's context - so all the various teams that used to be in control of their own notification messaging now have to wait for someone to dig in to this other codebase which is basically unmaintained now.

It's got its own custom date parser, because... why not? And there have been random "wrong dates" being sent out for months because... no one can quite trace down the root problem. I finally did, and I'm not saying "no one ever can", but it's under no one's direct ownership, and the people that have tried to take it over have just been overwhelmed with trying to trace through it, make it testable, etc.

And last week I heard rumblings that another team is "rebuilding notifications".

It's as much a failure of management/oversight as it is "overengineering" in general, but at each review step (so I was told), things "looked good", in isolation. No one quite pieced together that this was an unwieldy behemoth until after this guy was gone. :/ (no doubt an extremely typical story, but one which, by now, I would have hoped would be 'less typical').

[mdpopescu]

Beware Chesterton's fence. [1]

Is the network always reliable? Are the messages supposed to be at-most-once, at-least-once, or exactly-once? What happens if power is off when the service is supposed to send the email, and then it comes back: should the email be sent anyway? What if there are five thousand emails waiting to be sent? What if "X is not ready" at the moment, but there's a message waiting to be sent?

A lot of that complication might be because, like Spolsky says [2], it "fixes that bug that Nancy had when she tried to install the thing on a computer that didn’t have Internet Explorer".

[1] https://abovethelaw.com/2014/01/the-fallacy-of-chestertons-f... [2] https://www.joelonsoftware.com/2000/04/06/things-you-should-...

[Maro]

One way to keep this from happening is code reviews. Don't show me a presentation, don't show me blocks of architecture, let's open Github and start looking at the code. Nothing formal, just a 10 minute walkthrough and discussion. Unfortunately in many Eng cultures the EMs stop doing this, or do it too late. In my mind this simple idea is similar to the Toyota Production System's "Go and See for Yourself (Genchi Genbutsu)".

[falsedan]

people feel bad about publishing a PR and being told "don't do it like this" and get defensive, argue about the sunk cost, throw references justifying why their approach is Good Actually, accuse the reviewer of not being serious about software engineering, etc.

The real way to project success is egoless programming.

[lowercased]

started on this project a few months back and have had PRs blocked with stuff like

"in this test file, just use one assertion, don't need to use two"

"there's too many comments here"

"i don't like the extra line breaks in this file"

"bookAdminId needs to be bookAdministratorId"

these are generally comments just on test files.

I've struggled to deal with these because... well... ego is part of it, but when there are bigger structural issues, and the focus is on - imo - minutia (without much acknowledgement that these are, indeed, minor points), it's troubling. To then be told after a few weeks that you're "not delivering" is doubly so. It's hard to take 'ego' out of this altogether, but thanks for reminder.

[falsedan]

Yeah this sucks. the egoless approach is to suck it up and do the changes, but also to raise with the team the difference between feedback for implementing the feature & expressing opinions. One thing I liked about Review Board (!) was making comments as 'Issue' (to make it clear this had to be fixed before merging) vs. not (making it just opinion/non-critical feedback). Though in a team of pedants you would find every comment being marked as an issue

[lowercased]

what's "review board"?

also, it's triply bothersome because... i've been doing this > 20 years. that certainly doesn't mean I don't make mistakes, and am never wrong, but it does get tiresome to have whitespace arguments take up time 2 decades in. On top of this, there's actually an automated linter that we all adhere to, but these are 'extra' style things that someone likes to focus on outside of the linter. It's just wasteful of the company's time, more than anything else.

[neurotrace]

Stuff like this is why I've become a huge advocate for automatic formatting tools. If you can set rules for naming conventions, line breaks, etc. then it significantly cuts down on the minutia arguments.

It doesn't fix everything, of course. Someone can still argue about using too many assertions or comments but it at least helps. If someone makes an argument about formatting then you can just say "well, Tool X formatted it this way so this is how it will be. If we think that should change in the future then let's open a different issue and discuss that."

[tincholio]

Man, I feel for you. You could be working with an ex-colleague of mine, from all you've said. I inherited _his_ codebase when he was let go, and had to re-write most of it, there was no way to fix that in a reasonable amount of time, if even at all.

But the same type of things on the PR comments, the over-architected, highly-coupled monstrosity that never quite worked.

In the end, we ended up with 6K fewer lines of code (out of a total of 16K or so), and the thing actually works now. Go figure...

[GordonS]

Code formatting tools/linters and static analysis tools can deal with the bulk of this before it even gets to review.

For the small stuff that remains, ideally the reviewer can fix it - the reviewee is probably back in flow on another task, and it's irritating to have focus broken for inconsequential things.

As a side note, I like how github implemented this as "review suggestions", where the reviewer can make small changes inline and the reviewee approves them with a button push.

[lowercased]

IIRC, code reviews were done, just... no one really saw the entire 'big picture' and how unwieldy this was until the very end.

[pjc50]

I wonder how much of this is driven by the interview culture of forcing people to write complex solutions from scratch in isolation.