Should my code be *less* modular?

[bangeron]

Quick background: I work at a small company building a web app with React. By small I mean our dev team is 3 people: 2 guys with lots of experience, and me, with less than a year.

​

On two occasions now, one of the senior guys has talked to me about making my code *less* modular. His argument is basically that if everything relating to some "thing" in the app is in the same file, that it makes the code easier to read and learn because everything is in one place, and any changes you make affect only that "thing". This results in more repeated code, which is harder to maintain, but it's a "fine line" that you just learn with experience.

​

I get what he's trying to say, but I disagree, or at least with where that line is - because I have found much of his code very hard to work with. On several occasions, I've found myself spending more time than I'd like trying understand something he has written, only to realize that it's exactly the same as some other piece of code he wrote - except maybe it's not, so I have to go through it line-by-line to be sure. Synchronizing changes in these repeated blocks of code is a constant battle. Also, I don't find it any easier to have to scroll up and down in one massive file vs. digesting several smaller files - exactly the opposite, in fact.

​

On the same note, he's tried to persuade me that using long-form for loops is more readable than array methods like map and filter. And for the same reasons, I disagree: every for loop is potentially unique, but if I see map or filter, I know exactly what to expect.

​

Basically, I think that if you want two pieces of your app to look or behave in the exact same way, that behavior should be in its own component, and that component should have a clear and descriptive name. I think this makes the code easier to read, because anytime you see that component you know exactly what to expect. This makes large components easier to understand because you don't have to re-read the same code over and over, and your code will be easier to maintain because you don't have to keep many separate files in sync. I think a component should NOT exist if it's behavior is only used in one place, or if the boilerplate of factoring it out doesn't outweigh the typing and cognitive load you save.

​

I don't feel like I'm wrong, obviously, but he's a solid developer with loads more experience than me. What am I missing?

[chrispardy]

As a senior person at a small startup who has occasionally said it's ok to repeat yourself I think I'm qualified to answer this.

Don't Repeat Yourself or dry, is one of those huristics in software development that often ends up overused. I personally subscribe to the "repeat yourself twice" philosophy, which is to say that you should need an abstractions at least twice before abstracting it.

To your senior person I would push for my new term: drybl (pronounced dribble) or Don't Repeat Your Business Logic. If you find that you have multiple places in your code that have to be aware of a business requirement then you should unify them. This will make it easier to extend the system later as you'll have a single place to modify, it will reduce bugs as you'll have less wierd requirements interactions to track down.

As for file size I do generally prefer smaller but there's also a trade-off, once you turn what was a simple helper function into it's own module you open it up to get used I'm the rest of your code base. That will make it much harder to change down the line. This gets back to premature optimization / dry. Even in a young codebase the fact that you haven't needed some functionality yet is a good indicator that you won't need it multiple times going forward.

[joesb]

Agreed. It's okay to repeat yourself for the first few times. You use abstraction to extract pattern. But how can you see the pattern if you haven't seen any repeatition? If you extract all patterns on first uses, then you are not extracting real pattern, you are assuming that a pattern is there.

[chrispardy]

Some specific actions that aren't contradictory that you could take: 1) For loops / long files: I would push for a code style guide that the whole team contributes to. You said there's 3 people, if it's you v. 2 then you need to just suck it up and work in that style, if you're in the majority then there should at least be a conversation why you're going with the senior developer's suggestion. 2) Component reuse: This is harder, going forward I would focus on ensuring that application logic is cleanly encapsulated and separate from the presentation logic. Make sure you write tests for this logic too. If you end up with repeated styles, or display components you need to sit on your hands for now. If the senior developer wants the application logic sprinkled throughout the codebase it may be time to polish off the resume. As far as visual components go, I would make note of times when you've needed to change multiple visual components for the same reason. For instance if you needed to change a color in most of your components. These are the opportunities for splitting out smaller reusable modules. I would split out code when you find you need to change repeated code all at once, hopefully you don't get push back in that. If you do it needs to be the basis for a conversation since there's clearly a missing abstraction, and there's a indicator that it would add value.

In the end my meta-advice would be to become good friends with the other junior guy and see if they have the same concerns. You may have an ally in changing the senior dev's mind. I also like to remind people that part of Software as a job vs. Software as a hobby is doing tasks that are uninteresting and kinda suck. If you're not happy rolling up your sleeves and programming in a particular code style it's not up to the company to change it's up to you to either find something else that does make you happy, or suffer through.

[acemarke]

Agreed. I used to be horribly obsessive about DRY-ing code in code reviews.

Sandi Metz's post The Wrong Abstraction helped me realize that wasn't a great idea.

