Hacker News Reader

> Also, changing the slightest things required to go through 5-6 files.

That is a sign it was incorrectly modularised. Good modularisation means high cohesion and low coupling. Having to make changes in multiple files means the opposite.

Don't rack on modularisation (great concept!) when your only experiences are of bad implementations of it. (Which is not surprising, because most people do get it wrong.)

Maybe the takeaway is "better not do it at all than do it incorrectly."

More concretely, the code you describe seems to have suffered from the awful common implementation of the model-view-controller pattern, where module boundaries are based on technical implementation details (routes are their own module, pages their own, controllers their own, etc.) This looks "clean" when you are inexperienced, but it is contrary to all good modularisation.

Module boundaries should be based on business domain concepts, not implementation details.

That sounds like myriads of programmers have done it wrong for decades. Can you elaborate on this and maybe add a few sources?

The ultimate source is David Parnas' 1970s' articles On the Criteria to be Used in Decomposing Systems Into Modules, and Designing for Ease of Extension and Contraction by the same author. The 1960s' NATO conferences on software engineering also touch on this.

These articles essentially introduced modularization, and they argue very well for why their approach is superior to the misunderstanding we frequently see today.

There was a lot of good research into how to build software right back in the '60s and '70s that was mostly ignored/forgotten during the '90s which is when many of the mistaken patterns we see today was invented.

Not much of an elaboration, for which I'm sorry, but I think the original sources are a more efficient use of time than my retelling, for anyone curious.

Edit: I can say one thing in different words than the original papers: one thing you want from your modules is that their interfaces are stable. (This is what's known as "information hiding", i.e. the modules themselves can change as much as you want, but since their interface is stable those changes don't propagate to other parts of the code.)

If you define a module for your routes, then the interface of that module will have to change for literally anything you add or remove to the site. This is the opposite of a stable interface.

Figuring out how to design a stable interface is tricky -- this is why good engineers are worth so much -- but one rule of thumb is to base modules on business domain concepts. The business domain has likely existed for longer than your application, and it's concepts will likely outlive your application. They tend to be stable, compared to whatever implementation details seem reasonable today.

Thanks a lot for response and sources. Will look at them now.

If you have to change in 5 places to change one "thing", the code is probably not DRY.

You have 5 places in code where you draw a blue rectangle:

    drawRectangle("blue", x0, y0, x1, y1);

Do you refactor them into drawBlueRectangle(x0, y0, x1, y1)?

It seems you removed the duplication but you didn't. Because if you now have the requirement to draw red rectangles instead you surely won't leave it as

    def drawBlueRectangle(x0, y0, x1, y1):
        drawRectangle("red", x0, y0, x1, y1)

So instead of changing 5 places you now have to change the invocation in 5 places and implementation in 1 place.

You can argue it's because you named it wrongly, it should be "drawWhateverThingTheyHaveInCommonRectangle" instead, for example drawHighlightingRectangle. And you'll be right.

But you don't know if they will have that thing in common forever. And splitting the code is harder than refactoring it, so it often leads to code like this:

     def drawHighlightingRectangle(someDecidingFactor1, someDecidingFactor2, x0, y0, x1, y1):
         if someDecidingFactor == something && someDecidingFactor2 != somethingElse:
            drawRectangle("red", x0, y0, x1, y1)
         elif someDecidingFactor2 == somethingElse:
            drawRectangle("blue", x0, y0, x1, y1)

This code may seems ok, but it tends to grow business logic inside, and you don't immediately know what combinations of deciding factors are actually possible without looking at all the invocations. So either you look at all the invocations before implementing your change (and then the refactor didn't actually save you any work - what does it matter if you look at code and change it vs just look at code and change stuff elsewhere), or you ignore the invocations and add your change in isolation (probably writing code that is redundant and overcomplicated because you handle cases that cannot happen).

This is obviously oversimplified example, but I've done these exact mistakes several times :)

I think you should ask yourselves why you are drawing blue rectangles. What is their purpose? Do they just happen to be blue? Or are they blue because they have they all "do the same job"? Maybe you should have

drawInlineHelpBox(x0, y0, x1, y1)

or

drawEnergyShield(x0, y0, x1, y1)

We can see from the language that unlike your example this is a true abstraction. You went from color parameter to a specific color. It's both phrased in terms of colors.

But here we go from color to ui elements. The terminology is completely different.

     def drawEnergyShield(shield_capacity, incoming_dps, x0, y0, x1, y1):
         hue = incoming_dps / shield_capacity
         alpha = shield_capacity / 200
         drawRectangle(hsva_color(hue, 1, 1, alpha), x0, y0, x1, y1)

And I think you should embrace having business logic in there.

Yeah, the idea of DRY is simple, but it doesn't change the fact that you have to use your judgement, do and learn from mistakes etc. Nothing ever does.

I'd tweak your code example to this:

     def drawHighlightingRectangle(someDecidingFactor1, someDecidingFactor2, x0, y0, x1, y1):
         def highlight_color(factor1, factor2):
             if factor1 == something && factor2 != somethingElse:
                 return "red"
             else:
                 return "blue"

         color = highlight_color(someDecidingFactor1, someDecidingFactor2)
         drawRectangle(color, x0, y0, x1, y1)

This doesn't Repeat the drawRectangle() call.

Maybe

    drawColouredRectangle("red", x0, y0, x1, y1)

    decideHighlightColour(someDecidingFactor1, someDecidingFactor2)

then call separately? Depends on the number of calls to drawHighlightingRectangle I guess.

Something I've learned recently is to try to think in terms of behaviours, then compose them instead of mixing them together. Also it's easier to juggle the pieces by erring on the side of decomposition initially, then recompose a bit once they're organised, but only if it makes more sense.

