Good refactoring vs. bad refactoring

https://www.builder.io/blog/good-vs-bad-refactoring

by steve8708 • 2 years ago

226 150 2 years ago

The first example complained about the refactor appealing to functional thinkers (implying that it would be difficult to grok by the existing devs), but then the “improved” version is virtually the same save for the (unnecessary?) use of Ramda in the first.

And while many devs are resistant to try functional ways, this first example reads so much better than the original code that I find it impossible to believe that some prefer the imperative loop/conditional nesting approach.

happytoexplain• 2 years ago

Are you saying you find it hard to believe that some prefer the first "Before" example over the first "Bad refactor" example?

p2501• 2 years ago

Aesthetic aside, I am under the impression that people start programming, by and large, with imperative for/if style => so the imperative style is readable by more people. Even for more experienced programmers, reading imperative probably cost less energy, since it is more internalised?

Futhermore, in JS, the functionnal style is less performant (nearly twice on my machine, i assume because it do less useless memory allocations)

So, same functionnality, readable by more people, more performant? The imperative example seems like the better code.

joshka• 2 years ago

I, a Datapoint of 1, find the functional style to generally express the ideas of what's happening to be significantly easier to grok. Particularly if intermediate variables and conversion constructors are introduced rather than relying on full chains. E.g.:

    function processUsers(users: User[]): FormattedUser[] {
      let adults = users.filter(user => user.age >= 18);
      return adults.map(user => FormattedUser.new_adult(user));
    }

On the performance tip, what scale are we talking? Is it relevant to the target system? Obviously the example is synthetic, so we can't know that, but does it seem like this would have a runtime performance that is meaningful in some sort of reasonable use case?

zahlman• 2 years ago

I am under the impression that people start programming, by and large, with imperative for/if style => so the imperative style is readable by more people.

