52

I've got a project. In this project I wished to refactor it to add a feature, and I refactored the project to add the feature.

The problem is that when I was done, it turned out that I needed to make a minor interface change to accommodate it. So I made the change. And then the consuming class can't be implemented with its current interface in terms of the new one, so it needs a new interface as well. Now it's three months later, and I've had to fix innumerable virtually unrelated problems, and I'm looking at solving issues that were roadmapped for a year from now or simply listed as won't fix due to difficulty before the thing will compile again.

How can I avoid this kind of cascading refactorings in the future? Is it just a symptom of my previous classes depending too tightly on each other?

Brief edit: In this case, the refactor was the feature, since the refactor increased the extensibility of a particular piece of code and decreased some coupling. This meant that external developers could do more, which was the feature I wanted to deliver. So the original refactor itself should not have been a functional change.

Bigger edit that I promised five days ago:

Before I began this refactor, I had a system where I had an interface, but in the implementation, I simply dynamic_cast through all the possible implementations that I shipped. This obviously meant that you couldn't just inherit from the interface, for one thing, and secondly, that it would be impossible for anybody without implementation access to implement this interface. So I decided that I wanted to fix this issue and open up the interface for public consumption so that anybody could implement it and that implementing the interface was the entire contract required- obviously an improvement.

When I was finding and killing-with-fire all the places that I had done this, I found one place that proved to be a particular problem. It depended upon implementation details of all the various deriving classes and duplicated functionality that was already implemented but better somewhere else. It could have been implemented in terms of the public interface instead and re-used the existing implementation of that functionality. I discovered that it required a particular piece of context to function correctly. Roughly speaking, the calling previous implementation looked kinda like

for(auto&& a : as) {
     f(a);
}

However, to get this context, I needed to change it into something more like

std::vector<Context> contexts;
for(auto&& a : as)
    contexts.push_back(g(a));
do_thing_now_we_have_contexts();
for(auto&& con : contexts)
    f(con);

This means that for all operations that used to be a part of f, some of them need to be made a part of the new function g that operates without a context, and some of them need to be made of a part of the now-deferred f. But not all methods f call need or want this context- some of them need a distinct context that they obtain through separate means. So for everything that f ends up calling (which is, roughly speaking, pretty much everything), I had to determine what, if any, context they needed, where they should get it from, and how to split them from old f into new f and new g.

And that's how I ended up where I am now. The only reason that I kept going is because I needed this refactoring for other reasons anyway.