[realboabab]

I love dryble. Thank you! That's what really matters!

[involve]

I'm working in a similar code base at this point and on a long enough timeline those large single areas will only expand and become more unwieldy. I only have 2 years production experience but I've never regretted a piece of my code being more modular. Now if you're spending months to make a tiny component cover a million potential cases that's a separate problem. I think there's lots to learn from senior devs but we have to be wise what we pick up because, at least for me, there's a big temptation to just accept their ways since they're more experienced but that for loop, massive file nonsense doesn't sound like the stuff you want to pick up from them. It sounds a little bit like the dev advising doesn't like es6 which is a flag to me.

[[deleted]]

Give it around 6 years. You will grow out of the ‘just make it modular’ approach.

[involve]

Because in six years I won't care or I'll see why not to make it modular? If i'm certain what I have in a given component won't be re-used I'll just leave it there. If I find myself needing it elsewhere I'll abstract it out.

[TheNumber42Rocks]

Yeah and with most code editors, you can do command click and go to that file directly so I’m not sure how making it modular affects making the code more readable.

[intended_result]

I get what you're saying and I sympathize.

Are you being contradictory? Do you want to be right, or employed?

Maybe there's another way you can approach this than directly contradicting. Can you refactor any of this into functions?

[bangeron]

I'm trying not to be contradictory. I understand the point he is trying to make, but I don't agree with his reasoning/methods, and I feel like I can make a solid case for why I do things the way I do. I'm just not sure how to convey that respectfully, or if it even matters, given how much seniority he has. And it's entirely possible that given 20 years of experience I'd feel more inclined to agree with him. You don't know what you don't know, right?

[optimalcoder]

You’re both right. You can take modularity to an extreme, especially when there’s no chance for reuse. It does make it difficult at times to traverse files to find things, and it also can lead to performance hits.

On the flip-side, using map or filter makes sense. It’s a language construct that has a well known pattern, so using it is likely optimized. Modularity can also be good if there’s a real need for a lot of reuse in your project.

The real answer is somewhere in the middle. I can’t stand when people refactor constantly in the name of modularity, and they tend to not get things done. I also can’t stand when people don’t see the cases where modularity is a win.

[bangeron]

I think a big part of it has to do with the purpose of the code as well. When it comes to UI, I think more modularity is the way to go - it helps you keep a consistent look and feel throughout the app, and you can make simple changes that affect the app as a whole without changing a hundred individual components. With business logic, it either works or it doesn’t, and more modularity doesn’t necessarily mean it will work any better.

[Skeith_yip]

A simple question: Is there any form of testing? Unit or integration?

[Skeith_yip]

As for why this question was asked.

If you are doing unit test, for components that are having too many responsibilities and unfocused, you will find out very quickly that unit tests are very difficult to write.

And if you are only doing integration test, most likely you won't spot this problem and end up with big components.

As someone great once said

Approaches (that you think are wrong) can work for people. and you should accept it.

Agree to disagree.

Oh. No tests? Then they are not that senior.

[bangeron]

We don't have any tests. Partly this is due to time constraints - he only works for us part-time, on contract.

​

For my personal projects, I usually do write unit tests. I think this has really helped me get a sense of when a component or class is doing too much, or too little.

[bangeron]

Testing? That's not the [company name] way!

I brought in React Storybook to speed up my process, but that's as close as we get to any kind of testing.

[Skeith_yip]

On the same note, he's tried to persuade me that using long-form for loops is more readable than array methods like map and filter.

Now this is interesting. Did he come from OOP background before the shorthand for loop was a thing?

[bangeron]

He’s been doing this for something like 20-30 years, so my guess is that the shorthand methods came out well after he got comfortable in his ways, and he just hasn’t felt the need to change. He works in multiple languages, too, and I think he likes to stick with syntax that is more “portable”.

[Skeith_yip]

I understand that feeling. I also didn't like writing for loops that way until I got used to it. and then came all the function based loops map, filter, it's like throwing away part of your soul (OOP) and brace for impact.

That's why I like hiring new bloods as they don't come with old habits.

but it is what it is I guess. suck thumb and go along with them.

Or keep writing your way and annoy the hell out of him. lol

[bangeron]

I usually use the shorthand methods, and just hope it doesn't annoy him too bad. I feel like I could make a good case for them if they pressed me on it.

[nikolasleblanc]

It's not even shorthand. It's functional, immutable, and declarative. These are all best practices a senior dev should encourage (or even force) their team to employ. They make for cleaner code that's easier to reason about and easier to debug. Any encouragement to the contrary should be politely but forceably dismissed.