IMO, this is a simple consequence of technology moving faster than society. There are still instructors out there who learned to program in an environment where the go-to options for imperative programming were C and FORTRAN; the go-to options for other paradigms (if you'd even heard of other paradigms) were things like Lisp, Haskell and Smalltalk; and CPU speeds were measured in MHz on machines that you had to share with other people. Of course you're going to get more experience with imperative programming; and familiarity breeds comprehension.

But really, I believe strongly that the functional style - properly factored - is far more intuitive. The mechanics of initializing some output collection to a default state (and, perhaps, the realization that zero isn't a special case), keeping track of a position in an input collection, and repeatedly appending to an output, are just not that interesting. Sure, coming up with those steps could be a useful problem-solving exercise for brand-new programmers. But there are countless other options - and IMX, problem-solving is fiendishly hard to teach anyway. What ends up happening all the time is that you think you've taught a skill, but really the student has memorized a pattern and will slavishly attempt to apply it as much as possible going forward.

Futhermore, in JS, the functionnal style is less performant (nearly twice on my machine, i assume because it do less useless memory allocations)

Sure. Meanwhile in Python:

    $ python -m timeit "x = []" "for i in 'example sequence':" "  x.append(i)"
    500000 loops, best of 5: 796 nsec per loop
    $ python -m timeit "x = [i for i in 'example sequence']"
    500000 loops, best of 5: 529 nsec per loop

... But, of course:

    $ python -m timeit "x = list('example sequence')"
    2000000 loops, best of 5: 198 nsec per loop

Horses for courses.

The_Colonel3• 2 years ago

Even for more experienced programmers, reading imperative probably cost less energy, since it is more internalised?

I disagree. For-cycles are usually more difficult to reason about, because they're more general and powerful. If I see "for (...", I only know that the subsequent code will iterate, but the actual meaning has to be inferred from the content.

Meanwhile, a .map() or .filter() already give me hints - the lambda will transform the values (map), will filter values (filter), these hints make it easier to understand the logic because you already understand what the lambda is meant to do.

Other benefits stem from idiomatic usage of these constructs. It's normal to mix different things into one for-cycle - e.g. filtering, transformation, adding to the resulting collection are all in the same block of code. In the functional approach, different "stages" of the processing are isolated into smaller chunks which are easier to reason about.

Another thing is that immutable data structures are quite natural with functional programming and they are a major simplification when thinking about the program state. A given variable has only one immutable state (in the current execution) as opposed to being changed 1000 times over the course of the for-loop.

skybrian4• 2 years ago

(Raises hand.) I prefer the for loop. Pushing items to an array is idiomatic Javascript for creating an array. An if statement is an idiomatic way to do it conditionally. It's also easier to debug.

The map and filter methods are nice too, but they're for one-liners.

nine_k3• 2 years ago

Writing assembly was the idiomatic way of programming before Fortran and human-readable languages came.

Writing with goto was the idiomatic way before Algol and structural programming came.

Having only a handful of scalar types was the idiomatic way until structural data types came (and later objects).

Writing programs as fragments of text that get glued together somehow at build time was the idiomatic way until module systems came. (C and partly C++ continue to live in 1970s though.)

Callback hell was the idiomatic way to do async until Futures / Promises and appropriate language support came.

Sometimes it's time to move on. Writing idiomatic ES5 may feel fun for some, but it may not be the best way to reach high productivity and correctness of the result.

bobthepanda2• 2 years ago

I will say in 2024 i feel like for/of or forEach would at least let you avoid the boilerplate of an index.

asddubs• 2 years ago

I also prefer the loop, I would negate the condition and use continue, but otherwise leave it unchanged. I don't have a problem with the functional version but it doesn't scan as well to me.

mabster• 2 years ago

I don't code in JS very often, so there's that.

Both sets of code were fine, but I understood the loop variant instantly, while it took me a bit longer with the FP code.

As a side note: The only real JS I did was optimising some performance critical code, and I did have to refactor a number of FP chains back to loops. This was because the FP way keeps constructing a new list for each step which was slow.

diatone• 2 years ago

I find it impossible to believe that some prefer the imperative loop/conditional nesting approach.

Yeah, there’s your problem. This is in fact possible!

jamil7• 2 years ago

I've haven't written JS in a long time – are engines like V8 smart enough to roll the filter and map into a single loop? Otherwise wouldn't a reduce be more efficient there?

nevon• 2 years ago

It's not a matter of being smart enough. Since JavaScript is interpreted, the optimization happens at runtime. If the code is executed once and the number of items in the array is small, then it will take more time for the compiler to optimize the code than to naively execute it. Most code falls into this category.

As for whether or not it's possible at all to combine a map and filter into a single loop I guess depends on whether the first operation can have side effects that affect the second operation or the collection that is being iterated over. I don't know the answer, but I would be surprised if there wasn't some hard to detect corner case that prohibits this kind of optimization.

ggm2• 2 years ago

Good refactoring respects the idioms of the language and the culture of the organisation. Change to new methodology is thoughtful and probably slow, except when a revolution happens but then, its still respectful to the new culture.

Bad refactoring is elitist, "you won't understand this" commented and the owner walks with nobody left behind who understands it.

That the examples deprecated FP and preferred an idiom natural to Java(script) only speaks to the principle. I can imagine a quant-shop in a bank re-factoring to pure Haskell, out of somthing else, and being entirely happy that its FP respecting.

So the surface "FP patterns are bad" is a bit light-on. The point was, nobody else in that specific group could really be expected to maintain them unless they were part of the culture.

"If you unroll loops a la duff's device, you should explain why you're doing it" would be another example.

boxed2• 2 years ago

The dig against FP is weird since the "good refactor" also uses FP, just a built in one in JS. Which I agree is better, but mostly by being built in and idiomatic, it's still exactly as functional.

ggm2• 2 years ago

"I don't like this style of FP coding" ok: if you run the group and own the codebase you can enforce that. So good refactoring is style guide enforcement.

I think I over-read his dislike of FP. really the complaint is "why did you introduce a new dependency" which I am totally fine with, as a complaint. Thats not cool.

Many of his examples kind-of bury the lede. If he had tried to write an abstract up front, I think "dont code FP" wouldn't have been in it. "use the methods in the language like .filter and .map" might be.

watwut2• 2 years ago

I think that was the point - the library added nothing, the same thing could be done with pure javascript.

carlmr• 2 years ago

On point. Even then mentioning it used filter and map. But the bad refactor also uses filter and map. It's the exact same change of programming paradigm.

Given the text, I would have expected some minor refactor with range-based for loops (are these a thing? My JS is rusty). Where you get the advantage of map (no off-by-one indexing errors) without changing the programming paradigm.

achillesheels• 2 years ago

Good refactoring respects the idioms of the language and the culture of the organisation. Change to new methodology is thoughtful and probably slow, except when a revolution happens but then, its still respectful to the new culture.

Burkean programming lol

nailer• 2 years ago

Oh god the ‘object oriented’ refactor. I wish everyone who had OO thrust upon them in the early 2000s received some explicit communication that what they were taught is essentially a hoax and bears no resemblance to Alan Kay’s original intention

spc476• 2 years ago

Some of that is on Alan Kay because it took him twenty years to realize people couldn't read his mind on the proper definition of "object oriented."

sevnin6• 2 years ago

To be fair OOP of today is much more similar to Simula then to Small Talk, reading the wikipedia I can see almost 1:1 mapping including the modeling philosophy and all that, people mostly yoinked the name from Alan Kay.

CRConrad8• 2 years ago

"Object" and "orient" are ordinary English words; Alan Kay doesn't own either of them. He may have been the first to combine them as a single term, but he doesn't own that either. (Or, did he trademark it or anyything? I doubt it.) That another definition of it than his came to be the dominant one is just the way things are, not in any way "a hoax".

vips7L7• 2 years ago

That OO refactor isn’t actual OO. The tell tale sign is that it is named by what it does rather than what it is (verb vs noun) and the -or ending in the name [0]. It’s just a function masquerading as a class.

The better refactor to introduce OO concepts would have been to introduce an isAdult function on the user class and maybe a formatted function. This + the functional refactor probably would have made for the best code.

    return users.filter(u => u.isAdult())
      .map(u => format(u)); // maybe u.formatted()

[0] https://www.yegor256.com/2015/03/09/objects-end-with-er.html

dominicrose• 2 years ago

What about a pure function that can take anything that has an age as input? Well obviously that wouldn't work for a cat but it's just an example. It requires typescript and I'm not sure how to name the file it would go in, but I think it's interesting to consider this duck-typing style.

    function isAdult({age}: {age: int}) {
        return age >= 18
    }

ps: I replaced const by function because I don't like the IDE saying I can't use something before it is defined. It's not a bug it's an early feature of javascript to be able to use a function before it is defined. Code is just easier to read when putting the caller above the callee.

vips7L7• 2 years ago

That wouldn’t be object oriented. In OO you tend to want to ask an object about itself, Yegor talks a bit about this in his book Elegant Objects.

What you are proposing is just functions or data-oriented programming; which is fine if that’s your thing, but I’d be weary because of the reasons you outline above. Can a book be an adult? What about a tv show? Or recipe from the 9th century? isAdult really only applies to users and really belongs on that object.

Frieren• 2 years ago

u.isAdult()

Being adult is not a property of the user but of the jurisdiction that the user is in. In some places or some purposes it is 18 but it could be, e.g., 21 for other purposes.

If you software is not going to just run on the USA it is not a good idea to implement isAdult in the user but in a separated entity that contains data about purpose and location.

vips7L7• 2 years ago

With proper OO you could still implement that on the user object.

    boolean isAdult() {
        return this.age >= this.location.ageOfAdulthood();

        // or this.location.isAdult(this.age);  pick your poison!
    }

…anyway it’s just an example of how to introduce OO concepts. As everything in programming it depends

CRConrad8• 2 years ago

Being adult is not a property of the user but of the jurisdiction that the user is in.

It's a function of both. And I'd argue it's really mainly a function of the person: Independently of jurisdiction, there's at least a rough global consensus what "being adult" means, and most jurisdictions set rather similar (many of them, identical) limits.

A four-year-old isn't an adult anywhere; a fourty-year-old is everywhere.

piotrkaminski• 2 years ago

What I get out of this is that even for teams that prioritize trust and velocity and eschew the use of pre-commit code reviews, there's a strong argument to be made for putting all new hires on an approval-required list for the first few months!

jv_be• 2 years ago

A good refactor does not change behaviour, I would like the author to start with that point. Take many more much smaller steps while doing so. Not touching a piece of code in the first 6 to 9 months is something I don’t really agree with. Breaking complex methods up by extracting variables and methods can really help learning the code, whilst not breaking it. If you are worried about consistency, just pair up of practice ensemble programming instead of asynchronous code reviews. Leaving a new dev alone with the code and give them feedback about the things they did wrong after they went through everything is just not a great way to treat people in your team.

kolme3• 2 years ago

A refactor does not change behavior, period. By definition.

If something changed, it's not a refactor. It's a change.

Like in the example where the caching was removed: NOT a refactor.

Ore where the timeouts for the requests were changed: NOT a refactor.

The definition of refactor: change the structure of the code without altering the behavior.

It's like saying a crash is a bad landing.

sevnin6• 2 years ago

"By definition" proceeds to define the word in a way that most people won't agree with. Okay mate. Your definition is ass. The person that wrote the article doesn't agree with you, I don't agree with you. I will still use the word refactor when I talk about simplifying the application such that code becomes simpler through simplifying and improving the design.

whilenot-dev2• 2 years ago

A refactor does not change behavior, period. By definition.

Refactoring does not change the external behavior. If you can't change the internal behavior, then you can't reduce complexity.

So with that in mind...

- changing implicit caching => refactoring

- changing implicit timeouts => refactoring

- changing explicit caching => more than refactoring

- changing explicit timeouts => more than refactoring

Because the word external implies conceptual boundaries, I would personally also distinguish refactoring by levels:

- system design

- service design

- program design

- component design

- module design

- function design

...where this blog post only talks about the latter two.

nuancebydefault6• 2 years ago

By _your_ definition. If you apply it, a lot of effort is needed for little gain. The best refactors i've seen improved behavior while making code more concise.

azangru2• 2 years ago

The first example of a good refactor is a meh refactor at best, and possibly a bad refactor. Array methods such as map or filter are not "more conventional" in javascript; they are "as conventional" as for-loops, and arguably less "conventional", given how for-loops have been around since the introduction of the language. They are also inevitably more expensive than for-loops (every cycle creates an anonymous function; a map followed by a filter means another iteration of the array). The original example was fine; there was no need to "refactor" it.

knallfrosch• 2 years ago

Disagree on this. filter and map are much more readable and especially extensible than result-arrays. Plus it eliminates out-of-bonds indexing.

See the variable name. It's forced to be 'result' so that it's consistent with the result-array style. Therefore it lacks a descriptive name.

For the functional methods, you can easily assign the filter(age > 18) result to an intermediate variable like adultUsers to make the code even more descriptive. Useful when you have more steps. With the result-array approach, you'd have to repeat the looping code or bury the description deep in the loop itself and so you usually avoid that.

asp_hornet• 2 years ago

much more readable

That’s down to preference.

Doesnt both filter and map copy the array increasing gc pressure?

kolme3• 2 years ago

every cycle creates an anonymous function

No, that's not how it works. The function is evaluated once before the call and passed as an argument, then internally reused.

Also, you're microptimizing. Prioritizing supposed performance over readability.

And yes, for-loops and mutable structures are more error prone than map-filter-reduce. The original is OK but could be better.

azangru2• 2 years ago

No, that's not how it works. The function is evaluated once before the call and passed as an argument, then internally reused.

Yes, sorry; you are right of course.

mariopt• 2 years ago

Words can not express my hate for this kind of articles.

Imagine working on a legacy codebase where the PM holds the dogma of refactoring being a bad thing and expecting you to do it wrong, even micro managing your PRs.

Most often than not, I do see projects suffering and coders actually resigning due to a lack of internal discussing about best practices, having space/time to test potential solutions, having Lead devs who resemble dictators quite well.

Let me guess, some PM wrote this article and they just want you to push the product asap by applying pressure and not allowing you ever to refactor. This is just a casual day in software development. I'm not surprised anymore when most web apps have silly bugs for years because it's gonna be a Jira ticket and a big discussion about..... one evil thing called refactor.

Several years ago I rewrote a full SaaS in about 3 months, it took another team 12 months with 5 devs. Guess which version made the investors happy, mine.

Bad refactoring is just a product of poor engineering culture.

exe34• 2 years ago

more people just means more time spent trying to coordinate and in the limit, you spend all the time talking and none coding.

remus• 2 years ago

Imagine working on a legacy codebase where the PM holds the dogma of refactoring being a bad thing and expecting you to do it wrong, even micro managing your PRs.

I don't think the article said that anywhere? It was just a list of some common things that can go wrong when refactoring, along with some examples.

CRConrad8• 2 years ago

Let me guess, some PM wrote this article

Nah, judging from the ancillaries (domain name, links to other articles, ads, etc) of the article, it was some guy selling an "AI" code tool of some kind who wrote the article.

(Probably a tool with Magikal Refactoring Functionality built-in... For a price.)

Jean-Papoulos2• 2 years ago

I feel this article is honestly disingenuous. One of the "common pitfalls of refactoring" mentioned is "Not understanding the code before refactoring". Well yeah ? The same would apply to doing anything with the code. The following one is "Understand the business context" (note that the author has already departed from the pattern of listing "common pitfalls" to just write whatever he feels like. Or he just published his first draft).

Not a very qualitative article.

MattHeard• 2 years ago

I got as far as here:

If you need to introduce a new pattern, consider refactoring the entire codebase to use this new pattern, rather than creating one-off inconsistencies.

Putting aside the mis-application of "pattern" (which _should_ be used with respect to a specific design problem, per the Gang of Four), this suggestion to "refactor the entire codebase" is impractical and calcifying.

Consistency increases legibility, but only to a certain point. If the problems that your software is trying to solve drift (as they always do with successful software), the solutions that your software employs must also shift accordingly. You can do this gradually, experimenting with possible new solutions and implementations and patterns, as you get a feel for the new problems you are solving, or you can insist on "consistency" and then find yourself having to perform a Big Rewrite under unrealistic pressure.

Diggsey• 2 years ago

Putting aside the mis-application of "pattern" (which _should_ be used with respect to a specific design problem, per the Gang of Four)

This is not in any way a mis-application of the word "pattern". There is no exhaustive list of all design patterns. A design pattern is any pattern that is used throughout a codebase in order to leverage an existing concept rather than invent a new one each time. The pattern need not exist outside the codebase.

Consistency increases legibility, but only to a certain point.

It's the opposite: inconsistency decreases legibility, and there is no limit. More inconsistency is always worse, but it may be traded off in small amounts for other benefits.

Take your example of experimenting with new solutions: in this case you are introducing inconsistency in exchange for learning whether the new solution is an improvement. However, once you have learned that, the inconsistency is simply debt. A decision should be made to either apply the solution everywhere, or roll back to the original solution. This is precisely the point the author is making by saying "consider refactoring the entire codebase to use this new pattern".

This refactoring or removal doesn't need to happen overnight, but it needs to happen before too many more experiments are committed to. Instead what often happens is that this debt is simply never paid, and the codebase fills with a history of failed experiments, and the entire thing becomes an unworkable mess.

Y_Y2• 2 years ago

"pattern" (which _should_ be used with respect to a specific design problem, per the Gang of Four)

Why is that true? Particularly if you're not an OOP user/believer. It's not like "pattern" is some obscure term of art.

_a_a_a_6• 2 years ago

For consistency, the same design problems should have the same design solutions a.k.a. pattern. If you don't value consistency, feel free to take a different approach every time. That will confuse your users. I used to work with a guy, the simple problem of reading a CSV was done using a library, problem sorted. Out of sheer excitement he then rewrote it was with combinator parsers, then as some astronaut architect functional monstrosity, so complex no one else could comprehend it.

This is how not to do it – for same problem, use same solution. I admit that's an extreme case but it's also a real one and illustrates the issue well.

(also patterns are not specific to OO, nor is OO incompatible with a functional style)

watwut2• 2 years ago

It is not just about legibility. Developers are trying to repeat patterns they see around. If you do not refactor previous places, they will reproduce the outdated pattern. Plus, it makes code reviews frustrating.

kolme3• 2 years ago

There was a great refactoring chance in the example where the cache was removed.

That is, extracting the caching logic from the API call logic.

Caching could have been a more generic function that wraps the API call function. That way each function does exactly one thing, and the caching bit can get reused somewhere else.

Instead, this weird advice was given: changing behavior is bad refactoring. Which is weird because that's not even what we call refactoring.

Edit: removed unnecessary negativity.

ahoka3• 2 years ago

Developers regularly make code changes that change behavior and insist that it’s refactoring.

pif2• 2 years ago

Concerning the first example: whoever thought that passing property names as string can be better than any other style should have his coding licence revoked!

pif2• 2 years ago

Second example: doesn't the bad refactor also modify the list of users?

creesch• 2 years ago

I am not sure if I agree with the article, however I do agree that not every time code actually needs to be formatted.

More than a few of them have come in with a strong belief that our code needed heavy refactoring.

The code might, but a blind spot for many developers is that just because they are not familiar with the code doesn't mean it is bad code. A lot of refactoring arguments I have seen over the years do boil down to "well, I just don't like the code" and are often made when someone just joins a team at a point where they haven't really had time to familiarize themselves with it.

The first point of the article sort of touches on this, but imho mainly misses the point. In a few teams I worked in we had a basic rule where you were not allowed to propose extensive refactoring in the months (3 or more) of being on the team. More specifically, you would be allowed to talk about it and brainstorm a bit but it would not be considered on the backlog or in sprints. After that, any proposal would be seriously considered. This was with various different types of applications, different languages and differently structured code. As it turned out, most of the time if they already did propose a refactor, it was severely scaled down from what they initially had in mind. Simply because they had worked with the code, gained a better understanding of why things were structured in certain ways and overall gotten more familiar with it. More importantly, the one time someone still proposed a more extensive refactoring of a certain code base it was much more tailored to the specific situation and environment as it otherwise would have been.

Edit: Looks like it is being touched on in the fourth point which I glossed over. I would have started with it rather than make this list of snippeted examples.

aappleby• 2 years ago

Good refactoring should significantly reduce the size or complexity of a codebase.

These two metrics are interrelated, but as a general rule if the gzipped size of the codebase (ignoring comments) does not go down, it's probably not a good refactoring.

_a_a_a_6• 2 years ago

I'm going to disagree and see what other people say.

I don't think that reduction of size is of any relevance. I admit my own refractors tend to make things smaller but it's only a tendency. Most definitely some increase the size overall. I'm currently refactoring a code base – for each item there used to be one class. Each object was examined after creation then a runtime flag was set: Rejected or Accepted. As the code crew I found I was wasting a lot of time around this Accepted/Rejected stuff. Now I'm refactoring so I have two classes for each item, one for when it's Accepted and one for when it's Rejected. The amount of boilerplate has definitely bulked up the code but it will be worth it.

As for complexity, I don't know.

The only thing I refactor for is human comprehensibility. That is the final goal. What other goal can there be?

conceptme• 2 years ago

Other goals can be performance, testability

riwsky• 2 years ago

Sure, comprehensibility is the terminal goal—but size reduction is relevant because it’s an easily measured, decently predictive proxy for that. No, code-golfing 3 lines down to one doesn’t help, but the bigger size differences are always of the “composition over XYZ” variety that change the entire complexity class of how much code you need to write (like React letting people write O(states) rendering code instead of O(transitions)).

tstrimple• 2 years ago

I don't think that reduction of size is of any relevance.

More code equals more bugs. This has been a pretty consistent finding dating back decades. Simpler and smaller code bases will generally have fewer bugs.

https://news.ycombinator.com/item?id=33566329

charlie02• 2 years ago

Agreed with everything except the following:

Remember, consistency in your codebase is key. If you need to introduce a new pattern, consider refactoring the entire codebase to use this new pattern, rather than creating one-off inconsistencies.

It's often times not practical (or even allowed by management due to "time constraints") to refactor a pattern out of an entire codebase if it's large enough. New patterns can be applied to new features with large scopes. This can work especially in the cases of old code that's almost never changed.

Flop7331• 2 years ago

The key word is consider. If you wouldn't apply the pattern to the whole codebase, maybe you don't actually want to introduce it in just this one new place.

charlie02• 2 years ago

It's not that it wouldn't be applied to the whole codebase, it's that it wouldn't be applied to the whole codebase __at once__. You have to start somewhere and new features are a good place to start new patterns. Older code can be refactored piece by piece.

corytheboyd• 2 years ago

To me it’s less about “consistency” as some nebulous, subjective thing. If you want to set the new standard for $thing, whole-ass it and set the new standard for $thing. I fully support this, but within reason of course. The point at which I duck out of this is when numerous replacement operations require significant non-trivial changes to highly depended on and/or untested code. Otherwise if it’s a simple task that a good IDE and a couple hours of hard work can solve… just do it!

osigurdson• 2 years ago

Consistency is good but isn't everything. If you have a very large code base and insist on perfect consistency then no changes can practically be made and all code maintains a "day one" style forever.

ncr100• 2 years ago

"Foolish Consistency is foolish." -me, wasting your reading attention

The cost / benefit needs to be calculated, whichever approach is chosen, at an Appropriate periodicity, which also needs to be considered.

doctorM• 2 years ago

Reading this I realised I've kind of drifted away from the idea of refactoring for the point of it.

The example with the for-loop vs. map/filter in particular - it's such a micro-function that whichever the original author chose is probably fine. (And I would be suspicious of a developer who claimed that one is 'objectively' better than the other in a codebase that doesn't have an established style one way or the other).

Refactor when you need to when adding new features if you can reuse other work, and when doing so try to make minimal changes! Otherwise it kind of seems more like a matter of your taste at the time.

There's a limit of course, but it's usually when it's extremely obvious - e.g. looong functions and functions with too many parameters are obvious candidates. Even then I'd say only touch it when you're adding new features - it should have been caught in code review in the first place, and proactively refactoring seems like a potential waste of time if the code isn't touched again.

The (over) consolidation of duplicated code example was probably the most appealing refactor for me.

kagevf• 2 years ago

I agree, which is better - for or map - depends on context. map typically is functional and allocates memory, for does not. But for is more likely to have side-effects. Which trade-offs matter depends on the larger context of the surrounding code.

CRConrad8• 2 years ago

map typically is functional and allocates memory, for does not. But for is more likely to have side-effects.

If / when memory is low, allocating memory can itself be a side-effect...

move-on-by• 2 years ago

I agree! No one wants to review unnecessary stylistic changes.

it should have been caught in code review in the first place

There is probably a ticket rotting in someone’s backlog to ‘clean it up’, unless someone declared ticket bankruptcy.

lofaszvanitt• 2 years ago

Why hire someone new and then let the person do refactoring. And then write a useless article about it, like it's some groundbreaking insight.

refactoring is overrated plus refactor is all about: clarify the terms, then do the thing

alphazard• 2 years ago

Refactoring isn't an end on its own, and it shouldn't ever be considered a standalone project.

The easiest way to accomplish a real goal like fix a bug, or add a feature, may very well be to first refactor the code. And yes maybe you want to merge that in as its own commit because of the risk of conflicts over time. But just having the code look nice (to who exactly?) isn't valuable, and it encourages the addition of useless abstractions. It may even make later real-work harder. Never refactor outside the context of real-work.

The cartoon with the PM also gets at a ridiculous pattern: engineers negotiating when to do various parts of their job with non-technical people who have no idea how do any part of their job. The PM doesn't know what a refactor is, the EM probably doesn't either. It doesn't make the organization function any better to tell these people about something they don't understand, and then ask them when it should be done. Budget it as part of the estimate for real-work.

tyleo• 2 years ago

I tend to think that a good measure for many sorts of refactoring is, “is it less code?”

I’ve found almost always that less is best.

pensatoio2• 2 years ago

This code is unreadable chaos, before and after.

This article just reminds me why I hate JavaScript so much. I know you frontend engineers can’t avoid it, but I wish we could come up with something better.

davidmurdoch• 2 years ago

What languages do you like to spend time in?

pensatoio2• 2 years ago

Golang is my favorite right now, but I think it’s possible to create readable code in many languages, even ones I like less. The JavaScript and its ilk strike me as some of the worst of the “modern” languages. Again, no hate here. I started out with JS and PHP, 20 years ago. I just cringe any time I see modern JS syntax, and wish it were simpler so I could get back into it.

abcde777666• 2 years ago

Short version: good refactoring = improve maintainability, brevity and usability without compromising functionality.

Basically, make it simpler without breaking it or actually making it not simpler.

danfritz• 2 years ago

Clearly the real good refactor was using reduce instead of filter / map.

No need to loop twice over the array. Always use reduce if there are chained methods going through arrays

darioush• 2 years ago

I agree with the sentiment of this article, more recently I have come to think the best refactoring is the one you don't undertake. Hoping to achieve consistency is a very high standard.

Often code structure matters much less than data flow and how it is piped. Since most codebases use a mixture of:

- global state or singletons, - configurations provided externally (config file, env var, cmd option, feature flag, etc.), that are then chopped up and passed around, - wrappers and shims, - mix of push and pull to get input/outputs to functions, - no consistency or code representation of assumptions about handling mutable state,

It may be better to build around existing code using ideas listed here than to try to refactor code to improve its structure: - open/closed principle = compose new code for new functionality (instead of modifying), - building loosely coupled modules (that interface via simple types and a consistent way of passing them) - enforcing an import order dependency via CI (no surprise cyclic dependencies months after an unrelated feature added some import that doesn't "belong")

The code's structure will be simple if the dataflow (input, outputs, state, and configuration) flows consistently through the codebase.

gloosx• 2 years ago

Ad-post for yet another AI tool.

Refactoring is about moving existing code around, not introducing new code. Replacing localStorage methods with cacheManager is a fix/feature. Updating one part of the codebase to work completely differently from the rest is a fix/feature. Changing processUsers to a whole useless class is not considered refactoring, it is a fix/feature. A single page app for a SEO-focused site is NOT a bad idea since 2018. Most examples of "refactors" in the article are actual fixes and features which brought (bad), or not brought (good) new regressions into the software.

CRConrad8• 2 years ago

Ad-post for yet another AI tool.

Ad-posts for AI tools seem (almost?) always to be written by AI tools.

Only I don't know what proportion of them are written by computerised AI tools.

idrios• 2 years ago

I hate this article. It's a very smug way of blaming the dev who's just trying to make the app better when it's probably the culture that's the problem. Bad refactors usually happen because the person doing the refactor is getting a ton of pushback on it -- they probably underestimated the effort involved and are getting chewed out for taking too long on it, so they cut corners that might accidentally lose functionality, or they don't finish the desired abstraction / clean code they were going for which leaves the code less readable.

For a dev that's a new hire, refactoring the code is also a way for them to feel ownership over it. The PM should be happy that they're thinking about the way the code works and the way the code should work. It's on the company to have review & qa processes that catch problems before they lead to downtime.

I don't disagree that some of the examples given are bad refactors, but in regards to adding inconsistency I see that happen a lot more when rushing out new features or bug fixes than when refactoring; usually the refactor is the effort trying to establish some kind of consistency. And example 5 isn't a refactor it's just removing functionality. If that was the intent, the person should be told not to do that. If it's an accidental side effect of some larger refactor effort, then just add the functionality back in a new PR. Accept that mistakes happen, adopt some QA controls to catch them, and build a culture that encourages your developers to care about your product.

greenhearth• 2 years ago

Not bad, but then it turned out to be an ad for their AI thing

CRConrad8• 2 years ago

Not particularly good either, so on the whole pretty much just an ad.

madcocomo• 2 years ago

This is an interesting topic, but I don't think the article effectively conveys its message.

The title focuses on good and bad refactoring, but most of the content discusses good and bad design. This means that many of the bad examples are inherently bad, regardless of whether they were refactored from another version or written from scratch. The introductory comic and the conclusion mention how to perform refactoring, but the rest of the article drifts away from this and only discusses the resulting code. The first pitfall mentions changing the coding style, but the explanation actually addresses the problem of introducing external dependencies. The fifth point, "understand business context," should actually be "not understanding business context." If we perform refactoring incrementally, it's inevitable that there will be some inconsistencies during the process. Therefore, the third pitfall, "adding inconsistency," should include additional explanations.

In summary, I think the article would be more helpful if it focused more on how to perform refactoring rather than criticizing a specific piece of code.

Good refactoring vs. bad refactoring

Related Stories