Working Effectively with Legacy Code

Author: Michael C. Feathers
4.5
All Stack Overflow 260
This Year Stack Overflow 5
This Month Stack Overflow 3

About This Book

Is your code easy to change? Can you get nearly instantaneous feedback when you do change it? Do you understand it? If the answer to any of these questions is no, you have legacy code, and it is draining time and money away from your development efforts.

 

In this book, Michael Feathers offers start-to-finish strategies for working more effectively with large, untested legacy code bases. This book draws on material Michael created for his renowned Object Mentor seminars: techniques Michael has used in mentoring to help hundreds of developers, technical managers, and testers bring their legacy systems under control.

 

 

The topics covered include

  • Understanding the mechanics of software change: adding features, fixing bugs, improving design, optimizing performance
  • Getting legacy code into a test harness
  • Writing tests that protect you against introducing new problems
  • Techniques that can be used with any language or platform—with examples in Java, C++, C, and C#
  • Accurately identifying where code changes need to be made
  • Coping with legacy systems that aren't object-oriented
  • Handling applications that don't seem to have any structure

This book also includes a catalog of twenty-four dependency-breaking techniques that help you work with program elements in isolation and make safer changes.

Comments

by kbouck   2019-07-12
I've heard the book Working with Legacy Code [0] recommended for strategies to bring order to these kind of projects. Haven't read it myself yet...

[0]: https://www.amazon.com/Working-Effectively-Legacy-Michael-Fe...

by edem   2019-07-12
I'd suggest the book [Working Effectively with Legacy Code](https://www.amazon.com/Working-Effectively-Legacy-Michael-Fe...).
by jrochkind1   2019-07-12
This book is pretty great:

https://www.amazon.com/Working-Effectively-Legacy-Michael-Fe...

by kolinko   2019-05-10
Yes. By the way - „Working effectively with legacy code”.

https://www.amazon.com/Working-Effectively-Legacy-Michael-Fe...

If you start from scratch you may bump into the same edge cases that the original writers bumped into, and end up with a code that is not much better than the original - even in the original is 2 years out of date.

I’m sure there were cases when writing from scratch was a good call, but I don’t remember hearing about it.

by anonymous   2019-01-13

I believe that the mocking support in CppUTest is invasive, i.e. you need to add mocking support to your production code as well. Example:

void success(void)
{
    mock().actualCall("success");
    ....
}

For non-invasive unit testing of plain C code you could instead use e.g. preprocessor or link seams. Check out Michael Feathers' great book Working Effectively with Legacy Code for details.

Actually, an extract from that book covering those seam types can be found here. I'd still recommend any C programmer to read that book - it's invaluable.

by anonymous   2019-01-13

Your ClassToMock tightly coupled with FileReader, that's why you are not able to test/mock it. Instead of using tool to hack the byte code so you can mock it. I would suggest you do some simple refactorings to break the dependency.

Step 1. Encapsulate Global References

This technique is also introduced in Michael Feathers's wonderful book : Working Effectively with Legacy Code.

The title pretty much self explained. Instead of directly reference a global variable, you encapsulate it inside a method.

In your case, ClassToMock can be refactored into this :

public class ClassToMock {
  private static final String MEMBER_1 = FileReader.readMemeber1();

  public String getMemberOne() {
    return MEMBER_1;      
  }
}

then you can easily using Mockito to mock getMemberOne().

UPDATED Old Step 1 cannot guarantee Mockito mock safely, if FileReader.readMemeber1() throw exception, then the test will failled miserably. So I suggest add another step to work around it.

Step 1.5. add Setter and Lazy Getter

Since the problem is FileReader.readMember1() will be invoked as soon as ClassToMock is loaded. We have to delay it. So we make the getter call FileReader.readMember1() lazily, and open a setter.

public class ClassToMock {
  private static String MEMBER_1 = null;