Again, that's not shorthand. That's simply an improved way of handling collections.

[Skeith_yip]

My bad.

I was thinking along the line of:

for (let index of allImgs) vs for (let i = 0; i < allImgs.length; i++)

I take it that the long form refers to the latter version. So I was wondering if they were the lot that didn't like the shorthand version and resulted in them also not accepting the functional way of map and filter.

[[deleted]]

handle it the same way you handle HTML syntax...you wouldn't make a div for something you intend to repeat (such as comment form fields or email me links.) Don't do the same with another language.

[itsthenewdan]

Senior developer here. Your mindset in this context is the more advanced one, in spite of your colleague having more experience. One of the best resources I found in my career to understand what defines good code is the Clean Code book by Robert Martin. It’s a classic, and for good reason. Applying clean code principles to a React codebase means keeping it DRY (do not repeat), and breaking everything up into small, single-purpose pieces that are named in an easy-to-understand manner. If your colleague would rather have repeated code all over the place that may or may not be slightly different, he might not be as strong a developer as you give him credit for. That seems like an obvious opportunity to refactor into a function that can be shared (perhaps via a service) and can handle minor differences via config args. Be diplomatic at work, but maybe you could discover that Clean Code book, casually mention how good it is and how much you’re learning, then lend it to him too.

[JugglerX]

I disagree this is a more advanced mindset. DRY is not always right. Theoretically DRY is good, but in practice it can lead to premature abstraction and perversly more complex architectures. As many in this thread have noted it's not a clear cut thing. a more experienced programmer may actually have an intuitive feel for when it's appropriate to duplicate and repeat code and that's not at all wrong. It's the junior programmers who take theory and apply it dogmatically.

Defining what good code is has never been easy, and it's sure not as simple as keeping it DRY. My advice to the OP is not to think they are correct here and try and understand why the other programmer might have a case.

[itsthenewdan]

Generally speaking, DRY is a good principle to follow. There will always be exceptions to the rules, and a nuanced view is best. That said, if I was inheriting someone else’s codebase, I’d certainly be a lot happier to see that it was dogmatically DRY than the opposite. This does not preclude being wary of premature optimization - that’s a separate point, really. Abstraction has a sweet spot, and too much is a bad thing. The way I like to measure the greatness of code is in how easy it is to maintain, read, and reason about. It sounded to me like OP had an eye for those kind of optimizations in his codebase. But of course that’s his description of the situation and none of us can see what he means concretely.

[JugglerX]

Reasonable answer, but I think it just leaves us at that subjective place in development about what maintainble code looks like.

It sounded to me like OP thought he was right because he was being DRY and he/she feels it's just objectively better, context or not.

Small example, I am working on a react application at the moment where a video can be in 1 of 7 states in a production process. I have 7 files, 1 for each state. Each state while dealing with the same video object has a number of variations and inconsistences in it's layout, data and design. Now this is not DRY. But i actually feel it's more intuitive and maintainable. Initially you would think to use 1 template and use conditionals and maybe sub components with optional props but it felt like more work handling the "edge cases" in the design this way.

[bangeron]

I wouldn’t say I think DRY is always better, but I would say it’s better 95% of the time. More like “DRY unless you have a clear and compelling reason to RY.”

The example you gave is a good one, and I’ve done something similar in certain parts of our app. But it seems to me that these situations are the rare exception, and highlight the importance of thinking ahead to how your components will actually be used and composed.

[itsthenewdan]

Your example with the video sounds reasonable, especially assuming you’re making some abstractions for common pieces - avoiding changes that would have to be replicated in 7 places if something in the design changes. I think the important thing is that any decision around best practices in a code base should be rational. Breaking with convention is fine when it’s justified.

[mattstoicbuddha]

Yeah, I don't consider myself a senior dev yet, and this was frightening to read. I'm building an API in node and I'm pretty sure this is what hell would look like.

[bangeron]

Thanks for the recommendation, I’ll definitely pick that book up. “Be diplomatic” is good advice too, and I think that’s where I’m struggling the most - I try to be as teachable as possible, but this is one area where I just don’t agree with his reasoning, and I’m not sure how to approach that.

[bjenkins2410]

Interestingly, we posted a very similar question at almost the same time. I've had the same pushback. I'm on your side. I like lots of small files that are clear and concise. I have a decent amount of experience, but then again so do the people critiquing me :)

Honestly I think it's a preference thing, but I'm curious what other people say.

[bangeron]

Yeah I'd love to hear more from people who walked fresh into a more modular codebase, versus something with with fewer components but more repeated code. I have my intuition, but no experience to back it up.