72

I have been in two software product houses for three years in a row.

The first is a small company maintaining a fairly small management system with a monolithic legacy code base (almost twenty years). Tightly coupled code is everywhere without sufficient unit test coverage. However, the management usually does not want developers to refactor the legacy code.

The second is a fairly big company maintaining a big domain-specific system with a huge monolithic Java legacy code base (over ten years). The layered architecture indeed decoupled the infrastructure from the business logic. However, in their business layer, there are also some giant classes with more than 3 thousand lines of code. Developers still continuously inject more and more code into those legacy classes. Developers are allowed to refactor their own fairly new code about adding new features, but are warned not to refactor these giant spaghetti classes, either. Experienced senior developers say that changes or refactoring on those classes might be disastrous due to the lack of regression tests.

However, personally I have read practical books about clean code and refactoring. Most of the books strongly recommend developers to refactor actively. But why in real world companies are against this?

So I would like to collect answers from very experienced developers. Why do these two companies I was in prefer to keep the super legacy code unrefactored? Isn't this disastrous?

  • 59
    Most of the books strongly recommend developers to refactor actively. But why in real world companies are against this? -- You already know the answer to this. The code isn't covered by unit tests, and it's too brittle to refactor without unit tests to cover it, so instead of doing it the right way and spending the money to write unit tests and refactor, the companies simply say "Do not touch it." It's not necessarily a bad strategy; if the code works, and it's never touched again, then it's never going to break, is it? (fingers crossed). – Robert Harvey Sep 25 at 21:45
  • 17
    Has anything bad happened? – Robert Harvey Sep 25 at 21:47
  • 58
    There are books, and there is reality. – Robert Harvey Sep 25 at 21:51
  • 23
    One thing that some of the answers hint at, but don't directly state is that the existing application is not just an application. It also contains the only documentation for probably thousands of requirements that were implemented, but never written down in the project documentation. It is the product of hundreds of thousands or even millions of hours of testing by real world usage. Nothing that you can do can allow large-scale refactoring without risking breaking things. Sometimes, in coding, two wrongs can make a right, and if you fix one of the wrongs, and not the other, you break things. – Itsme2003 Sep 27 at 20:23
  • 19
    I have yet to see the testing (automated or not) which can compare to running ten years in a live environment. Nothing you can build in a year will nearly be as though as a code having been challenged and fixed for ten years - it has seen corner cases you can't dream of. - It can still be better to build something new, just know that it will take time to become as resilient and battle-hardened as the old junk. – Falco Sep 28 at 12:56
117

It‘s a question of risk management:

  • Refactoring a system always creates the risk of breaking something that worked before.

  • The larger the system, the higher its complexity, and the higher the risk of breaking something.

  • With spaghetti-code (or any other poorly structured code) the real structure of the code remains fuzzy, and the dependencies might be hidden. Any change in one place could easily have impacts anywhere else. This increases the risks of breaking something to the highest level.

  • With TDD, or any other technique guaranteeing a comprehensive set of test cases, you can quickly verify that refactored parts (and dependent parts) still work. Of course, this is effective only with the help of proper encapsulation.

  • Unfortunately, tests are often missing for legacy code, or their coverage or depth is insufficient.

In other words, with large legacy spaghetti code bases, refactoring creates a high risk of breaking something that worked before, and the impact of this risk cannot be reduced with automated tests. The significant risk of refactoring simply outweighs refactoring benefits in this case.

Additional remark: An alternative approach at a lower risk is: don't touch the running system, but implement new and replaced features with state of the art testable code and clear boundaries. This more evolutionary approach is not always feasible, but it can provide significant short term and long term benefits.

| improve this answer | |
  • 4
    Well said, and I would add that we as software developers often adopt an all-or-nothing stance on software quality that is unlikely to be appreciated by the business folks. A perfectly-designed, state-of-the-art system makes a lot of money, but a crappy, outdated system still makes some money - money that is at risk of stopping flowing when we tinker with the existing codebase too much in the name of code quality – crizzis Sep 26 at 16:57
  • 37
    A perfectly designed, state-of-the-art system first needs to pay for itself. But the crappy outdated system already did, so it actually makes more money, especially in the time-frame that decides the next bonus. – Jan Hudec Sep 26 at 19:51
  • 6
    Note that even one of the most comprehensively tested piece of code in the world, sqlite, had a couple of bugs fixed in each release. So while decent test suite is a requirement to be able to touch the code without breaking it to pieces at all, it still does not eliminate the risk completely. – Jan Hudec Sep 26 at 19:55
  • 3
    @Christophe: Nor is it just about the code working or not working. If you replace working code X that's been around quite a while with new code Y that works (in the sense that results are correct) but does things differently, you irritate and/or confuse your user base. – jamesqf Sep 27 at 4:46
  • 4
    @jamesqf That is very prescient. I've worked on projects that replaced existing systems (as per the additional remark). In practice, they always have to be bug-for-bug replacements. In situations where it would be significantly more work to replicate a bug than to just leave it out, you generally have to engage with users and other stakeholders to ensure they're happy with the change. – James_pic Sep 28 at 9:19
