Working Effectively with Legacy Code

Yes. But it’s a great paying skill. There are techniques for working with legacy code.

This is the go to book for it. https://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

I think it would be a good exercise to learn something new and then go back to your old projects and apply them. For example, learn about testing and go back to your calculator and try to get a good chunk of it under tests (another good resource is Working Effectively with Legacy Code, which covers getting projects under tests when they don't have any). Then learn about CI/CD, which use tests, and make a build pipeline for your calculator. Then read Clean Code, read about architectures and patterns, and see if you can improve your architecture. This is just an example, but any of those could give you good talking points if interviewers ask about your projects.

Michael C. Feathers (who wrote the book Working Effectively With Legacy Code) defines legacy code as code without tests. If code is tested, you can modify or replace it with confidence and without tests, you can have no confidence that changes to the code won't break something. If you can't change your code without fear of it breaking, your code is legacy.

What you should have initially are integration tests. These will test that the functions perform as expected and you could hit the actual database for this.

Once you have that savety net you could start refactoring the code to be more maintainable and introducing unit tests.

As mentioned by serbrech Workign Effectively with Legacy code will help you to no end, I would strongly advise reading it even for greenfield projects.

http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

The main question I would ask is how often does the code change? If it is infrequent is it really worth the effort trying to introduce unit tests, if it is changed frequently then I would definatly consider cleaning it up a bit.

As far as I know, there are no tools for automatically bringing existing code under unit tests - if it were that easy, there should be no new bugs at all, right? As arne says in his answer, if code was not designed to be tested in the first place, it usually has to be changed to be testable.

The best you can do in my opinion is to learn some techniques of how to introduce unit tests with relatively few changes (so that you can introduce the unit tests before you start the "real" modifications); one book on this subject I've read recently is Michael Feathers' "Working Effectively with Legacy Code" (Amazon Link: http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052). Although it has some shortcomings, it has pretty detailed descriptions of techniques how you can easily introduce unit tests.

I've had this problem before and I've asked around (before the days of stack overflow) and this book has always been recommended to me. http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

This question came up recently on the RSpec mailing list, and the advice we generally gave was:

• Don't bother trying to retro-fit specs to existing, working, code unless you're going to change it - it's exhausting and, unless the code needs to be changed, rather pointless.
• Start writing specs for any changes you make from now on. Bug fixes are an especially good opportunity for this.
• Try to train yourself into the discipline that before you touch the code, first of all write a failing example (=spec) to drive out the change.

You may find that the design of code which wasn't driven out by code examples or unit tests makes it awkward to write tests or specs for. This is perhaps what your friend was alluding to. You will almost certainly need to learn a few key refactoring techniques to break up dependencies so that you can exercise each class in isolation from your specs. Michael Feathers' excellent book, Working Effectively With Legacy Code has some great material to help you learn this delicate skill.

I'd also encourage you to use the built-in spec:rcov rake task to generate code coverage stats. It's extremely rewarding to watch these numbers go up as you start to get your codebase under test.

This is often referred to an 'sensing' within a class under test: you have to find a way to tell whether a method has been called. These kinds of things are covered well in the book 'Working Effectively with Legacy Code' which everyone should have (disclaimer: I have no affiliation with the book's author, I just think it's a good book to have).

What does the CreateRegisteringMailMessage method do? Does it affect the content of the RegisterModel instance passed in during the call?

Answer to most of your questions http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

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...

I'd suggest the book [Working Effectively with Legacy Code](https://www.amazon.com/Working-Effectively-Legacy-Michael-Fe...).

This book is pretty great:

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

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.

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.

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.

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.

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);
}

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...

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...

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.