Working Effectively with Legacy Code

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

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.

Working Effectively with Legacy Code

4.5

Review Date:

Comments

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?