  protected String getMemberOne() {
    if (MEMBER_1 == null) {
      MEMBER_1 = FileReader.readMemeber1();
    }
    return MEMBER_1;      
  }

  public void setMemberOne(String memberOne) {
    MEMBER_1 = memberOne;
  }
}

Now, you should able to make a fake ClassToMock even without Mockito. However, this should not be the final state of your code, once you have your test ready, you should continue to Step 2.

Step 2. Dependence Injection

Once you have your test ready, you should refactor it further more. Now Instead of reading the MEMBER_1 by itself. This class should receive the MEMBER_1 from outside world instead. You can either use a setter or constructor to receive it. Below is the code that use setter.

public class ClassToMock {
  private String memberOne;
  public void setMemberOne(String memberOne) {
    this.memberOne = memberOne;
  }

  public String getMemberOne() {
    return memberOne;
  }
}

These two step refactorings are really easy to do, and you can do it even without test at hand. If the code is not that complex, you can just do step 2. Then you can easily test ClassToTest


UPDATE 12/8 : answer the comment

See my another answer in this questions.

by anonymous   2019-01-13

UPDATE 12/8 : answer the comment

Question : What if FileReader is something very basic like Logging that needs to be there in every class. Would you suggest I follow the same approach there?

It depends.

There are something you might want to think about before you do a massive refactor like that.

  1. If I move FileReader outside, do I have a suitable class which can read from file and provide the result to every single class that needs them ?

  2. Beside making classes easier to test, do I gain any other benefit ?

  3. Do I have time ?

If any of the answers is "NO", then you should better not to.

However, we can still break the dependency between all the classes and FileReader with minimal changes.

From your question and comment, I assume your system using FileReader as a global reference for reading stuff from a properties file, then provide it to rest of the system.

This technique is also introduced in Michael Feathers's wonderful book : Working Effectively with Legacy Code, again.

Step 1. Delegate FileReader static methods to instance.

Change

public class FileReader {
  public static FileReader getMemberOne() {
    // codes that read file.
  }
}

To

public class FileReader {
  private static FileReader singleton = new FileReader();
  public static String getMemberOne() {
    return singleton.getMemberOne();
  }

  public String getMemberOne() {
    // codes that read file.
  }
}

By doing this, static methods in FileReader now have no knowledge about how to getMemberOne()

Step 2. Extract Interface from FileReader

public interface AppProperties {
  String getMemberOne();
}

public class FileReader implements AppProperties {
  private static AppProperties singleton = new FileReader();
  public static String getMemberOne() {
    return singleton.getMemberOne();
  }

  @Override
  public String getMemberOne() {
    // codes that read file.
  }
}

We extract all the method to AppProperties, and static instance in FileReader now using AppProperties.

Step 3. Static setter

public class FileReader implements AppProperties {
  private static AppProperties singleton = new FileReader();

  public static void setAppProperties(AppProperties prop) {
    singleton = prop;
  }

  ...
  ...
}

We opened a seam in FileReader. By doing this, we can set change underlying instance in FileReader and it would never notice.

Step 4. Clean up

Now FileReader have two responsibilities. One is read files and provide result, another one is provide a global reference for system.

We can separate them and give them a good naming. Here is the result :

// This is the original FileReader, 
// now is a AppProperties subclass which read properties from file.
public FileAppProperties implements AppProperties {
  // implementation.
}

// This is the class that provide static methods.
public class GlobalAppProperties {

  private static AppProperties singleton = new FileAppProperties();

  public static void setAppProperties(AppProperties prop) {
    singleton = prop;
  }

  public static String getMemberOne() {
    return singleton.getMemberOne();
  }
  ...
  ...
}

END.

After this refactoring, whenever you want to test. You can set a mock AppProperties to GlobalAppProperties

I think this refactoring would be better if all you want to do is break the same global dependency in many classes.

by anonymous   2019-01-13

Example from Book: Working Effectively with Legacy Code