83

One reason is it's really difficult to measure the loss of productivity the messy code is causing, and difficult to estimate the work it will take to clean it properly and fix any regressions.

The other reason is many developers incorrectly call any rewriting refactoring. True refactoring is very methodical, almost mechanical. You make very small changes that improve the structure but don't change the behavior, and you verify the behavior didn't change using automated tests. If there aren't good tests, you add them first.

It makes experienced developers nervous when someone says they don't understand some code so they need to rewrite it. Changing code you don't understand is a recipe for disaster unless you are making dead simple changes.

So if you want to refactor some code, make your change so dead simple that anyone reviewing the pull request can instantly see the behavior is preserved. My first pull request in really messy legacy code is usually nothing but tests. My second pull request is purely mechanical changes like extracting repeated code into a function, and more tests that take advantage of those changes. Then on the third pull request I might start to rewrite some (now much smaller) functions for clarity, now that I have thorough tests. By this time, I have a fairly thorough understanding of the code, with all its quirks, and on the fourth pull request, I might make changes that affect the bigger picture.

If someone tried to skip straight to the fourth pull request, I would argue strongly against it, whereas I've never seen anyone argue against a pull request that just adds tests. If they won't let you make a high-risk change, make a low-risk change that moves you in the same direction.

| improve this answer | |
  • 44
    "Changing code you don't understand is a recipe for disaster unless you are making dead simple changes." – This is a variation of Chesterton's Fence. If you can't see why something is there, don't touch it. Only when you understand why something is there, can you delete it. Also, this story from the Jargon File is relevant. – Jörg W Mittag Sep 26 at 5:00
  • For the part of code I have refactored, I indeed have understanding on it as I have been maintaining the legacy code base for years. – Rui Sep 26 at 7:32
  • To add to the measurement problem, Measuring the correctness of your changes is often mighty difficult. This is especially true if the company hasn't been spending the money to maintain the kinds of testing and requirements which make it easier. – Cort Ammon Sep 26 at 17:49
  • As you say, it is often unclear what negative impact the legacy code has. At the same time it is obvious that refactoring will occupy resources lacking elsewhere, and as always the initial estimation probably underestimates the effort. This can be combined to the statement that the net benefit of the refactoring is not apparent to management, and indeed the management may be right! – Peter - Reinstate Monica Sep 27 at 12:55
  • Re "many developers incorrectly call any rewriting refactoring": Yes, indeed (as refactoring has now become cool - refactoring is a synonym for "good"). Often what is really (incorrectly) meant is any improvement that is not adding new features: restructuring, changing/improving interfaces, and removing parts of the system that seems not be used any more (it is also now cool to delete code, even if you don't fully understand it). (Real story - I wasted more than one year of my work life on this account.) – Peter Mortensen Sep 28 at 4:01
36

It depends on your definition of "correct practice".

I'm currently working on said old spaghetti code, much of it is old enough to drink. It's a critical safety system.

Changes to this code have to be audited and signed-off by an independent third party. The owners of the company consider it to be "correct practice" for the company not to budget money on re-factoring this code because it's working the way it is and has been for years.

And yes, some of the code is pretty much garbage, but it's well-tested and trusted garbage. We don't disassemble and re-assemble rivetted bridges just because nowadays nuts and bolts are best practice.

Rivets

Also, what's considered "best practice" in software engineering changes with the weather. Another 10 years and goto-spaghetti might be back in fashion ;)

| improve this answer | |
  • 2
    +1 Nice answer with practical real-life arguments :-) About your provocative final statement, I doubt that goto with global variables will be in again. But after all, the spaghetti was not the initial aim of former design: it emerged over time. So who knows: we might well discover in 10 years that the one or the other modern practices from today also had drawbacks that were not yet anticipated ... – Christophe Sep 27 at 18:25
  • 6
    we don't keep adding new pillars to old bridges either, often enough we build a new bridge to temporarily replace the old bridge, then rebuild the old bridge with modern tech and remove the temporary bridge again. @Christophe and most refactoring isn't about introducing new tech, it's about redesigning once multiple changes messed up the original intend of the code and made it harder to read and understand. So yes keeping old systems running, no issue, but the more you need to modify them regularly the more a refactoring makes sense. Might make the auditing even cheaper in such a case. – Frank Hopkins Sep 28 at 2:48
  • 1
    @FrankHopkins the analogy with construction work has its limits. Take the Eiffel tower: it’s a legacy that was initially supposed to be temporary. Maintenance changes periodically parts because of corrosion. There is no equivalent in software: software doesn’t corrode. And it’s not refactoring because the structure remains unchanged. There were however extensions, like additional elevators, which are added with the latest technology. But it’s made in a way that the overall structure is changed as little as possible. When elevators can no longer be maintained, upgraded, then the are replaced. – Christophe Sep 28 at 6:31
  • 1
    @FrankHopkins so what happens is that over time, the legacy remains unchanged. It’s extensions and the parts in contact with the extension are reengineered more often. But as said, although we can see some similarities, software is not concrete nor iron, and there’s still a lot if cobol out there ;-) – Christophe Sep 28 at 6:36
  • 3
    @EriksKlotins interesting thought, but a different phenomenon. Corrosion happens to a material without anybody making a change to it: iron of the bridge just rusts when in contact with its environment, without the intervention of any black-smith. The software doesn't rot just because it's executed and input is thrown at them by the user. About the dangers of analogy in engineering and science – Christophe Sep 28 at 21:18
