9 ms·

Good refactoring vs. bad refactoring

226 points by steve8708 2y ago

michaelteter 2y 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 2y ago
  Are you saying you find it hard to believe that some prefer the first "Before" example over the first "Bad refactor" example?
- p2501 2y 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 2y 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 2y 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.
    - vasco 2y ago
      People think imperatively though. If I think of visiting my friend, grabbing gas on the way back, the way I'll visualize the steps is not functional.
      - The_Colonel 2y ago
        That's quite debatable. 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.
  - The_Colonel 2y 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.
    - math_dandy 2y ago
      No need to fear mutable local state. Shared state is where immutable data structures really shine.
    - persnickety 2y ago
      > If I see "for (...", I only know that the subsequent code will iterate And then someone slaps do {} while(0) in a macro.
- skybrian 2y 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_k 2y 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.
    - skybrian 2y ago
      Making analogies like this doesn't prove anything, they're just suggestive. All I'm getting out of this is that you think for loops are old-fashioned.
      - Jean-Papoulos 2y ago
        That's because they are. Functional code is more readable. And if you look back, basically all advances in programming languages have been about "making stuff more readable". Thus, for loops (for this usage) are "old".
        CRConrad 2y ago
        > Functional code is more readable. All I get out of that is that you like functional code.
        2y ago
- diatone 2y 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 2y 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 2y 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.
ggm 2y 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.
- achillesheels 2y 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
- boxed 2y 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.
  - ggm 2y 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.
  - watwut 2y ago
nailer 2y 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
- CRConrad 2y 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".
- spc476 2y 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."
- sevnin 2y 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.
vips7L 2y 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 https://www.yegor256.com/2015/03/09/objects-end-with-er.html
- dominicrose 2y 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.
  - vips7L 2y 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 2y 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.
piotrkaminski 2y 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 2y 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.
- kolme 2y 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.
  - sevnin 2y 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.
    - gcarvalho 2y ago
      I think it’s widely accepted that refactors should not change behavior. That’s my experience, at least.
      - sevnin 2y ago
        Huh, I guess that's true, even on wikipedia. I will have to stop using the word then. Though I'm pretty sure most people use it in day-to-day with much more abandon.
azangru 2y 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 2y 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 2y ago
    > much more readable That’s down to preference. Doesnt both filter and map copy the array increasing gc pressure?
- kolme 2y 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.
  - azangru 2y 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 2y 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.
- CRConrad 2y 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.)
- exe34 2y 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 2y 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.
Jean-Papoulos 2y 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 2y 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 2y 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_Y 2y 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.
kolme 2y 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.
- ahoka 2y ago
  Developers regularly make code changes that change behavior and insist that it’s refactoring.
pif 2y 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!
pif 2y ago
Second example: doesn't the bad refactor also modify the list of users?
creesch 2y 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 2y 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_ 2y 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 2y ago
    Other goals can be performance, testability
    - _a_a_a_ 2y ago
      Testability is a good one, thanks. I'm not so sure about performance and I'll chew that one over for now.
      - eithed 2y ago
        Imagine that you've refactored code and reduced complexity, but also reduced performance (the caching example) - would you move forward with the refactor? 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
charlie0 2y 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 2y 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.
  - charlie0 2y 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.
    - sidmitra 2y ago
      I've had success with strategies at introducing some abstractions/patterns at my current place(doing this alone for a enterprise SaaS company with 200-ish devs). It's weird that we don't teach these or talk about them in software engineering(AFAIK). I see them being re-invented all the time. 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.
doctorM 2y 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 2y 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.
  - CRConrad 2y 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 2y 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 2y 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 2y 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 2y 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.
pensatoio 2y 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 2y ago
  What languages do you like to spend time in?
  - pensatoio 2y 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 2y 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 2y 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 2y 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 2y 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.
- CRConrad 2y 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 2y 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.
kjrfghslkdjfl 2y ago
[dead]
greenhearth 2y ago
Not bad, but then it turned out to be an ad for their AI thing
- CRConrad 2y ago
  Not particularly good either, so on the whole pretty much just an ad.