DRY is not very useful as a concept. It doesn't define any rules how to identify bad repeated code, nor how to change it and doesn't define limitations of the concept or it's application.

I know this sounds wild and harsh but we have to overcome the habit of explaining non trivial or even unsolved problems with truisms.

Yeah, it really only points out where you have a problem.

The solution is still up to your expertise. Including deciding some repetition in OK.

Still, being able to see these design problems has been immensely valuable in my work!

In my last interdisciplinary role, I tried to simplify this as "you drew circles around the wrong parts". If many lines still cross the module boundary...

I would question your assumption that "clean code" that you describe is really modularization and abstraction. (In particular, separating everything into small scattered pieces, which is IMHO a horrible Java habit that I have to endure now.)

I mean, both modules and abstractions should have purpose.

The purpose of modules is to "do one task well" so to speak. So if for a typical change you need to modify several of them, you're not modularizing it right.

The purpose of abstraction is to provide another "language" which lets you forget certain technical details, while clarifying the bigger picture. Again, unless this language has been designed wrong, you shouldn't have to touch the abstraction.

Addendum: Of course, there is also the classic https://gbracha.blogspot.com/2011/06/types-are-anti-modular.... (and we can consider types to be a form of abstraction), so to some extend these things go against one another.

> Very clean. Also, changing the slightest things required to go through 5-6 files.

Martin Fowler's "Refactoring" is a great book (I only know the 2nd edition from 2019). From a naive understanding of the "clean code" school of thought one might assume that splitting up everything into small functions, classes, modules is always the way to go. But Fowler's advice is much more nuanced than that. His list of bad code smells includes the aptly named "Shotgun Surgery", which describes just your quoted situation. The suggested way to go is then to first inline all the scattered stuff, next to extract parts such that the logic is more contained.

Your rewrite sounds very similar. And of course you are right: School and books probably don't do a good job of transferring this knowledge. That "Shotgun Surgery" paragraph is easy to discard by a reader who hasn't experienced the pain themselves.

I remember coming across Refactoring after having already adopted it for some years. I wish I had come across it sooner. But with my initial experience under my belt I was able to realize what a gold mine it is. Anyone working in a higher level OOP language especially should give it a read

”You aren’t gonna need it” (YAGNI)[1] is good to keep in mind.

When you are building the abstractions too early you are guessing the future needs. If you never need to change X then the code to make that change possible is just extra weight.

[1] https://en.m.wikipedia.org/wiki/You_aren%27t_gonna_need_it

"Each class in his own file...."

I think there's a subtle trap where programmers think their code isn't nicely organized unless they themselves have written all those abstraction layers.

But one of the virtues of picking up a framework is precisely that it's already there for you. Hopefully it matches what you need. But within reason, if you pick a good framework for your task, you don't then need to layer on another set of abstractions... it's already got them!

It's completely sensible to pick up a framework and bash out 2000 lines of code that simply spends the abstraction budget of the core framework. It's half the value of the framework in the first place. Obviously if you're going to build another 100K of lines on the framework you may need to bring some additional organization to the party, but the space of "framework + 2K lines of code" is a pretty rich one.

> Very clean. Also, changing the slightest things required to go through 5-6 files.

No, it was not clean at all!

Sandy Metz' book "99 bottles" (there was a second edition recently, now you can choose between beer and milk and between Ruby and Javascript) tries to hammer those points in. I'd say it basically dedicated to the topic. You have one problem and explore differnt ways to approach it with pros and cons revealed and discussed along the way. And "do not rush with an abstraction" is something that Sandy tries to get accross. The wrong abstraction will cost you more in a long run than some code duplication. Highly recommended.

Just as a counterexample to this, my last gig (I got out after 4 months thankfully) was written with some of these "do copy and paste" and "write a big lump of code" principle and it was an absolute nightmare to work with. Most functions were hundreds of lines long and files were huge. The code had no architecture and was very hard to reason with or modify. Most of the work was to fix a steady inflow of bugs which had no concrete repro steps and happened sporadicly.

People rag on premature optimization but I will take it over no optimization at all. Someone who prematurely optimizes at least has good intentions in their heart and is trying to do better.

At the end of the day, every day, even every hour as a developer you constantly have a choice - be lazy or be good. If you are lazy you will add one more if then statement to the already huge function. If you are good, you will refactor it. Its OK to be lazy every once in a while, but thats a very slippery slope and years of laziness will crush a codebase in its own eight.

Having too much abstraction is a problem.

Having too little is also a problem.

There is also the case where even a well-abstracted codebase will suffer because of the particular feature requested. Even the best codebases will need changes in 5-6 files, if not more, if the feature doesn't fit with the rest of the system. It is trivial for stakeholders to request something that doesn't fit with the rest of the system, where one can then declare the system 'poorly abstracted'.

> Loccount on my "ugly, not modular" backend[...] > > all SLOC=2284 (100.00%) LLOC=0 in 13 files > > Loccount on the "very clean" backend of the previous contractor: > > all SLOC=12507 (100.00%) LLOC=0 in 262 files

This begs the question: so what?

Comparison will only make sense when another pour soul will try and change something on the likely mess you look proud to have created. What you found was not well maintainable, but will work be?

It is arguable whether 2x LOC reduction is worth it. But 5x LOC reduction on the same technology stack kind of changes the complexity category of the code base. 2kLOC (borderline trivial application) is almost always easier to maintain than 12kLOC (small application) — as 60kLOC would be easier to maintain than 12kLOC.

This is valid, of course, only for comparable code, not taking into account obfuscation, complete removal of tests, etc.

This isn't a modularization thing; it's a JS/TS thing.

The real lesson is to never work in webdev.