12

I once had the joy of watching someone “refactoring” some legacy code that I had written, about two years earlier. My code was complicated, because it covered about two dozen corner cases that had been found through intensive testing. Each corner case handled was heavily documented.

The new code was a beauty. It was a quarter of the size, and very easy to understand. I took an old version of the code for reference and tried the first corner case. It didn’t work. I showed the new developer and he couldn’t understand why it didn’t work. I showed him exactly the code which made it work and which he had removed, ignoring the comments. Then I tried the second corner case and it didn’t work. I left him to it. I was on a different project now, and that was between him and his manager. Never checked what they did.

Now that happened with the author of the original code still there. Now imagine ten year old code, where the original author is gone, the guy working with him is gone, the one picking up after him is gone, and nobody knows what the code does and why.

I’m not saying you can’t refactor it, but you need someone who is really, really really good or you will break things in fantastic ways.

| improve this answer | |
  • 1
    Nice real life story. In what way were the hidden cases manifest? Was it an artefact of the language used, or awkward 'business' logic. I.e. how did the side-effects and side-causes get into the system - could the code ever have been pure functional code? – Philip Oakley yesterday
  • 1
    I voted your answer up because it is fundamentally the same as what I was going to say. I hope you will consider editing to explicitly call out that Formal requirements capture only that which is known in advance. Bugfixes, performance tweaks and modifications that extend or change the planned behaviour are absent from the spec. Often these augmentations are done in a hurry and are totally undocumented. Unless done by the same team while the issues are still large in their minds, a do-over abandons all this hard won knowledge and experience. – Peter Wone 8 hours ago
10

How do you define 'correct'?

I have seen more disasters when people tried to fix something that was not broke by fixing spaghetti code.

There are times when you will be forced to make changes.

Some folks try to totally rewrite it 'correctly'. Those attempts usually take longer and have more problems than they estimated; and sometimes they fail completely.

I have been there, done that, and I say if it ain't broke, don't fix it. Leave it for the next schmoe to worry about after you have moved on to a better job somewhere else.

| improve this answer | |
  • 2
    A rewrite is not refactoring. – Peter Mortensen Sep 28 at 4:11
  • @Peter: Refactoring code that is not under test is a rewrite. – mattnz 12 hours ago
5

You need a reason to be changing any code. That reason may be as simple as dead code (unreachable), making it easier to read (loops to LINQ for instance) or as complex as refactoring multiple modules for multiple interrelated reasons.

When refactoring important and large blocks of code, the first thing to determine isn’t how desirable or even necessary the change is, the first thing you want to determine is how safely the change can be made. The more important the code is to the business the higher the standards for safety. An existing bug has a known cost associated with it, and it hasn’t yet been fatal to the organization. You don’t want to replace it with a bug that drives you out of business.

A procedure that is several thousand lines, and is extremely valuable to the organization (and not just initializing a bunch of ui controls position and values), is not something that should have more than trivial changes done without first considering the risk.

That said, the first change I would consider making for such a large procedure is breaking it down into sub procedures, if it’s 3500 lines, and you can turn that into 35 sequential procedures: proc1, proc2..proc35, I would consider that a win.