madcocomo 2y 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.

226 points by steve8708 2y 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 2y ago
Are you saying you find it hard to believe that some prefer the first "Before" example over the first "Bad refactor" example?
p2501 2y 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 2y 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 2y 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.
  - vasco 2y ago
    People think imperatively though. If I think of visiting my friend, grabbing gas on the way back, the way I'll visualize the steps is not functional.
    - The_Colonel 2y ago
      That's quite debatable. 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.
- The_Colonel 2y 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.
  - math_dandy 2y ago
    No need to fear mutable local state. Shared state is where immutable data structures really shine.
  - persnickety 2y ago
    > If I see "for (...", I only know that the subsequent code will iterate And then someone slaps do {} while(0) in a macro.
skybrian 2y 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_k 2y 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.
  - skybrian 2y ago
    Making analogies like this doesn't prove anything, they're just suggestive. All I'm getting out of this is that you think for loops are old-fashioned.
    - Jean-Papoulos 2y ago
      That's because they are. Functional code is more readable. And if you look back, basically all advances in programming languages have been about "making stuff more readable". Thus, for loops (for this usage) are "old".
      - CRConrad 2y ago
        > Functional code is more readable. All I get out of that is that you like functional code.
      - 2y ago
diatone 2y 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 2y 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 2y 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.

ggm 2y 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.

achillesheels 2y 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
boxed 2y 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.
- ggm 2y 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.
- watwut 2y ago

nailer 2y 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

CRConrad 2y 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".
spc476 2y 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."
sevnin 2y 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.

vips7L 2y 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 https://www.yegor256.com/2015/03/09/objects-end-with-er.html

dominicrose 2y 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.
- vips7L 2y 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 2y 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.

piotrkaminski 2y 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 2y 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.

kolme 2y 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.
- sevnin 2y 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.
  - gcarvalho 2y ago
    I think it’s widely accepted that refactors should not change behavior. That’s my experience, at least.
    - sevnin 2y ago
      Huh, I guess that's true, even on wikipedia. I will have to stop using the word then. Though I'm pretty sure most people use it in day-to-day with much more abandon.

azangru 2y 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 2y 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 2y ago
  > much more readable That’s down to preference. Doesnt both filter and map copy the array increasing gc pressure?
kolme 2y 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.
- azangru 2y 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 2y 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.

CRConrad 2y 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.)
exe34 2y 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 2y 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.

Jean-Papoulos 2y 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 2y 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 2y 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_Y 2y 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.

kolme 2y 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.

ahoka 2y ago
Developers regularly make code changes that change behavior and insist that it’s refactoring.

pif 2y 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!

pif 2y ago

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

creesch 2y 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 2y 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_ 2y 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 2y ago
  Other goals can be performance, testability
  - _a_a_a_ 2y ago
    Testability is a good one, thanks. I'm not so sure about performance and I'll chew that one over for now.
    - eithed 2y ago
      Imagine that you've refactored code and reduced complexity, but also reduced performance (the caching example) - would you move forward with the refactor? 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

charlie0 2y 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 2y 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.
- charlie0 2y 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.
  - sidmitra 2y ago
    I've had success with strategies at introducing some abstractions/patterns at my current place(doing this alone for a enterprise SaaS company with 200-ish devs). It's weird that we don't teach these or talk about them in software engineering(AFAIK). I see them being re-invented all the time. 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.

doctorM 2y 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 2y 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.
- CRConrad 2y 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 2y 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 2y 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 2y 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 2y 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.

pensatoio 2y 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 2y ago
What languages do you like to spend time in?
- pensatoio 2y 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 2y 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 2y 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 2y 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 2y 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.

CRConrad 2y 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 2y 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.

kjrfghslkdjfl 2y ago

[dead]

greenhearth 2y ago

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

CRConrad 2y ago
Not particularly good either, so on the whole pretty much just an ad.

madcocomo 2y 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.