DeadMG
  • 36,794
  • 8
  • 70
  • 139
  • 68
    When you say you "refactored the project to add a feature", what do you mean exactly? Refactoring does not change the behavior of programs, by definition, which makes this statement confusing. – Jules Jan 11 '15 at 15:30
  • 5
    @Jules: Strictly speaking, the feature was to permit other developers to add a particular type of extension, so the feature was the refactor, which made the class structure more open. – DeadMG Jan 11 '15 at 16:01
  • 5
    I thought this is discussed in every book and article that talks about refactoring? Source control comes to rescue; if you find that to do step A you have to do step B first, then scrap A and do B first. – rwong Jan 11 '15 at 19:36
  • 1
    Also, ambitious refactoring (or even re-architecting) requires a branch (or a fork). Again, source control comes to rescue. – rwong Jan 11 '15 at 19:37
  • @rwong: Actually this work is on a branch, fortunately. But this is a one-man project so it doesn't have the same value. However, performing the work in the *inverse* order, if possible, sounds like a useful suggestion that might apply. – DeadMG Jan 11 '15 at 20:04
  • 4
    @DeadMG: this is [the book](http://www.manning.com/ellnestam/) I originally wanted to quote in my first comment: *"The game "pick-up sticks" is a good metaphor for the Mikado Method. You eliminate "technical debt"—the legacy problems embedded in nearly every software system—by following a set of easy-to-implement rules. You carefully extract each intertwined dependency until you expose the central issue, without collapsing the project."* – rwong Jan 11 '15 at 20:37
  • 2
    May, can you clarify which programming language we are talking about? After reading all your comments, I come to the conclusion, that you are doing it by hand instead of using an IDE to assist you. Thus I would like to know, whether I can give you some practical advice. – thepacker Jan 11 '15 at 22:08
  • @thepacker: I am using Visual Studio 2013 with C++. But I am not aware of any IDE which is capable of performing this task automatically. – DeadMG Jan 11 '15 at 23:03
  • 1
    @DeadMG: Do you use tools like Visual Assist and Rename; Even QT-Creator comes with some built-in refactoring tools. If you are developing for money, using such tools will pay off in a few days. You should also have a look at the c++ version of resharper. https://www.jetbrains.com/resharper/features/cpp.html – thepacker Jan 11 '15 at 23:29
  • 1
    @thepacker: None of those tools can cover what happens when your code simply doesn't make sense anymore. Changing a few names is a trivial problem. – DeadMG Jan 11 '15 at 23:34
  • 1
    Do not throw/spend more time on a solution where you can sense the dead end. It is not worth it / You are wasting a lot of money. If you spend three month on refactoring/damaging a piece of code, now should be definetly the time to end such things, revert and learn from that experience. Just try it out - revert to first version, if you find another path of refactoring, this is fine - otherwise you can always return to your last damaged version again. – thepacker Jan 11 '15 at 23:41
  • @DeadMG: it is hard to tell you what went went wrong in your case without actually seeing the "real thing". It won't make sense to post the code here, of course, but can you give an outline or example of the "cascade" you are experiencing? – Doc Brown Jan 12 '15 at 07:09
  • I am going to describe it in more detail when I return from work today. – DeadMG Jan 13 '15 at 09:21
  • 1
    As others have commented, that doesn't strictly sound like refactoring. [This article by Joel Spolsky](http://www.joelonsoftware.com/articles/fog0000000348.html) lays out some sensible rules of thumb for a refactoring exercise. You may also want to consider the [sunk cost fallacy](http://en.wikipedia.org/wiki/Escalation_of_commitment) when deciding whether to press on or revert and change approach. – jonrsharpe Jan 13 '15 at 15:08
  • This scenario sounds like a perfect fit for the Mikado method https://mikadomethod.wordpress.com - in a nutshell, you develop your plan by starting with the refactoring you wish to make, find the refactorings that are needed for this, and continue from there until you find the refactorings that don't depend on anything else (the top sticks in your Mikado stack). These are the first refactorings you actually perform - from there, you work backwards until you finally perform the refactoring you wanted to make in the first place. – Frank Schmitt Jan 17 '15 at 08:19

11 Answers11

69

Last time I tried to start a refactoring with unforeseen consequences, and I could not stabilize the build and / or the tests after one day, I gave up and reverted the codebase to the point before the refactoring.

Then, I started to analyze what went wrong and developed a better plan how to do the refactoring in smaller steps. So my advice for avoiding cascading refactorings is just: know when to stop, don't let things run out of your control!

Sometimes you have to bite the bullet and throw away a full day of work - definitely easier than to throw away three months of work. The day you loose is not completely in vain, at least you have learned how not to approach the problem. And to my experience, there are always possibilities to make smaller steps in refactoring.

Side note: you seem to be in a situation where you have to decide if you are willing to sacrifice full three months of work and start over again with a new (and hopefully more successful) refactoring plan. I can imagine that is not an easy decision, but ask yourself, how high is the risk you need another three months not just to stabilize the build, but also to fix all unforeseen bugs you probably introduced during your rewrite you did the last three months? I wrote "rewrite", because I guess that is what you really did, not a "refactoring". It is not unlikely that you can solve your current problem quicker by going back to the last revision where your project compiles and start with a real refactoring (opposed to "rewrite") again.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
55

Is it just a symptom of my previous classes depending too tightly on each other?

Sure. One change causing a myriad of other changes is pretty much the definition of coupling.

How do I avoid cascading refactors?

In the worst sort of codebases, a single change will continue to cascade, eventually causing you to change (almost) everything. Part of any refactor where there is widespread coupling is to isolate the part you're working on. You need to refactor not just where your new feature is touching this code, but where everything else touches that code.

Usually that means making some adapters to help the old code work with something that looks and acts like the old code, but uses the new implementation/interface. After all, if all you do is change the interface/implementation but leave the coupling you're not gaining anything. It's lipstick on a pig.

Telastyn
  • 108,850
  • 29
  • 239
  • 365
  • 33
    +1 The more badly a refactoring is needed, the more widely the refactoring will reach. It's the very nature of the thing. – Paul Draper Jan 12 '15 at 09:46
  • 4
    If you're truly *refactoring*, though, other code shouldn't have to concern itself with the changes right away. (Of course you'll eventually want to clean up the other parts...but that shouldn't be immediately required.) A change that "cascades" through the rest of the app, is bigger than refactoring -- at that point it's basically a redesign or rewrite. – cHao Jan 12 '15 at 18:12
  • 1
    +1 An adapter is exactly the way to isolate the code you want to change first. – winkbrace Jan 15 '15 at 20:51
17

It sound like your refactoring was too ambitious. A refactoring should be applied in small steps, each of which can be completed in (say) 30 minutes - or, in a worst-case scenario, at most a day - and leaves the project buildable and all tests still passing.

If you keep each individual change minimal, it really shouldn't be possible for a refactoring to break your build for a long time. The worst case is probably changing the parameters to a method in a widely used interface, e.g. to add a new parameter. But the consequential changes from this are mechanical: adding (and ignoring) the parameter in each implementation, and adding a default value in each call. Even if there are hundreds of references, it shouldn't take even a day to perform such a refactoring.

Jules
  • 17,614
  • 2
  • 33
  • 63
  • What if there is no suitable default? – DeadMG Jan 11 '15 at 18:24
  • 4
    I don't see how such a situation can arise. For any reasonable refactoring of a method's interface there must be an easily-determined new set of parameters that can be passed that will result in the behavior of the call being the same as it was before the change. – Jules Jan 11 '15 at 18:30
  • 1
    That would assume that every refactoring makes the method's contract more broad, instead of, say, more narrow to support more implementations. – DeadMG Jan 11 '15 at 18:45
  • 3
    I've never been in a situation where I wanted to perform such a refactoring, but I have to say it sounds quite unusual to me. Are you saying you removed functionality from the interface? If so, where did it go? Into another interface? Or somewhere else? – Jules Jan 11 '15 at 18:55
  • It turns out that it simply doesn't make sense to have that functionality anymore- so it didn't *go* anywhere, I am removing it completely. – DeadMG Jan 11 '15 at 20:07
  • 5
    Then the way to do it is to remove all the usages of the feature to be removed before refactoring to remove it, rather than afterwards. This allows you to keep the code building while you're working on it. – Jules Jan 11 '15 at 20:40
  • Removing all usages of that feature would render the project completely non-functional. – DeadMG Jan 11 '15 at 21:38
  • 12
    @DeadMG: that sounds strange: you are removing one feature which is not needed any more, as you say. But on the other hand, you write "the project becomes completely non-functional" - that actually sounds the feature is absolutely needed. Please clarify. – Doc Brown Jan 11 '15 at 22:24
  • @DocBrown: The new feature replaces the old feature. It's not needed anymore when the new feature is complete. But before the new feature is ready to go, then you really, really need it. – DeadMG Jan 11 '15 at 23:05
  • 26
    @DeadMG In such cases you would normally develop the new feature, add tests to ensure it works, transition existing code to use the new interface, and *then* remove the (now) superfluous old feature. That way, there shouldn't be a point at which things break. – sapi Jan 11 '15 at 23:45
  • @sapi: I have a branch for it. The branch without this stuff still works just fine. – DeadMG Jan 18 '15 at 11:27
12

How can I avoid this kind of cascading refactor in the future?

Wishful Thinking Design

The goal is excellent OO design and implementation for the new feature. Avoiding refactoring is also a goal.

Start from scratch and make a design for the new feature that is what you wish you had. Take the time to do it well.

Note however that the key here is "add a feature". New stuff tends to let us largely ignore the current structure of the code base. Our wishful thinking design is independent. But we then need two more things:

  • Refactor only enough to make a necessary seam to inject/implement the new feature's code.
    • Resistance to refactoring should not drive the new design.
  • Write a client facing class with an API that keeps the new feature & existing codez blissfully ignorant of each other.
    • It transliterates to get objects, data, and results back and forth. Least knowledge principle be damned. We're not going to do anything worse than what existing code already does.

Heuristics, Lessons Learned, etc.

Refactoring has been as simple as adding an default parameter to an existing method call; or a single call to a static class method.

Extension methods on existing classes can help keep the new design's quality with absolute minimal risk.

"Structure" is everything. Structure is the realization of the Single Responsibility Principle; design that facilitates functionality. Code will stay short and simple all the way up the class hierarchy. Time for new design is made up during testing, re-work, and avoiding hacking through the legacy code jungle.

Wishful thinking classes focus on the task at hand. Generally, forget about extending an existing class - you're just inducing the refactor cascade again & having to deal with the "heavier" class' overhead.

Purge any remnants of this new functionality from existing code. Here, complete and well encapsulated new feature functionality is more important than avoiding refactoring.

radarbob
  • 5,808
  • 18
  • 31
9

From (the wonderful) book Working Effectively with Legacy Code by Michael Feathers:

When you break dependencies in legacy code, you often have to suspend your sense of aesthetics a bit. Some dependencies break cleanly; others end up looking less than ideal from a design point of view. They are like the incision points in surgery: there might be a scar left in your code after your work, but everything beneath it can get better.

If later you can cover code around the point where you broke the dependencies, you can heal that scar, too.

Kenny Evitt
  • 560
  • 3
  • 10
6

It sounds like (especially from the discussions in comments) you've boxed yourself in with self-imposed rules that mean that this "minor" change is the same amount of work as a complete rewrite of the software.

The solution has to be "don't do that, then". This is what happens in real projects. Plenty of old APIs have ugly interfaces or abandoned (always null) parameters as a result, or functions named DoThisThing2() which does the same as DoThisThing() with an entirely different parameter list. Other common tricks include stashing information in globals or tagged pointers in order to smuggle it past a large chunk of framework. (For example, I have a project where half the audio buffers contain only a 4-byte magic value because that was much easier than changing how a library invoked its audio codecs.)

It's hard to give specific advice without specific code.

pjc50
  • 10,595
  • 1
  • 26
  • 29
3

Automated tests. You don't need to be a TDD zealot, nor do you need 100% coverage, but automated tests are what allow you to make changes confidently. In addition, it sounds like you have a design with very high coupling; you should read about the SOLID principles, which are formulated specifically to address this kind of issue in software design.

I'd also recommend these books.

  • Working Effectively with Legacy Code, Feathers
  • Refactoring, Fowler
  • Growing Object-Oriented Software, Guided by Tests, Freeman and Pryce
  • Clean Code, Martin
asthasr
  • 3,439
  • 3
  • 17
  • 24
  • 2
    Automated tests only apply once your codebase can compile again, and I'm trying to reach that stage. I already possess automated testing, but they are of little benefit when you can't run them because it can't compile. – DeadMG Jan 11 '15 at 16:02
  • 1
    But your code base should never get to the point where it can't compile. You should be running your tests *constantly*, and the moment you can't, you have a broken situation and need to fix it. One form of "broken build" is "I can't even run the tests," and you should never continue working until that situation is fixed. Read about [Continuous Integration](http://en.wikipedia.org/wiki/Continuous_integration). – asthasr Jan 11 '15 at 16:13
  • 1
    I have continuous integration. The whole point of the question is that I have a broken situation, but every fix just breaks it again in a new way. – DeadMG Jan 11 '15 at 16:22
  • 3
    Your question is, "how do I avoid this [failure] in the future?" The answer is that, even if you currently "have" CI and tests, you are not applying them correctly. I haven't had a compile error that lasted more than ten minutes in years, because I view compiling as "the first unit test," and when it is broken, I fix it, because I need to be able to see tests passing as I am working further on the code. – asthasr Jan 11 '15 at 16:45
  • 2
    @DeadMG: Well, please consider reverting it and do it once more. Three consecutive month of failing builds is not an option. Revert and refactor using little steps. – thepacker Jan 11 '15 at 16:47
  • 2
    There is no alternative, because the refactoring is to core interfaces. Either I don't refactor them or the work takes a very long time because it affects everything- in the current state. syrion, your suggestion implies that you simply never perform any refactoring of heavily used interfaces. – DeadMG Jan 11 '15 at 17:25
  • 1
    @DeadMG even though that is pretty much the worst-case scenario, I find that with each call site typically taking only a handful of seconds to fix, even if there are thousands of calls to a method whose definition has changed, it shouldn't take more than a few hours before the project is building again. – Jules Jan 11 '15 at 18:20
  • 6
    If I am refactoring a heavily used interface, I add a shim. This shim handles the defaulting, so that legacy calls continue to work. I work on the interface behind the shim, then, when I am done with it, I begin to change classes to use the interface again instead of the shim. – asthasr Jan 11 '15 at 18:49
  • 1
    @syrion: That strategy can only handle the case where you are *broadening* the interface, instead of narrowing it or completely changing it. In my case, the previous calls simply do not make sense anymore- there is no meaningful translation and no useful default. – DeadMG Jan 11 '15 at 20:05
  • 5
    Continuing to refactor despite build failing is akin to [dead reckoning](http://en.wikipedia.org/wiki/Dead_reckoning). It's a navigational technique *of last resort*. In refactoring, it is possible that a refactoring direction is just plain wrong, and you've already seen the telltale sign of that (the moment it stops compiling, i.e. flying without airspeed indicators), but you decided to press on. Eventually the plane falls off the radar. Fortunately we don't need blackbox or investigators for refactoring: we can always "restore to last known good state". – rwong Jan 11 '15 at 20:53
  • 4
    @DeadMG: you wrote "In my case, the previous calls simply do not make sense anymore", but in your question "a *minor* interface change to accommodate it". Honestly, only one of those two sentences can be true. And from your problem description it seems pretty clear that your interface change was definitely not a *minor* one. You should really, really think harder about how to make your change more backward compatible. To my experience, that is always possible, but you have to come up with a good plan first. – Doc Brown Jan 11 '15 at 22:14
  • 1
    @DocBrown: The minor change was only the first change or two, which were fairly minor. The problem is the resulting cascade of necessary fixes to core interfaces, which were decidedly not minor at all. – DeadMG Jan 11 '15 at 23:07
  • 1
    @DeadMG - each change in refactoring should be minor, and should be finished and tested before you begin the next change. There shouldn't be a large change like you describe. Add a new interface is simple, change one existing use of an old interface to the new one should be simple, removing an interface that is no longer used should be simple. I really don't understand why this has become so complex. – Jules Jan 12 '15 at 10:09
  • @Jules: Because the uses of the old interfaces *cannot* be transformed into the new one. – DeadMG Jan 12 '15 at 18:21
  • 3
    @DeadMG In that case, I think what you are doing *cannot* reasonably be called refactoring, the basic point of which is to apply design changes as a series of very simple steps. – Jules Jan 12 '15 at 18:57
3

Is it just a symptom of my previous classes depending too tightly on each other?

Most probably yes. Although you can get similar effects with a rather nice and clean code base when the requirements change enough

How can I avoid this kind of cascading refactorings in the future?

Apart from stopping to work on legacy code, you can't I'm afraid. But what you can is using a method that avoids the effect of not having a working code base for days, weeks or even months.

That method is named "Mikado Method" and it works like this:

  1. write down the objective you want to achieve on a piece of paper

  2. make the simplest change that takes you into that direction.

  3. check if it works using the compiler and your test suite. If it does continue with step 7. otherwise continue with step 4.

  4. on your paper note the things that need to change to make your current change work. Draw arrows, from your current task, to the new ones.

  5. Revert your changes This is the important step. It is counter intuitive and physically hurts in the beginning, but since you just tried a simple thing, it isn't actually that bad.

  6. pick one of the tasks, that has no outgoing errors (no known dependencies) and return to 2.

  7. commit the change, cross out the task on the paper, pick a task that has no outgoing errors (no known dependencies) and return to 2.

This way you will have a working code base in short intervals. Where you also can merge changes from the rest of the team. And you have a visual representation of what you know you still have to do, this helps to decide if you want to continue with the endevour or if you should stop it.

Jens Schauder
  • 1,503
  • 8
  • 13
2

...I refactored the project to add the feature.

As said @Jules, Refactoring and adding features are two very different things.

  • Refactoring is about changing the program's structure without altering its behavior.
  • Adding a feature, on the other hand, is augmenting its behavior.

...but indeed, sometimes you need to change the inner workings to add your stuff, but I'd rather call it modifying rather than refactoring.

I needed to make a minor interface change to accommodate it

That's where things get messy. Interfaces are meant as bounderies to isolate the implementation from how it's used. As soon as you touch interfaces, everything on either side (implementing it or using it) will have to be changed as well. This can spread far as you experienced it.

then the consuming class can't be implemented with its current interface in terms of the new one, so it needs a new interface as well.

That one interface requires a change sounds fine ...that it spreads to another implies changes spread even further. It sounds like some form of input/data requires to flow down the chain. Is that the case?


Your talk is very abstract, so it's difficult to figure out. An example would be very helpful. Usually, interfaces should be pretty stable and independent from each other, making it possible to modify part of the system without harming the rest ...thanks to the interfaces.

...actually, the best way to avoid cascading code modifications are precisely good interfaces. ;)

dagnelies
  • 5,415
  • 3
  • 20
  • 26
1

Refactoring is a structured discipline, distinct from cleaning up code as you see fit. You need to have unit tests written before starting, and each step should consist of a specific transformation that you know should make no changes to functionality. The unit tests should pass after every change.

Of course, during the refactoring process, you will naturally discover changes that should be applied that may cause breakage. In that case, try your best to implement a compatibility shim for the old interface that uses the new framework. In theory, the system should still work as before, and the unit tests should pass. You can mark the compatibility shim as a deprecated interface, and clean that up at a more suitable time.

200_success
  • 1,568
  • 11
  • 20
-1

I think you usually cannot unless you are willing to keep things as they are. However, when situations like yours, I think the better thing is to inform the team and let them know why there should be some refactoring done in order to continue more healthy development. I wouldn't just go and fix things by myself. I would talk about it during Scrum meetings (assuming you guys have them), and approach it systematically with other developers.

Tarik
  • 777
  • 5
  • 12