Good Refactoring vs. Bad Refactoring

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.

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.

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.

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.

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.

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.

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.

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

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.

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.

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.

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

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.

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.

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.

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!

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.

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

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.

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.

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.

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.

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

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!

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.

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

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

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.