I spent about a 12 year period of my career doing triage and app performance work. What struck me was how often the core problems were architectural. Either the design was bad from the beginning, or the requirements had changed so dramatically over time (think scaling, for example) that the architecture no longer worked. I bring this up because while I saw a lot of questionable code, at the micro level it really didn't matter. Sure it could have been refactored and improved, but that would have been essentially diminishing returns. Often the improvements that were needed would have been so painful that rather than make them, I watched companies spiral around either throwing hardware at problems or replacing the application entirely because that kind of Bandaid rip was deemed "easier" in the overall politics of the corporate world. So point being - sure, refactor what you can, but don't get too hung up on things that ultimately won't matter.
jamesfinlayson 138 days ago [-]
> What struck me was how often the core problems were architectural
Agreed - I remember at a previous job a new guy turned up and directed a rewrite of an old Perl application into a new Go version. He crowed on and on about how Go was so much faster than Perl but the underlying issue was the original architecture (old one was pinned to a single core and polled a database once a minute, the new one was dispatching requests pulled off a Kafka queue). The old one had been written when the system was handling less than a transaction a minute and the only attempts to fix it were upgrading Perl or various libraries.
lelanthran 141 days ago [-]
Simple and brief rules are more successful in practice than long and complicated rules.
I feel a briefer and more-to-the-point "When To Refactor" guide is to ask the following questions in the following order and only proceed when you can answer YES to every single question.
1. Do we have test coverage of the use-cases that are affected?
2. Are any non-trivial logic and business changes on the horizon for the code in question?
3. Has the code in question been undergoing multiple modifications in the last two/three/four weeks/months/years?
Honestly, if you answer NO to any of the questions above, you're in for a world of hurt and expense if you then proceed to refactor.
That last one might seem a bit of a reach, but the reality is that if there is some code in production that has been working unchanged for the last two years, you're wasting your time refactoring it.
More importantly, no changes over the last few years means that absolutely no one in the company has in-depth and current knowledge of how that code works, so a refactor is pointless because no one knows what the specific problems actually are.
PaulHoule 138 days ago [-]
I'd argue with that. Small-scale "micro-refactoring" operations are safe almost by definition. [1]
It depends on your language. In a large Python system [2], all bets are off, but in Java if you use your IDE to rename a method to have a clearer name or change the signature of a method or extract or inline a method the risk of breaking anything is close to zero whether or not you have tests.
Personally I think there is no conflict between feature work and micro-refactorings. If micro-refactorings are helpful for a feature you're working on, jump to it!
Personally I've had the experience of being a Cassandra [3] when it comes to YAGNI; [4] maybe it is different for a junior dev but so often I go to a meeting and say "what if you decide you need to collect N phone numbers instead of 2?" and over the next few months there is a stream of tickets for 3, 4, 5 phone numbers until they finally make it N. Or a problem w/ the login process that is obvious to me becomes the subject of a panic two months later.
As such I see the rule of three [5] to apply DRY is too conservative, it is way two common that the senior dev wrote two cases and then the junior dev comes in and copies it 15 times. At least on the projects I've worked on (mainly web-oriented, but many involving 'intelligent systems', data science, etc.) people have made way too many excuses for why repeating themselves is good and it has had awful consequences.
I'd extend #2 to any change, not just non-trivial. It's the classic Kent Beck tweet, "for any change, make the change easy (warning: this may be hard), then make the easy change"
Fixing my mental model of thinking about refactoring as a separate thing from "normal" development was key for me. Once I viewed refactoring as a thing that you do all the time as part of development, then I stopped even asking this question.
sshine 138 days ago [-]
I somehow ended up doing the same. Probably as a result of too many failed heroic attempts at modifying read-only code in one big go.
Now any heroic modification of code comes in a series of incontroversial, isolated and testably idempotent modifications, followed by a minimal change to business logic.
ClumsyPilot 138 days ago [-]
> More importantly, no changes over the last few years means that absolutely no one in the company has in-depth and current knowledge of how that code works, so a refactor is pointless because no one knows what the specific problems actually are.
reply
That’s is the actual reason you might need to rewrite it.
If your critical system is written in an ancient script that nobody can understand, and does no longer supported, and as a security risk, at some point it will simply stop working. And there will be nobody that can fix it.
Yes, rewrite may be painful, but if you can no longer find the people to support the old thing it may be necessary
lelanthran 136 days ago [-]
> If your critical system is written in an ancient script that nobody can understand, and does no longer supported, and as a security risk, at some point it will simply stop working. And there will be nobody that can fix it.
> Yes, rewrite may be painful, but if you can no longer find the people to support the old thing it may be necessary
I agree, but we weren't talking about rewrites, we were talking about refactors.
If you refactor some ancient old COBOL application, the result would be a new COBOL application.
If you rewrite some ancient old COBOL application, the result would most probably not be a new COBOL application.
poulsbohemian 138 days ago [-]
>the reality is that if there is some code in production that has been working unchanged for the last two years, you're wasting your time refactoring it.
So much this. I recall watching people early in their careers who wanted to make their mark go after code like this that was just a waste of time and more likely than not to blow up things downstream they didn't understand. And sadly watched managers praise their tenacity rather than understanding the explosions that were being created.
thomaslangston 138 days ago [-]
I'd say #2 and #3 need to be modified.
#2 Are there related changes on the horizon for the code being refactored?
I think more qualifications than that probably miss times you should refactor.
#3 Has the code been changing recently OR have changes been delayed because modifying the unrefactored code is considered too difficult.
I've seen too many times where unrefactored code is considered too dangerous/difficult to modify even with total test coverage. Refactoring is a necessary step towards self documenting code in those cases.
eyelidlessness 138 days ago [-]
One reason I might accept one or more NOs to your questions:
Does the refactor support pending work, which isn’t directly related to the refactored code, but benefits from the lessons learned and applied in the refactor… even in some indirect way?
This might be providing a clearer pattern you’ll apply to similar new functionality; or it might be providing a new abstraction or even eliminating a failed abstraction which sets that pending work on the right path.
AnimalMuppet 138 days ago [-]
Yeah. I did a (very small) refactor. It took, IIRC, four days. When I was done, I could write the new thing I was implementing in 10 new lines that used the newly-refactored existing code.
138 days ago [-]
scarface_74 138 days ago [-]
> Do we have test coverage of the use-cases that are affected?
With statically typed languages and good tooling like JetBrains ReSharper, there are guaranteed safe automated large refactorings that can be done as long as someone isn’t using reflection.m
rqmedes 138 days ago [-]
I regularly do large changes on many millions of lines legacy code bases with no unit tests. It can be done it just requires a lot of work. The only factor that matters is do we need to do this, the rest not so much
ninalanyon 138 days ago [-]
> 1. Do we have test coverage of the use-cases that are affected?
And
0. Do we know which use-cases will be affected?
andrei_says_ 141 days ago [-]
These are very simple and clear and confirm my own approach to refactoring. Thank you.
BeetleB 138 days ago [-]
Everyone talks about refactoring code. What I don't see enough discussion on is refactoring tests.
It's a constant battle at work, and I tend to be firm with: "If the test is working, no matter how horrible the (test) code, leave it be."[1]
For regular code, we rely on tests to validate our refactors. With test code, we don't have that support.
If you have a bunch of tests that could do with a refactor, and your motivation is that you need to write more tests, then set up whatever abstractions you need and write the new tests, but don't touch the existing tests! If you really want, you can overtest: Write new tests with your refactored code that test the same thing as the old ones. It's OK to test the same thing multiple times (when testing is cheap).
About a third of the time a coworker looks at my tests and refactors them, he makes an error - the most common one being not testing what the original test meant to test. See, the reason he wanted to refactor was that the original test was hard to follow. And because of that, he failed to interpret it correctly and failed to properly reproduce the test in his refactored code.
I then have to code review all his refactors. I have to spend time to figure out what my difficult-to-understand test did, and confirm he didn't introduce bugs. It's very, very tiring. And for what?
And he's not a newbie. He's as good at SW as I am. Plenty of experience. If he gets it wrong, I expect most people will get it wrong. With a higher error rate.
This is the one case where I say: "Feel free to overcomment." If you took a long time understanding a given test, write out what you learned so that the next time you read it, you know what it does. I'll be happy to code review that.
[1] Unless you are the original author of the tests, and they are still fresh in your mind. In that case, refactor all you want.
maples37 138 days ago [-]
For tests, when in doubt, I don't touch the original tests and just add my own separately. That way I know that the old tests still validate the originally-intended behavior, and my new tests validate what I expect them to do.
If the tests aren't broken, is there ever a good reason to make sweeping changes to them?
foo_barrio 138 days ago [-]
This is a good point! We've gotten some instances of Chesterson's Fence that some devs casually remove during a "test refactor" that later allowed a regression to make it into prod.
I've caught some errors in "test refactoring" from our multiple levels of testing having large overlap with teach other. Our end-to-end tests have a lot of overlap with the integration tests which in turn have a large overlap with unit testing. The unit tests run in a matter of seconds compared to the end-to-end which can take minutes or in some cases hours for our manual testing so the levels of testing also serve as an efficiency for us.
kylereeve 138 days ago [-]
When refactoring tests, some form of mutation coverage [0] would be really nice. Verify that the tests break when the underlying code changes.
Sounds like the tests would benefit from comments explaining what they are testing and why. Just a few days ago there was an article from an experienced dev that after multiple decades he thinks you simply can't put too many comments in tests. I was on the fence with it but seeing your comment, I now know where it comes from.
Maybe this could also be solved with better tooling - tests that allow substituting the tested system with a dummy that fails and thus verify the test is actually working.
BeetleB 138 days ago [-]
If the name is not descriptive enough, I put it in the test's docstring. Still, it sometimes is not enough.
I like languages where the test infrastructure lets you put an arbitrary string as the test name (e.g. F#, Elisp, Racket, etc). This way, you don't spend time thinking up a weird name for the test. You simply write a string that describes what you're testing, and what is expected. It's OK if that string is a few paragraphs long.
gbuk2013 138 days ago [-]
> If we’re embarking on a change that is not really refactoring (for example looking at a bug or an adjustment after a third party change), we can’t fix it with refactoring.
Funnily enough I did once fix a bug through refactoring. IIRC it was some intermittent race condition in some async code that I just couldn’t figure out, so I rewrote the code to do the same thing but simpler and the problem disappeared. I was too tired and relieved to continue figuring it out why it was originally broken.
zingar 133 days ago [-]
I suppose theoretically this must have been behaviour change that crept in with the refactoring, that luckily had a good result and not a bad one.
I get nervous about things that accidentally work but I definitely relate to being so tired or under too much pressure to move on that I just trust in luck.
That said, I would guess that even if the accidental behaviour change didn't occur, the bug would have been much easier to solve after the refactor made the intention of the code clearer.
TiredGuy 138 days ago [-]
Same. Sometimes the best way to fix a bug is to refactor it out of existence.
Salgat 138 days ago [-]
And if you're lucky a new hidden bug appears.
bob1029 138 days ago [-]
If it won't impact your margins or the customer's willingness to pay for the product then it is difficult to justify.
I've been involved with (and responsible for) many "developer aesthetic" refactors over the years. They feel good in the moment but after blowing 2-3 weeks with regression testing, hot patches, etc., you start to wonder if it was all worth it.
There is stuff that is properly nasty and needs to be dealt with, but if you are spinning your wheels on things like namespaces being stuck on old company/product names, I would just give it up and move on. The average customer cannot see any of the things that frustrate us unless they are being done very poorly.
magicalhippo 138 days ago [-]
We've got a lot of 20+ year old code in production, a lot written late at night in a crunch. Much of it's ugly, non-optimal and screams to be ripped out and replaced.
But it is working, in areas that see little change and are not overly performance sensitive.
So we only do something with it if we need larger changes. Otherwise we leave it be and spend our time being productive elsewhere.
gspencley 138 days ago [-]
> We often reach a point during refactoring where it seems like there is an easy improvement that applies to almost all cases. It’s usually better not to impose additional abstraction if it only matches “almost” all cases.
Need to nit-pick on this one. There are design patterns, depending on your programming paradigm, that enable you to de-dupe the common stuff while allowing for all of those little variances to do their own unique thing without duplicating anything.
In OOP the Template Method pattern comes to mind.
I would even go so far as to say that the "almost all cases" problem is such a common problem in software development, that the patterns world has come up with various solutions to that which are intended to simplify, not complicate.
I'm sure there are concrete examples where there are no good pre-existing solutions, and there are often examples where cures can be worse than the diseases and you need to make a judgement call. But don't give yourself an excuse to duplicate code because you don't know how to de-dupe in the "almost all cases" scenario. That's a very solved problem.
hylaride 138 days ago [-]
Refactoring depends heavily on circumstances. I've seen terrible attempts over my career for all the reasons other people have and will mention here.
But I've also seen huge successes. Usually it boils down to "what are you trying to accomplish?" against current code and architecture. I've seen targeted refactors of micro-service code, including language shifts (eg python -> java where threading improved things a lot in that use case), as well as targeted shifts of code to AWS Lambdas, where execution scaling also benefited the use case. But in both examples there was a clear benefit, execution plan, and way to measure success.
When you start refactoring large mono-app code bases for nebulous, hard to measure reasons then the risks are much higher (and that's even before scope creep comes into play). This gets even worse if the reason the current code is no longer desirable has reasons (both good and bad) that it is the way it is, and you risk recreating bad code (this was mentioned in the article) into new bad code. Also, migrations to new code can suck if the underlying dependencies, including data(bases) is just as much part of the problem?
mont_tag 138 days ago [-]
One more consideration is whether a team of people have invested time building a mental map of the current code. Perhaps some rearrangement would be better, faster, clearer, or have some other virtue. But that benefit has to be balanced against invalidating the mental caches of all the other team members.
More than once, I've studied code and become adept at maintaining it. Then someone "refactors" everything and I have to invest the effort again. After a few cycles of this, I lose interest in ever seeing the code again. At that point, I can no longer orient a new dev to the code because I'm no longer oriented myself.
zingar 133 days ago [-]
Great point about team mental context!
Without knowing the context I would guess that this a bad refactor (away from revealing the intention)... probably including some cases of applying new abstractions that don't fit the real problem. How does that match with your experience?
Update: it occurs to me that I have seen complex code (for which people would need a mental map) be simplified so drastically that no-one needs to worry about mental maps. I would much prefer "no-one needs a mental map" over "there's a team with good mental maps".
ozim 138 days ago [-]
That is a pain with new guys on team that don’t want to spend time learning the code but rather rewrite it their way so they can understand it.
Only problem is that’s ultimately not polite to existing team and hopefully we can filter such people in hiring process but not always.
taeric 138 days ago [-]
I like these reasons.
I will offer some caution on rules for rejecting a proposal. Often times, the number one reason to do something is that there is energy there to do it. If you can take that energy and apply it somewhere else, by all means do so. All too often, suppressing some energy stops all effort.
tikhonj 138 days ago [-]
> There’s no benefit to improving code that never changes, even if it is highly complex.
Only if you don't care about being able to understand and debug the code in the future!
And even then it's if you're confident in predicting how much code will or won't need to change, which, well...
mason55 138 days ago [-]
The point is not to improve it until you actually need to change it.
Like you said, you're probably bad at predicting which code is going to need changes in the future, so if it's working now and you don't need to change it then it's a silly risk to try to improve it right now.
einpoklum 138 days ago [-]
Peeves about the items in the linked post:
> 1. The change is not really a refactor
Is anything ever really just a refactor, though?
> 3. Another refactor is already in progress
Ah, but - in large and poorly-maintained codebase, it is always the case that some refactoring is already in progress, likely stalled for weeks, months or years as priorities have shifted without it having been completed.
> 4. The code is unlikely to change
If the code is unlikely to change, there's a good chance you might want to refactor it out of your repository anyway, into some low-frequency-of-changing library.
> There’s no benefit to improving code that never changes
Oh, there are lots of benefits to improving code that never changes! And that's because other code _uses_ the code which never changes. And if originally the use is clunky, but you manage to improve it by your refactor, youve' done well indeed.
> 5. There are no tests
A repository which lacks test coverage is extremely likely to be in dire need of all kinds of refactoring. Waiting until someone (maybe even yourtself) writes tests for most code, before doing anything in the mean time - well, it basically means giving up on yourself.
> we can start by writing some tests!
Well, that's nice, but - if we do that, then no refactoring would get done, and we'll have to write code based on ugly and unweildy older code, which only adds stuff to refactor later. Plus, who's to say the tests don't fail _already_? :-(
Fishkins 138 days ago [-]
> Ah, but - in large and poorly-maintained codebase, it is always the case that some refactoring is already in progress, likely stalled for weeks, months or years as priorities have shifted without it having been completed.
This is true, and something I also thought of when reading that point. I don't think it's necessarily a counterargument, though. It's probably a better idea to spend your time helping to complete the previous refactor instead of starting your new one. Codebases in which many refactorings are started but not completed can be worse than ones that aren't refactored at all.
There could be exceptions if your new changes is very small, localized, and unlikely to interfere with the other changes going on.
einpoklum 137 days ago [-]
> It's probably a better idea to spend your time helping to complete
> the previous refactor instead of starting your new one
That is a fair suggestion - when practicable. Often, it isn't, because of the very reasons which made the old refactor stall in the first place, e.g.:
* The target factor of the code was not such a good choice to begin with.
* The code in question is critical and any change to it is discouraged or feared.
* The code in question is the responsibility of different groups of developers / different departments, and they, or their managers, are not in the mood for collaboration.
etc.
zingar 133 days ago [-]
Original author here :)
> Is anything ever really just a refactor, though?
I think you're making the point that very few worthwhile business goals could be achieved purely by refactoring. Which is true.
Some part of the work will probably be the safe mechanical steps that make up refactoring ("replace conditional with method", "extract method" etc), and some part is "normal" "non-refactoring" change.
The reason for this point is that I see developers avoid important conversations because they claim they're "just refactoring" and so they can safely make a change without involving anyone else. They're wrong unless they have a good handle on the difference between the two types of work.
> it is always the case that some refactoring is already in progress
When I've seen the "paused refactor" phenomenon, it isn't actually refactoring ... there's negotiation and consideration of trade-offs and impacts on various parties, because some important parts of the code need to actually do something different. But probably there are more cases in the world than I've personally seen.
But really in this case I am thinking on the level of developers who will extract a method while at the same time changing the content of the code being replaced. Without running any tests, committing etc. The start and end of the one step is easily in their grasp, but they think they can save time by mixing two steps at once.
> If the code is unlikely to change, there's a good chance you might want to refactor it out of your repository
Interesting point. The cases that were in my head were more like unique things that wouldn't group together well with anything else. What kind of code do you typically move out of a repo?
Would it be fair to say that in the case of extracting unchanging code, we're recognising that there is some difference between our intentions for the code that stays and the code that doesn't? And that by extracting it we're acting on a recognition that the intention has changed since we first wrote it?
> other code _uses_ the code which never changes... And if originally the use is clunky, but you manage to improve it by your refactor
Wouldn't this be more like changing my [previously] unchanging code to make it better fit the use cases of the client?
> Waiting until someone (maybe even yourtself) writes tests for most code
I wouldn't recommend writing tests for most of the code before any refactoring. There is certainly some small part that can be tested and then refactored while the rest stays untested and unchanged. Then on to the next test and refactor and so on.
> who's to say the tests don't fail _already_?
I guess I didn't mention this case explicitly, but I'd expect someone to fix the tests (or scrap the old and write _some_ new) and then get on with refactoring.
madmountaingoat 138 days ago [-]
It's all solid advice but that exchange in #5 that the author found delightful, drives me bonkers. If you talk to the people on your team like that, you're not being helpful, you're being an a-hole. I realize its just making a point in the context of the article, but this industry seems to collect pedants, so it seems worth pointing out as an anti-pattern.
zingar 127 days ago [-]
Article author here, this is a really good point and I can see how that exchange might undermine the point (or even the entire article) for some readers.
I think I understand it as fighting fire with fire: that people who are enthusiastic about refactoring without understanding and writing tests are prone to pushing their agenda without really engaging on the substantive points (as in someone who talks "refactoring" without taking in how central automated tests are in the refactoring community). Such a person sometimes needs a confrontational push to go back to the basics on this point.
GiorgioG 138 days ago [-]
Does anyone really get time to refactor anything these days? Seems like (at least where I'm at), everything is go-go-go, feature work.
foo_barrio 138 days ago [-]
I try to never ever use the word "refactor" or "clean up" in any of my work items or even commit messages. My boss and other teams I am collaborating with are like clients to me. Restaurant bills typically do not include explicit line items for "dish washing, mopping, grease-trap cleaning, oil disposal, etc."
The downside is that other people/teams can appear to work much faster and put you at a disadvantage or even risk of being let go due to this perception. Unfortunately this is where IMO you have to play the game and make sure you toot your own horn to the right people sometimes.
maccard 138 days ago [-]
If you work in a kitchen, you’re expected to keep the kitchen clean. The trick to that is knowing how to efficiently clear up, and being prepared for the waves when they come.
As an engineer it’s no different. If you’re not keeping in top of your own debt, you’re actually falling behind.
IshKebab 138 days ago [-]
Yeah just do it as part of your work without telling people. You're paid to write software and part of writing software is refactoring.
karmakaze 140 days ago [-]
I have one reason to refactor that's served me well. Does the code exhibit ongoing edge/corner case bugs, which are attributed to the implementation not being factored in the first place? Another sign of this situation is exhaustive test cases which combine factors as they can't reliably be tested in isolation. Distributed logic is another common pattern, where a new feature is made by sprinkling bits of logic here and there, just so, and it all seems to do what's required. You know the kind, where onboarding a new person might lead to the discussion of how this section of your Rube Goldberg contraption works. A fancy way to this world is a poorly designed and maintained home-grown DSL.
Continuing to build on such a 'foundation' is madness that spirals to code red situations: stop-the-world, no feature development, until shit gets stable.
PaulHoule 138 days ago [-]
The default-oriented answer to #2 is to pass the rows through some function that applies the default to 'loud' if loud is undefined. That's really pretty code and avoids the complex if-statement which is a universal bad smell.
kevmojay138 138 days ago [-]
> Instead of “refactoring” without tests, we can start by writing some tests!
I agree with this but I have always struggled with putting it into practice. Good code is easy to test. Often, I want to refactor to make it easy to test.
merb 138 days ago [-]
When working with git it’s better to split refactorings, bug fixes and features per commit or merge request (if you squash) else your git history will be not so useful anymore.
patrick451 138 days ago [-]
There are two conflicting rules of thumb floating around here
1. If hasn't changed in a long time and no business requirement on the horizon will force it to change, don't refactor.
2. Code is read more than written, so optimize for readability.
(1) can only be followed to the detriment of (2), unless you add the condition that the code is also code which developers never need to read and understand to implement other features.
137 days ago [-]
darioush 141 days ago [-]
another reason to not refactor is management will start out supportive of it but some weeks in they will wonder wtf their engineers are doing all day and why aren't they building the new pile of stuff they want built yesterday.
zingar 131 days ago [-]
I do encounter hostility but what I've realised is that it comes from experience of refactoring being done badly. "We made time for this refactoring thing but it made no difference so now we're not going to 'refactor' anymore".
The worst hostility is often from management that used to be technical but never learned how to refactor effectively so they have what they think is justifiable scepticism. s/refactoring/pairing or any other technique that is good in theory but requires skill to implement.
I usually find that people without technical background are willing to follow the advice of the developers that work with them, taking the things that work with them into their next roles. If refactoring worked in the beginning they'll be open to it in future, if it didn't work they'll dismiss it.
Edit: also - what everyone else said - "refactoring" isn't a business goal, it's one part of achieving the goal. Achieve the goal and no-one cares how you did it. ("add a new button to that widget" isn't a business goal either so there is a tricky aspect of getting management to understand that they should be bringing you problems/goals not solutions)
svachalek 138 days ago [-]
Refactoring is a terrible project plan. You never want your weekly status to be "refactored some more". Plan your other development work with enough time to refactor as you go.
eyelidlessness 138 days ago [-]
That’s not so much a reason, in my experience, as a frequent default/inertia position. In other words, I expect to start at that default, and challenge it with reasoning, in order to justify a significant refactor.
lesuorac 138 days ago [-]
The ability to ship small things is I think something really missed in discussion about microservices.
Like rather then refactoring the entire application to support a faster cache or something you can just add in another microservice that handles caching of the data.
If some way you write the services is poor you can rewrite one of the services a new way and easily evaluate if it's actually a good idea to change the rest or not.
IshKebab 138 days ago [-]
Don't spend 100% of your time refactoring.
xg15 138 days ago [-]
> “Before we can add tests, we need to refactor.” - “No.” - “Refactoring th–” - “No.” - “Refa–” - “No.”
What to do instead: Instead of “refactoring” without tests, we can start by writing some tests! Writing new tests will help us learn about the code. After that, we can come back to refactoring.
I assume the author means system tests here? Because it's absolutely possible to have code that is so entangled that it's impossible to write self-contained unit tests before, well, refactoring the code.
zingar 127 days ago [-]
I mean tests of any kind. The main reason I see for tests not existing is because the engineers don't have the skill to write effective tests fast enough (and after a while in that state, just deciding that tests aren't worth it). The code not being testable is a distant second or third.
einpoklum 138 days ago [-]
My top reasons not to refactor:
1. My boss won't let me. The usual excuse: "Too risky... there are other priorities... and besides, where are those performance improvements you promised me? We don't pay you to make the code pretty."
2. Boss of group which owns some of the code I need to refactor won't let me: "Too risky... we have other priorities... have your boss bring this up in the next [bogus] meeting"
3. Other developers get panties in a bunch over a tiny refactor (semantically) but which involves changes to names in a lot of files.
4. Knowing that if I'd start this refactor I will not be able to resist another refactor until I'm basically refactoring people's entire code to look quite different.
Agreed - I remember at a previous job a new guy turned up and directed a rewrite of an old Perl application into a new Go version. He crowed on and on about how Go was so much faster than Perl but the underlying issue was the original architecture (old one was pinned to a single core and polled a database once a minute, the new one was dispatching requests pulled off a Kafka queue). The old one had been written when the system was handling less than a transaction a minute and the only attempts to fix it were upgrading Perl or various libraries.
I feel a briefer and more-to-the-point "When To Refactor" guide is to ask the following questions in the following order and only proceed when you can answer YES to every single question.
1. Do we have test coverage of the use-cases that are affected?
2. Are any non-trivial logic and business changes on the horizon for the code in question?
3. Has the code in question been undergoing multiple modifications in the last two/three/four weeks/months/years?
Honestly, if you answer NO to any of the questions above, you're in for a world of hurt and expense if you then proceed to refactor.
That last one might seem a bit of a reach, but the reality is that if there is some code in production that has been working unchanged for the last two years, you're wasting your time refactoring it.
More importantly, no changes over the last few years means that absolutely no one in the company has in-depth and current knowledge of how that code works, so a refactor is pointless because no one knows what the specific problems actually are.
It depends on your language. In a large Python system [2], all bets are off, but in Java if you use your IDE to rename a method to have a clearer name or change the signature of a method or extract or inline a method the risk of breaking anything is close to zero whether or not you have tests.
Personally I think there is no conflict between feature work and micro-refactorings. If micro-refactorings are helpful for a feature you're working on, jump to it!
Personally I've had the experience of being a Cassandra [3] when it comes to YAGNI; [4] maybe it is different for a junior dev but so often I go to a meeting and say "what if you decide you need to collect N phone numbers instead of 2?" and over the next few months there is a stream of tickets for 3, 4, 5 phone numbers until they finally make it N. Or a problem w/ the login process that is obvious to me becomes the subject of a panic two months later.
As such I see the rule of three [5] to apply DRY is too conservative, it is way two common that the senior dev wrote two cases and then the junior dev comes in and copies it 15 times. At least on the projects I've worked on (mainly web-oriented, but many involving 'intelligent systems', data science, etc.) people have made way too many excuses for why repeating themselves is good and it has had awful consequences.
[1] https://en.wikipedia.org/wiki/Code_refactoring
[2] the best case for Python is that a large system in some other language could be a small system in Python
[3] https://en.wikipedia.org/wiki/Cassandra
[4] https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it
[5] https://en.wikipedia.org/wiki/Rule_of_three_(computer_progra...
Fixing my mental model of thinking about refactoring as a separate thing from "normal" development was key for me. Once I viewed refactoring as a thing that you do all the time as part of development, then I stopped even asking this question.
Now any heroic modification of code comes in a series of incontroversial, isolated and testably idempotent modifications, followed by a minimal change to business logic.
That’s is the actual reason you might need to rewrite it. If your critical system is written in an ancient script that nobody can understand, and does no longer supported, and as a security risk, at some point it will simply stop working. And there will be nobody that can fix it.
Yes, rewrite may be painful, but if you can no longer find the people to support the old thing it may be necessary
> Yes, rewrite may be painful, but if you can no longer find the people to support the old thing it may be necessary
I agree, but we weren't talking about rewrites, we were talking about refactors.
If you refactor some ancient old COBOL application, the result would be a new COBOL application.
If you rewrite some ancient old COBOL application, the result would most probably not be a new COBOL application.
So much this. I recall watching people early in their careers who wanted to make their mark go after code like this that was just a waste of time and more likely than not to blow up things downstream they didn't understand. And sadly watched managers praise their tenacity rather than understanding the explosions that were being created.
#2 Are there related changes on the horizon for the code being refactored?
I think more qualifications than that probably miss times you should refactor.
#3 Has the code been changing recently OR have changes been delayed because modifying the unrefactored code is considered too difficult.
I've seen too many times where unrefactored code is considered too dangerous/difficult to modify even with total test coverage. Refactoring is a necessary step towards self documenting code in those cases.
Does the refactor support pending work, which isn’t directly related to the refactored code, but benefits from the lessons learned and applied in the refactor… even in some indirect way?
This might be providing a clearer pattern you’ll apply to similar new functionality; or it might be providing a new abstraction or even eliminating a failed abstraction which sets that pending work on the right path.
With statically typed languages and good tooling like JetBrains ReSharper, there are guaranteed safe automated large refactorings that can be done as long as someone isn’t using reflection.m
And
0. Do we know which use-cases will be affected?
It's a constant battle at work, and I tend to be firm with: "If the test is working, no matter how horrible the (test) code, leave it be."[1]
For regular code, we rely on tests to validate our refactors. With test code, we don't have that support.
If you have a bunch of tests that could do with a refactor, and your motivation is that you need to write more tests, then set up whatever abstractions you need and write the new tests, but don't touch the existing tests! If you really want, you can overtest: Write new tests with your refactored code that test the same thing as the old ones. It's OK to test the same thing multiple times (when testing is cheap).
About a third of the time a coworker looks at my tests and refactors them, he makes an error - the most common one being not testing what the original test meant to test. See, the reason he wanted to refactor was that the original test was hard to follow. And because of that, he failed to interpret it correctly and failed to properly reproduce the test in his refactored code.
I then have to code review all his refactors. I have to spend time to figure out what my difficult-to-understand test did, and confirm he didn't introduce bugs. It's very, very tiring. And for what?
And he's not a newbie. He's as good at SW as I am. Plenty of experience. If he gets it wrong, I expect most people will get it wrong. With a higher error rate.
This is the one case where I say: "Feel free to overcomment." If you took a long time understanding a given test, write out what you learned so that the next time you read it, you know what it does. I'll be happy to code review that.
[1] Unless you are the original author of the tests, and they are still fresh in your mind. In that case, refactor all you want.
If the tests aren't broken, is there ever a good reason to make sweeping changes to them?
I've caught some errors in "test refactoring" from our multiple levels of testing having large overlap with teach other. Our end-to-end tests have a lot of overlap with the integration tests which in turn have a large overlap with unit testing. The unit tests run in a matter of seconds compared to the end-to-end which can take minutes or in some cases hours for our manual testing so the levels of testing also serve as an efficiency for us.
[0] https://en.wikipedia.org/wiki/Mutation_testing
Maybe this could also be solved with better tooling - tests that allow substituting the tested system with a dummy that fails and thus verify the test is actually working.
I like languages where the test infrastructure lets you put an arbitrary string as the test name (e.g. F#, Elisp, Racket, etc). This way, you don't spend time thinking up a weird name for the test. You simply write a string that describes what you're testing, and what is expected. It's OK if that string is a few paragraphs long.
Funnily enough I did once fix a bug through refactoring. IIRC it was some intermittent race condition in some async code that I just couldn’t figure out, so I rewrote the code to do the same thing but simpler and the problem disappeared. I was too tired and relieved to continue figuring it out why it was originally broken.
I get nervous about things that accidentally work but I definitely relate to being so tired or under too much pressure to move on that I just trust in luck.
That said, I would guess that even if the accidental behaviour change didn't occur, the bug would have been much easier to solve after the refactor made the intention of the code clearer.
I've been involved with (and responsible for) many "developer aesthetic" refactors over the years. They feel good in the moment but after blowing 2-3 weeks with regression testing, hot patches, etc., you start to wonder if it was all worth it.
There is stuff that is properly nasty and needs to be dealt with, but if you are spinning your wheels on things like namespaces being stuck on old company/product names, I would just give it up and move on. The average customer cannot see any of the things that frustrate us unless they are being done very poorly.
But it is working, in areas that see little change and are not overly performance sensitive.
So we only do something with it if we need larger changes. Otherwise we leave it be and spend our time being productive elsewhere.
Need to nit-pick on this one. There are design patterns, depending on your programming paradigm, that enable you to de-dupe the common stuff while allowing for all of those little variances to do their own unique thing without duplicating anything.
In OOP the Template Method pattern comes to mind.
I would even go so far as to say that the "almost all cases" problem is such a common problem in software development, that the patterns world has come up with various solutions to that which are intended to simplify, not complicate.
I'm sure there are concrete examples where there are no good pre-existing solutions, and there are often examples where cures can be worse than the diseases and you need to make a judgement call. But don't give yourself an excuse to duplicate code because you don't know how to de-dupe in the "almost all cases" scenario. That's a very solved problem.
But I've also seen huge successes. Usually it boils down to "what are you trying to accomplish?" against current code and architecture. I've seen targeted refactors of micro-service code, including language shifts (eg python -> java where threading improved things a lot in that use case), as well as targeted shifts of code to AWS Lambdas, where execution scaling also benefited the use case. But in both examples there was a clear benefit, execution plan, and way to measure success.
When you start refactoring large mono-app code bases for nebulous, hard to measure reasons then the risks are much higher (and that's even before scope creep comes into play). This gets even worse if the reason the current code is no longer desirable has reasons (both good and bad) that it is the way it is, and you risk recreating bad code (this was mentioned in the article) into new bad code. Also, migrations to new code can suck if the underlying dependencies, including data(bases) is just as much part of the problem?
More than once, I've studied code and become adept at maintaining it. Then someone "refactors" everything and I have to invest the effort again. After a few cycles of this, I lose interest in ever seeing the code again. At that point, I can no longer orient a new dev to the code because I'm no longer oriented myself.
Without knowing the context I would guess that this a bad refactor (away from revealing the intention)... probably including some cases of applying new abstractions that don't fit the real problem. How does that match with your experience?
Update: it occurs to me that I have seen complex code (for which people would need a mental map) be simplified so drastically that no-one needs to worry about mental maps. I would much prefer "no-one needs a mental map" over "there's a team with good mental maps".
Only problem is that’s ultimately not polite to existing team and hopefully we can filter such people in hiring process but not always.
I will offer some caution on rules for rejecting a proposal. Often times, the number one reason to do something is that there is energy there to do it. If you can take that energy and apply it somewhere else, by all means do so. All too often, suppressing some energy stops all effort.
Only if you don't care about being able to understand and debug the code in the future!
And even then it's if you're confident in predicting how much code will or won't need to change, which, well...
Like you said, you're probably bad at predicting which code is going to need changes in the future, so if it's working now and you don't need to change it then it's a silly risk to try to improve it right now.
> 1. The change is not really a refactor
Is anything ever really just a refactor, though?
> 3. Another refactor is already in progress
Ah, but - in large and poorly-maintained codebase, it is always the case that some refactoring is already in progress, likely stalled for weeks, months or years as priorities have shifted without it having been completed.
> 4. The code is unlikely to change
If the code is unlikely to change, there's a good chance you might want to refactor it out of your repository anyway, into some low-frequency-of-changing library.
> There’s no benefit to improving code that never changes
Oh, there are lots of benefits to improving code that never changes! And that's because other code _uses_ the code which never changes. And if originally the use is clunky, but you manage to improve it by your refactor, youve' done well indeed.
> 5. There are no tests
A repository which lacks test coverage is extremely likely to be in dire need of all kinds of refactoring. Waiting until someone (maybe even yourtself) writes tests for most code, before doing anything in the mean time - well, it basically means giving up on yourself.
> we can start by writing some tests!
Well, that's nice, but - if we do that, then no refactoring would get done, and we'll have to write code based on ugly and unweildy older code, which only adds stuff to refactor later. Plus, who's to say the tests don't fail _already_? :-(
This is true, and something I also thought of when reading that point. I don't think it's necessarily a counterargument, though. It's probably a better idea to spend your time helping to complete the previous refactor instead of starting your new one. Codebases in which many refactorings are started but not completed can be worse than ones that aren't refactored at all.
There could be exceptions if your new changes is very small, localized, and unlikely to interfere with the other changes going on.
> the previous refactor instead of starting your new one
That is a fair suggestion - when practicable. Often, it isn't, because of the very reasons which made the old refactor stall in the first place, e.g.:
* The target factor of the code was not such a good choice to begin with.
* The code in question is critical and any change to it is discouraged or feared.
* The code in question is the responsibility of different groups of developers / different departments, and they, or their managers, are not in the mood for collaboration.
etc.
> Is anything ever really just a refactor, though?
I think you're making the point that very few worthwhile business goals could be achieved purely by refactoring. Which is true.
Some part of the work will probably be the safe mechanical steps that make up refactoring ("replace conditional with method", "extract method" etc), and some part is "normal" "non-refactoring" change.
The reason for this point is that I see developers avoid important conversations because they claim they're "just refactoring" and so they can safely make a change without involving anyone else. They're wrong unless they have a good handle on the difference between the two types of work.
> it is always the case that some refactoring is already in progress
When I've seen the "paused refactor" phenomenon, it isn't actually refactoring ... there's negotiation and consideration of trade-offs and impacts on various parties, because some important parts of the code need to actually do something different. But probably there are more cases in the world than I've personally seen.
But really in this case I am thinking on the level of developers who will extract a method while at the same time changing the content of the code being replaced. Without running any tests, committing etc. The start and end of the one step is easily in their grasp, but they think they can save time by mixing two steps at once.
> If the code is unlikely to change, there's a good chance you might want to refactor it out of your repository
Interesting point. The cases that were in my head were more like unique things that wouldn't group together well with anything else. What kind of code do you typically move out of a repo?
Would it be fair to say that in the case of extracting unchanging code, we're recognising that there is some difference between our intentions for the code that stays and the code that doesn't? And that by extracting it we're acting on a recognition that the intention has changed since we first wrote it?
> other code _uses_ the code which never changes... And if originally the use is clunky, but you manage to improve it by your refactor
Wouldn't this be more like changing my [previously] unchanging code to make it better fit the use cases of the client?
> Waiting until someone (maybe even yourtself) writes tests for most code
I wouldn't recommend writing tests for most of the code before any refactoring. There is certainly some small part that can be tested and then refactored while the rest stays untested and unchanged. Then on to the next test and refactor and so on.
> who's to say the tests don't fail _already_?
I guess I didn't mention this case explicitly, but I'd expect someone to fix the tests (or scrap the old and write _some_ new) and then get on with refactoring.
I think I understand it as fighting fire with fire: that people who are enthusiastic about refactoring without understanding and writing tests are prone to pushing their agenda without really engaging on the substantive points (as in someone who talks "refactoring" without taking in how central automated tests are in the refactoring community). Such a person sometimes needs a confrontational push to go back to the basics on this point.
The downside is that other people/teams can appear to work much faster and put you at a disadvantage or even risk of being let go due to this perception. Unfortunately this is where IMO you have to play the game and make sure you toot your own horn to the right people sometimes.
As an engineer it’s no different. If you’re not keeping in top of your own debt, you’re actually falling behind.
Continuing to build on such a 'foundation' is madness that spirals to code red situations: stop-the-world, no feature development, until shit gets stable.
I agree with this but I have always struggled with putting it into practice. Good code is easy to test. Often, I want to refactor to make it easy to test.
1. If hasn't changed in a long time and no business requirement on the horizon will force it to change, don't refactor. 2. Code is read more than written, so optimize for readability.
(1) can only be followed to the detriment of (2), unless you add the condition that the code is also code which developers never need to read and understand to implement other features.
The worst hostility is often from management that used to be technical but never learned how to refactor effectively so they have what they think is justifiable scepticism. s/refactoring/pairing or any other technique that is good in theory but requires skill to implement.
I usually find that people without technical background are willing to follow the advice of the developers that work with them, taking the things that work with them into their next roles. If refactoring worked in the beginning they'll be open to it in future, if it didn't work they'll dismiss it.
Edit: also - what everyone else said - "refactoring" isn't a business goal, it's one part of achieving the goal. Achieve the goal and no-one cares how you did it. ("add a new button to that widget" isn't a business goal either so there is a tricky aspect of getting management to understand that they should be bringing you problems/goals not solutions)
Like rather then refactoring the entire application to support a faster cache or something you can just add in another microservice that handles caching of the data.
If some way you write the services is poor you can rewrite one of the services a new way and easily evaluate if it's actually a good idea to change the rest or not.
What to do instead: Instead of “refactoring” without tests, we can start by writing some tests! Writing new tests will help us learn about the code. After that, we can come back to refactoring.
I assume the author means system tests here? Because it's absolutely possible to have code that is so entangled that it's impossible to write self-contained unit tests before, well, refactoring the code.
1. My boss won't let me. The usual excuse: "Too risky... there are other priorities... and besides, where are those performance improvements you promised me? We don't pay you to make the code pretty."
2. Boss of group which owns some of the code I need to refactor won't let me: "Too risky... we have other priorities... have your boss bring this up in the next [bogus] meeting"
3. Other developers get panties in a bunch over a tiny refactor (semantically) but which involves changes to names in a lot of files.
4. Knowing that if I'd start this refactor I will not be able to resist another refactor until I'm basically refactoring people's entire code to look quite different.
0. https://bensguide.substack.com/i/144306439/local-symmetries