In short, the decision to leave it alone or not is not directly related to the code quality, it’s a function of weighing risk vs reward. If you can eliminate the risk, then the reward is irrelevant, conversely, the risk could be so great as to again make the reward irrelevant. In most cases the risk will be moderate and the direct benefits will be minimal. Which is why the changes get nixed. If you want to make changes, work on making them risk free. Smaller procedures, unit test, adding documentation so it is better understood and less opaque.

| improve this answer | |
  • 3
    +1 for your first paragraph. A company doesn't exist to develop software; it exists to make a profit. If there's no return on investment in refactoring, or if the return isn't enough to justify the investment, then it shouldn't happen. I have exactly this at work - there's a laundry list of new features I'd like to add, but there's only me to do it, and I have other commitments too besides just cutting code. That's why management exists, to work out what's a priority and what isn't. – Graham Sep 27 at 12:50
2

In addition to the good answers I'd like to add something from my experience.

A rewrite is not always feasible for different reasons, for instance if you need to still add new features continuously. So if you need to take a step-by-step approach for refactoring you will have to accept that there will still be legacy code in place for a long time (or even forever depending on the size of the codebase and lifetime of the software).

So most of the time you have to make trade-offs where to invest your time when performing refactorings because a huge code base cannot be refactored at once.

In this case I would be pragmatic and start with refactoring in places where the highest benefits are expected. This could be, for instance, extremely buggy parts of the code or parts of the system where changes happen a lot.

If changes happen a lot refactoring of that part can provide great benefits as clean code can easier be changed than messy code.

Code that does not change often and is considered correct (i.e. does not have known bugs) should be touched at last on the refactoring journey. Or maybe it will never be touched at all...

I think most companies want to avoid unnecessary risks. As refactoring legacy code can be risky the risk should be taken for the right reasons and for code where it pays off at first.

| improve this answer | |
1

I agree with the answer of Karl Bielefeldt, there is just a small point to add to it.

In a massive monolith of spaghetti code often a partial refactoring is impossible. Bits of code are so tightly coupled that one change in one point requires one or many changes to align the code in other points in a chain of changes so big that you easily go beyond what is reasonable in the modern agile process. So to change the code people prefer to wait for a change justified by the business in some way.

| improve this answer | |
  • 5
    Even extremely spaghettified code (with conditional goto statements depending on some global variables set far apart and whose values depend on other conditional goto statements) can be mechanically refactored (as Karl Bielefeldt mentioned) as the first step, putting linear sequences of code into functions/procedures. It may not be possible to find meaningful names for the functions at this first step, but that may be done later. Subsequent changes to the code are much easier and it may be possible to isolate the spaghetti part from the more structured part. – Peter Mortensen Sep 28 at 4:33
  • (I have worked on such a system (freelance), porting from one kind of system to another, translating to another programming language in the process.) – Peter Mortensen Sep 28 at 4:33
  • One of the 'mechanical' methods is described on "The Mikado Method". – Philip Oakley yesterday
1

From my experience, a long spaghetti code is a result of

  • reinventing things that can be done easier, e.g. using built-in features. I once audited a farily big Javascript app where a lot of existing Javascript core features were implemented using custom functions and even these were used inconsistently.

  • reinventing design patterns that have their idomatic implementations or should be based on well-known specs. I often see DI containers done from scratch in a way you need a while to realize what it's all about. Another example - the Observer pattern, reinvented almoast as if it's there in every Joe's subconsciousness. Another example - SSO protocols, it's been like 15 years when SAML was proposed, OAuth2 and OpenIDConnect are here also for a while but no, it's "Joe did it, he doesn't work here anymore and we are afraid to touch it"

  • not following SOLID GRASP recommendations. Design Patterns? Nope, it's worse. It's like a 3000 line of code refactored to 30 methods of 100 lines named almost like Foo1 to Foo30 where each FooN calls FooN+1 in its last line

  • having zero or not enough unit tests to cover basic and corner cases so that you could pretty much do whatever you want with the code and just look if tests pass. Instead, with insufficient tests, people are afraid that the code does weird things in corner cases and someone relies on these corner cases. And still somehow people are afraid to recreate these cases in unit tests so that they could refactor the code.

There's always quality vs cost/time and it's not that you should take care of everything. But any critical part could easily be isolated, tamed with unit tests and then, refactored. A good rule of refactoring is refactor until you are satisfied and then change each new bug into a new unit tests that covers this buggy case. With this in mind, I did at least couple of nasty refactorings where something completely messy turned into something that it totally under control.

| improve this answer | |
  • Thanks a lot, your suggestion is indeed pretty practical and I most of the time am practicing in such approach :) – Rui Oct 23 at 11:09

Not the answer you're looking for? Browse other questions tagged or ask your own question.