To run code containing singletons in a test harness, we have to relax the singleton property. Here’s how we do it. The first step is to add a new static method to the singleton class. The method allows us to replace the static instance in the singleton. We’ll call it setTestingInstance.

public class PermitRepository
{
    private static PermitRepository instance = null;
    private PermitRepository() {}
    public static void setTestingInstance(PermitRepository newInstance)
    {
        instance = newInstance;
    }
    public static PermitRepository getInstance()
    {
        if (instance == null) 
        {
            instance = new PermitRepository();
        }
        return instance;
    }
    public Permit findAssociatedPermit(PermitNotice notice) 
    {
    ...
    }
...
}

Now that we have that setter, we can create a testing instance of a PermitRepository and set it. We’d like to write code like this in our test setup:

public void setUp() {
PermitRepository repository = new PermitRepository();
...
// add permits to the repository here
...
PermitRepository.setTestingInstance(repository);
}
by chriswoodford   2018-12-01
I'll open by saying I've only ever had bad experiences with complete re-writes and these experiences have impacted my aversion to them.

"[Working Effectively with Legacy Code]" by Michael Feathers really helped me get through a situation like this.

My recommendation is not to try to understand the code per se, but understand the business that the code was being used in/by.

From there, over time, just start writing really high level end-to-end tests to represent what the business expects the codebase to do (i.e. starting at the top of the [test pyramid]). This ends up acting as your safety net (your "test harness").

Then it's less a matter of trying to understand what the code does, and becomes a question of what the code should do. You can iterate level by level into the test pyramid, documenting the code with tests and refactoring/improving the code as you go.

It's a long process (I'm about 4.5 years into it and still going strong), but it allowed us to move fast while developing new features with a by-product of continually improving the code base as we went.

[test pyramid]: https://martinfowler.com/bliki/TestPyramid.html [Working Effectively with Legacy Code]: https://www.amazon.com/FEATHERS-WORK-EFFECT-LEG-CODE/dp/0131...

by kat   2018-08-26
FYI, the Legacy code book is: Working effectively with Legacy Code by Michael Feathers. Its useful, I also strongly recommend it when you're feeling overwhelmed by a large sprawling code base.

https://www.amazon.com/Working-Effectively-Legacy-Michael-Fe...

by anonymous   2018-04-02
Why not improve the Grails Service you have? The problems you mentioned are not inherent in grails/groovy and in solving them you will probably uncover some aspects of the project that you don't know of now. If the tests are slow, what can you do to speed them up? What would a good API be for mocking the results for the tests that make http calls? I would recommend the book [Working Effectively with Legacy Code](https://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052), while it is quite old the advice in it is still mostly good.
by anonymous   2018-03-19

Indeed, there's code duplicated here, so it's a good idea to try to eliminate it.

You could do:

function askQuestion($data, $question){
  if($data !='') {
     echo "<span class='question'>$question</span><span class='answer'>".$data.'</span><br />';
   }
}

and use it like:

