DEV Community

Discussion on: How would you refactor this JS function?

Collapse
 
lgrammel profile image
Lars Grammel

Love it!

I'm a fan of simplicity so I ended up with the first solution you proposed (plus inlined hasLine), but I very much agree on your thoughts regarding null / undefined being passed in.

For your 3rd solution I was wondering where the line towards overengineering is, I think it depends very much on the project and it's context.

Collapse
 
nombrekeff profile image
Keff

Great point on overengineering, I must admit the last solution could be a bit overengineered. That's the reason I would need to check the full code base and see the real use of this function.

I too prefer simplicity in general, so I would probably go for the first example.

Collapse
 
nombrekeff profile image
Keff

On the point of the hasLine variable, it's just my personal preference, I really like how readable it makes the conditions.

Thread Thread
 
lgrammel profile image
Lars Grammel

It definitely makes the code more understandable.

Collapse
 
ashoutinthevoid profile image
Full Name

"It depends" definitely strikes me as the answer to the overengineering question.

Extracting predicates feels to me like an easy sell. Their reusability and the readability improvement approaches self evident. How many general purpose js libraries come without an isEmpty function?

The html strings would require more context (and pose more questions - what situation are we in that this is preferable to a robust templating solution?). Either Keff didn't mention it or I overlooked it, but the separate functions also permit independent testing.

Thread Thread
 
lgrammel profile image
Lars Grammel • Edited

In this particular example, I would consider extracting predicates premature abstraction. You might never need them, and you add unnecessary cognitive complexity. When it comes to testing, the function can easily be unit tested as is.

That being said, I am in favor of extracting predicates etc. when there is reuse and value in doing so, i.e. following the 3 strikes rule for refactoring.

Thread Thread
 
ashoutinthevoid profile image
Full Name • Edited

What added cognitive complexity is there in reading isEmpty rather than parsing the expression line !== '' && line != null?

WRT to testing, a mistake in a predicate would cause a test of that branch's eventual value to fail. Not because the code that produces that value is wrong, but because the predicate is wrong. Yes, you can still test every one of the diverse things this function is doing, but conflating multiple behaviors just makes finding the specific problem harder. It's changing 'the problem is here' to 'the problem is in this general area'.

Thread Thread
 
lgrammel profile image
Lars Grammel • Edited

The complexity is in the number of lines to read and in the links that you need to follow. The individual pieces might be simpler, but their sum is not due to their links. E.g. if you want to understand what paragraph does, you need to find and read the definition, keep it in your head, go back to the place where it was used, and then put it together.

Re testing, I agree that it's good to tear things apart (and introduce e.g. predicates), but only when the code is complex enought. I consider this method way too trivial to do that - it's far easier to test as one.

Thread Thread
 
ashoutinthevoid profile image
Full Name • Edited

Okay, I follow what you mean by complexity. Thank you for clarifying. I don't wish to say I disagree, but I don't think I share your perspective.

I think there's an argument to be made in the very particular case that a function (eg isEmpty) has a name (or function signature, or instantly visible documentation if one has the convenience of IDE like features) that is clear to the reader, and that function is separately tested. That it's separately tested allows me to trust that the implementation does what it should. That the name or signature (particularly the signature, tbh) accurately describe behavior allows me ignore implementation details when I'm reading lineChecker.

If my units (ie functions as the unit of composition) are well named and tested, I should be able to compose them without repeatedly diving into every implementation of every unit. If one can't do that, I think there is a massive problem in one's approach to writing and testing software.

Diving into every implementation detail of every function while reading to understand the behavior of a function/module/unit/etc strikes me as a premature step in a manner similar to what you've expressed about extracting things before there are demonstrated instances of reuse.

Edit to add: If one has to jump back to the implementation of every unit within their own code, Im not sure I see how one can logically avoid doing the same thing with library code. Does the average React user feel the need to read the implementation of useState, for example? If not, why is that function call privileged?

Thread Thread
 
lgrammel profile image
Lars Grammel

Overall I think I agree with you and would have made similar arguments in the past. I often split code along similar lines in my projects, maybe with slightly different thresholds to method/function extraction.

However it seems to assume a perfect world with perfect software development projects, or that you and only you own the code, and you never need to hand it over to someone else.

In the reality that I've encountered in professional life, time pressure, employee turnover, mistakes, and other forces lead to source code being far from perfect, sadly. Trusting the function name or that a function is sufficiently tested is a bet in that world. I would not call that "a massive problem in one's approach" - for one-person projects, it's not an issue at all. It happens when shifting teams of developers work on moving requirements over multiple years, and it's non-trivial to avoid.

In that world, I prefer going for code that's as simple and predictable as possible, and that's why I prefer not extracting functions or predicates unless it is genuinely needed (either for testing or because it reduces code complexity, e.g., by removing duplication).

Thanks for the long reply; I enjoyed hearing your perspective!

Thread Thread
 
ashoutinthevoid profile image
Full Name

Fair enough.

Professionally I've encountered sprawling monoliths with no tests at all and team leads who felt it was okay because it had been in production longer than I had been with the company. Your quite right, none of us can avoid reality. It does not, however, change what I would aim for or advise to others.

Likewise, thank you for the civil discussion!