Good refactoring vs. bad refactoring
builder.io
builder.io
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.
If / when memory is low, allocating memory can itself be a side-effect...
> 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.
>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.
To borrow from medicine: First step is to always stop the `stop the hemorrhage`, then clean the wound, and then protect the wound(or wounds).
- Add a deprecation marker. In python this can be a decorator, context-manager, or even a magic comment string. This i ideally try to do while first introducing the pattern. It makes searching easier next time.
- Create a linter, with an escape hatch. If you can static analyse, type hint your way; great! In python i will create AST, semgrep or custom ones to catch these but provide a magic string similar to `type: noqa` to ignore existing code. Then there's a way to track and solve offending place. You can make a metric out of it.
- Everything in the system as to have a owner(person, squad, team or dept). Endpoints have owners, async tasks have owners, kafka consumers might have owners, test cases might have owners. So if anything fails you can somehow make these visible into their corresponding SLO dashboards.
The other alternative to this last step is "if possible" some platform squad can take over and do this as zero-cost refactor for the other product squad. Ofcourse the product squads have to help test/approve etc. It's an easier way to get people to adopt a pattern if you do it for them. But the ROI on the pattern has to be there, and the platform squad does get stuck doing cruft thankless work sometimes. If you do this judiciously the win might be thanks enough, like more robust systems, better observability/traces, less flaky tests etc. etc.I hope the owner is the same person that owns the code.
Tests might cover more code than a single unit owned by different teams, thus end up with multiple owners. Prefer "squads" as the owners rather the individuals.
But just like documentation the ownership might be stale and out of sync. So the idea would be let some reds in SLO dashboard correct them over time. It's not possible to automatically link "tests" to the "code" always.
And unit tests should never break/be red. If the code needs to changed, the test needs to be changed at the same time.
End-to-end tests can be flaky. Those probably shouldn't prevent deployments and can be red for awhile. Should probably manually confirm if the test is acting up, there's a change in behavior, or if something is legitimately broken before ignoring them though.
The problem is that sometimes the new pattern gets overridden by an even newer pattern, and so on, until you've got three different implementations from 2016, 2019, 2021, and then you find that in 2024 you're working on implementation number four and all the people who did the first three have left the company without writing any documentation or finishing their work.
The cost / benefit needs to be calculated, whichever approach is chosen, at an Appropriate periodicity, which also needs to be considered.
> 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.
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.
Why is that true? Particularly if you're not an OOP user/believer. It's not like "pattern" is some obscure term of art.
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)
[0] https://www.oreilly.com/library/view/design-patterns-element...
I understood that was exactly what you were saying! Sorry, I'd had a drink.
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.
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.
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.
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.
you have forEach:
list.forEach((item, index) => doSomething());
for (const item of list) { doSomething() };
Burkean programming lol
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.
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.
function processUsers(users: User[]): FormattedUser[] {
let adults = users.filter(user => user.age >= 18);
return adults.map(user => FormattedUser.new_adult(user));
}
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
$ python -m timeit "x = list('example sequence')"
2000000 loops, best of 5: 198 nsec per loop
In this case, you first declare the end-goal - visit a friend and have full gas tank, with the actual steps to achieve them being much less important and often left to be defined at a later point (e.g. which particular gas station, which particular pump etc.). This corresponds more to functional thinking.
An imperative thinking would correspond more to "I will sit in the car, start the engine, ride on highway, stop at address X, converse with Y, leave 2 hours later, stop at gas station X" - in this case the imperative steps are the dominant pattern while the actual intent (visit a friend) is only implicit.
I certainly don't think like that. My main goal is to visit a friend. The transportation is subordinate, it's only a mean to the goal, an implementation detail which I don't care about much. I might even take a train instead of driving the car, or even ride a bike, if I feel like it and the weather is nice on the day of the visit.
Now reflecting on this, I think such focus on the process (as opposed to focus on the goal), exact imperative order, not being able to alter the plan even if the change is meaningless in relation to the goal, is a sign of autism. But I don't believe most people think like that.
I think most people think more or less like that, and that it is not "a sign of autism".
Say if I want to filter a sequence of users and omit users below the age of 18, I'll construct my predicate (a "what"), and want to apply that predicate to create a new sequence of user (another "what").
I really don't want to tell a computer how to process a list every single time. I don't care about creating an index first and checking the length of my list in order to keep track that I process each user in my list sequentially, and don't forget that important "i++". All I want at that moment is to think in streams, and this stream processing can happen in parallel just as well for all I care.
But I also do think Python, Haskell etc. are the most expressive here with list comprehensions. It can't get more concise than this IMHO:
users_adult = [
user
for user in users
if user.age >= 18
]The caveat here is to which extent computers have tinted their models of a brain, but they are professional cognitive researchers so I’d give them the benefit of the doubt :)
It's not more intuitive for the entire system.
When you make a new file in a file system, you're already violating functional programming, even if you atomically create that file with all the content specified, and make it immutable.
You must construct a new file system which is like the old one, but with that file, and then have the entire system tail call into a world where you pass that new file system as a parameter, so that the old one is not known (garbage).
Unix has been the most successful system in getting partial functional programming into ordinary people's hands.
A Unix pipeline like < source-file | command | command | ... | command > dest-file is functional except for the part where dest-file is clobbered. Or at least can be functional. The commands can have arguments that are imperative programs (e.g. awk) but the effects are contained.
In the famous duel between Doug McIlroy and Knuth in solving a problem, in which McIlroy wrote a concise Unix script combining a few tools, McIlroy's solution can be identified as functional:
tr -cs A-Za-z '\n' |
tr A-Z a-z |
sort |
uniq -c |
sort -rn |
sed ${1}q
With imperative programming you can follow a program step by step, line by line, it can be done with a debugger, pen and paper, or in your head.
With functional programming, not so much. It runs functions. What do these functions do? Don't know, they are pieced up from someplace else in the code. And thanks to lazy evaluation, they may not even exist. In the design phase, it is mostly a good thing, as it is flexible, and pure functions are less likely to make a mess than functions with side effects, but there will be a point where the program will not behave as it should, no matter your paradigm. And that's when it becomes a problem.
It is also a problem with object programming if you abuse abstraction, in fact, it is a general problem with abstraction, but functional programming makes it the default, whereas imperative programming is concrete by default.
As for the Python example, I am a bit surprised that the optimizer didn't catch it, all three are common and equivalent constructs that could have been replaced by the most performant implementation, presumably the third one. But well, optimizers are complicated.
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.
And then someone slaps do {} while(0) in a macro.
The map and filter methods are nice too, but they're for one-liners.
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.
Is this just fashion? Is there a way to settle it other than “I like it better?”
Yes.
Readability is a characteristic of the reader, not what is being read.
This simple truth seems to be so hard for many people to internalize. My theory as to why is that most programmers never get exposed to drastically different and unfamiliar languages and styles of programming. If they were forced to confront and internalize 2-3 different ways of writing code, they would realize this truth.
Personally, I once thought Lisp was unreadable... until I learned it. I once thought BASH was unreadable... until I learned it. Same with half a dozen other languages. Same for styles. "Readability" is just a familiarity and proficiency of the reader.
There is no way that
name: R.pipe(R.prop('name'), R.toUpper),
age: R.prop('age'),
isAdult: R.always(true)
name: user.name.toUpperCase(),
age: user.age,
isAdult: trueBut instead of
for (const i=0; i < data.length; i++) {
new_data[i] = old_data[i].toUpperCase();
}
const new_data = old_data.map((x) => x.toUpperCase());
https://www.typescriptlang.org/play/?#code/MYewdgzgLgBCA2ATA...
All syntactically valid Javascript is also syntactically valid Typescript, it just adds stuff, though you can get runtime errors for things like reassigning variables in a way Javascript is fine with that Typescript disallows.
All I get out of that is that you like functional code.
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.
Yeah, there’s your problem. This is in fact possible!
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.
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.
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?
From my perspective there should always be a buy in - after refactoring the system is more understandable, but also more coupled. Is this fine? If no, can given refactor be merged now and result tackled in separate refactor. Caching refactor can have a buy in as well - ie. remove caching because given request shouldn't be cached, or this functionality should be decoupled and done elsewhere
Maybe you're right. I've had a drink, I'll think over it in the morning, thanks.
float Q_rsqrt(float number) {
return 1 / sqrt(number);
}
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.
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.
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.
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.
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.
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.)
refactoring is overrated plus refactor is all about: clarify the terms, then do the thing
I’ve found almost always that less is best.
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.
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.
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.
> 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.
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()
function isAdult({age}: {age: int}) {
return age >= 18
}
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.
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.
boolean isAdult() {
return this.age >= this.location.ageOfAdulthood();
// or this.location.isAdult(this.age); pick your poison!
}
* You may need to determine adulthood for a different jurisdiction than where the person currently resides. Their citizenship may be elsewhere, or you may be running a report that expects "adulthood" to be by some other region's standards, etc.
* Sometimes the underlying kind of adult-need wanted is slightly different, like for consuming alcohol or voting.
* There may be a weird country or province has laws that need additional factors, like some odd place where it's a different age-cutoff for men and women.
class User {
// Convenience function to check if the user is an adult in their current location
boolean isAdult() {
return this.location.isAdult(this);
}
boolean isOfDrinkingAge() {
return this.location.isOfDrinkingAge(this);
}
}
interface Location {
boolean isAdult(User u);
boolean isOfDrinkingAge(User u);
}
class WeirdLawsLocation implements Location {
boolean isAdult(User u) {
return switch (u.gender()) {
case MALE -> u.age() >= 16;
case FEMALE -> u.age() >= 18;
}
}
boolean isOfDrinkingAge(User u) {
return u.age() >= 21
}
}
class SwedenLocation implements Location {
boolean isAdult(User u) {
return u.age() >= 18;
}
boolean isOfDrinkinAge(User u) {
return u.age() >= 18;
}
}
var sweden = new SwedenLocation();
sweden.isOfDrinkingAge(user); j = Jurisdiction.fromUserLocation(user);
j.isOfDrinkingAge(user);That’s just your opinion. It’s ok to provide convenience functions. I see no difference between the amount of indirection in our implementations, except mine is in the more natural place and you don’t have to know how to get a location or jurisdiction to answer the question: “is this user an adult?”. Knowing that it uses a location or jurisdiction is an implementation detail that you shouldn’t couple yourself to.
Cheers mate, I think I’m done moving goal posts for this conversation :)
Also from the guidelines:
> Please don't post shallow dismissals, especially of other people's work. A good critical comment teaches us something.
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.
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.
Not a very qualitative article.
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.
Basically, make it simpler without breaking it or actually making it not simpler.
No need to loop twice over the array. Always use reduce if there are chained methods going through arrays
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.
x^2 + x z + y x + y z
then you notice that you can express the same polynomial as:
(x + y)*(x + z)
It's still the same polynomial, but you "separated concerns", turning it into a product of two simpler factors.
Similar ideas apply to sets of tuples. Perhaps you were given
{(1, 1), (1, 4), (2, 1), (2, 4), (3, 1), (3, 4)}
and you notice that this can be expressed more simply as the Cartesian product:
{1, 2, 3} x {1, 4}
Again, a literal factoring. You can imagine how variations of this idea would apply to database tables and data structures.
That's where I think the word "refactor" comes from.
Nice to learn where the word originates from. Often the meaning of words change over time. E.g. today no horses need involved in bootstrapping.
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.
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.
That’s down to preference.
Doesnt both filter and map copy the array increasing gc pressure?
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.
Yes, sorry; you are right of course.