foreach ( $answers as $a )   {
  askQuestion($a->q2, "Question title?");
  askQuestion($a->q3, "Question label 2?");
  // and so on

And, as a general rule of thumb: when you're about to refactore code, don't forget to put in place non regression tests first (because it would be too bad to break the code when we're trying to improve it).

Last advice: if you have to work with legacy codebases on a regular basis, you might to read Working Effectively with legacy code, which gives a lot of practical tips to turn such codebases into stuff easier to work with.

by anonymous   2018-03-12
I'm not sure what you mean by sending events. I would recommend reading the book https://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052/ That book teaches you how to find the seams in your code where you can isolate behavior and just test what's important. The fact that your code is in private methods is good, but what about extracting those private methods into their own class and passing in all needed info in a public method that can be tested?
by bloat   2018-01-28
I would try and clean up the bits I was working on.

This is a good book on the topic refactoring a large code base with no tests.

https://www.amazon.co.uk/Working-Effectively-Legacy-Michael-...

by anonymous   2017-10-08
@gdbj good luck in any case, we've all been there! Strongly recommended reading: [Working Effectively With Legacy code by Michael Feathers](https://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052)
by PaulHoule   2017-10-06
If you have to write mocks in the native language, mocks will probably drive you insane.

Tools like mockito can make a big difference.

I worked on a project which was terribly conceived, specified, and implemented. My boss said that they shouldn't even have started it and shouldn't have hired the guy who wrote it! Because it had tests, however, it was salvageable, and I was able to get it into production.

This book

https://www.amazon.com/Working-Effectively-Legacy-Michael-Fe...

makes the case that unit tests should always run quickly, not depend on external dependencies, etc.

I do think a fast test suite is important, but there are some kinds of slower tests that can have a transformative impact on development:

* I wrote a "super hammer" test that smokes out a concurrent system for race conditions. It took a minute to run, but after that, I always knew that a critical part of the system did not have races (or if they did, they were hard to find)

* I wrote a test suite for a lightweight ORM system in PHP that would do real database queries. When the app was broken by an upgrade to MySQL, I had it working again in 20 minutes. When I wanted to use the same framework with MS SQL Server, it took about as long to port it.

* For deployment it helps to have an automated "smoke test" that will make sure that the most common failure modes didn't happen.

That said, TDD is most successful when you are in control of the system. In writing GUI code often the main uncertainty I've seen is mistrust of the underlying platform (today that could be, "Does it work in Safari?")

When it comes to servers and stuff, there is the issue of "can you make a test reproducible". For instance you might be able to make a "database" or "schema" inside a database with a random name and do all your stuff there. Or maybe you can spin one up in the cloud, or use Docker or something like that. It doesn't matter exactly how you do it, but you don't want to be the guy who nukes the production database (or a another developer's or testers database) because the build process has integration tests that use the same connection info as them.

https://www.amazon.com/Working-Effectively-Legacy-Michael-Fe...

by anonymous   2017-09-24
This may not be a nice experience. Consider reading "Working Effectively With Legacy Code" (https://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052)
by anonymous   2017-08-20

About testability

Due to the use of singletons and static classes MyViewModel isn't testable. Unit testing is about isolation. If you want to unit test some class (for example, MyViewModel) you need to be able to substitute its dependencies by test double (usually stub or mock). This ability comes only when you provide seams in your code. One of the best techniques used to provide seams is Dependency Injection. The best resource for learning DI is this book from Mark Seemann (Dependency Injection in .NET).

You can't easily substitute calls of static members. So if you use many static members then your design isn't perfect.

Of course, you can use unconstrained isolation framework such as Typemock Isolator, JustMock or Microsoft Fakes to fake static method calls but it costs money and it doesn't push you to better design. These frameworks are great for creating test harness for legacy code.

About design

  1. Constructor of MyViewModel is doing too much. Constructors should be simple.
  2. If the dependecy is null then constructor must throw ArgumentNullException but not silently log the error. Throwing an exception is a clear indication that your object isn't usable.

About testing framework

You can use any unit testing framework you like. Even MSTest, but personally I don't recommend it. NUnit and xUnit.net are MUCH better.

Further reading

  1. Mark Seeman - Dependency Injection in .NET
  2. Roy Osherove - The Art of Unit Testing (2nd Edition)
  3. Michael Feathers - Working Effectively with Legacy Code
  4. Gerard Meszaros - xUnit Test Patterns

Sample (using MvvmLight, NUnit and NSubstitute)

public class ViewModel : ViewModelBase
{
    public ViewModel(IMessenger messenger)
    {
        if (messenger == null)
            throw new ArgumentNullException("messenger");

        MessengerInstance = messenger;
    }

    public void SendMessage()
    {
        MessengerInstance.Send(Messages.SomeMessage);
    }
}

public static class Messages
{
    public static readonly string SomeMessage = "SomeMessage";
}

public class ViewModelTests
{
    private static ViewModel CreateViewModel(IMessenger messenger = null)
    {
        return new ViewModel(messenger ?? Substitute.For<IMessenger>());
    }

    [Test]
    public void Constructor_WithNullMessenger_ExpectedThrowsArgumentNullException()
    {
        var exception = Assert.Throws<ArgumentNullException>(() => new ViewModel(null));
        Assert.AreEqual("messenger", exception.ParamName);
    }

    [Test]
    public void SendMessage_ExpectedSendSomeMessageThroughMessenger()
    {
        // Arrange
        var messengerMock = Substitute.For<IMessenger>();
        var viewModel = CreateViewModel(messengerMock);

        // Act
        viewModel.SendMessage();

        // Assert
        messengerMock.Received().Send(Messages.SomeMessage);
    }
}
by Ruben Bartelink   2017-08-20

Other hanselminutes episodes on testing:

  • #112 The Past, Present and Future of .NET Unit Testing Frameworks - listened a while back, remember being slightly underwhelmed, but still worth a listen
  • #103 Quetzal Bradley on Testing after Unit Tests - extremely interesting, giving deep insight into how to think about the purpose of coverage metrics etc.
  • #146 Test Driven Development is Design - The Last Word on TDD (with Scott Bellware) - lives up to its name in that it slams home a lot of core concepts that you "always knew anyway". IIRC the podcast gives a favourable mention to the Newkirk/Vorontsov book - which I wouldnt particularly second (it's a long time since I read it -- I might just not have been ready to absorb its messages)

Other podcasts:

  • Herding Code Episode 42: Scott Bellware on BDD and Lean Development - recorded after Hanselminutes #146. Again, very good discussion that helps to cement ideas around "classic tests" vs BDD vs Context Specification and various such other attempts at classifications...
  • J.B. Rainsberger: "Integration Tests Are A Scam" is a recording of a presentation covering integration vs unit tests.

Other questions like this:

  • Good QA / Testing Podcast, which among others lists the meta podcast http://testingpodcast.com/

Blog posts:

  • It’s Not TDD, It’s Design By Example - Brad Wilson, similar in vein to HC #42, attempting to get across why you're writing the tests.

I know you didn't ask for books but... Can I also mention that Beck's TDD book is a must read, even though it may seem like a dated beginner book on first flick through (and Working Effectively with Legacy Code by Michael C. Feathers of course is the bible). Also, I'd append Martin(& Martin)'s Agile Principles, Patterns & Techniques as really helping in this regard. In this space (concise/distilled info on testing) also is the excellent Foundations of programming ebook. Goob books on testing I've read are The Art of Unit Testing and xUnit Test Patterns. The latter is an important antidote to the first as it is much more measured than Roy's book is very opinionated and offers a lot of unqualified 'facts' without properly going through the various options. Definitely recommend reading both books though. AOUT is very readable and gets you thinking, though it chooses specific [debatable] technologies; xUTP is in depth and neutral and really helps solidify your understanding. I read Pragmatic Unit Testing in C# with NUnit afterwards. It's good and balanced though slightly dated (it mentions RhinoMocks as a sidebar and doesnt mention Moq) - even if nothing is actually incorrect. An updated version of it would be a hands-down recommendation.

More recently I've re-read the Feathers book, which is timeless to a degree and covers important ground. However it's a more 'how, for 50 different wheres' in nature. It's definitely a must read though.

Most recently, I'm reading the excellent Growing Object-Oriented Software, Guided by Tests by Steve Freeman and Nat Pryce. I can't recommend it highly enough - it really ties everything together from big to small in terms of where TDD fits, and various levels of testing within a software architecture. While I'm throwing the kitchen sink in, Evans's DDD book is important too in terms of seeing the value of building things incrementally with maniacal refactoring in order to end up in a better place.