2

I've got some legacy code (both in the sense of code that has no tests, like the definition given in Working Effectively with Legacy Code, and of relatively old code) that I have to extend. There is this one class in particular, which is extensively used through the 'simple' expedient of accessing its member variables directly and using said member's methods (sometimes chained to a depth of 3 classes). That makes it a bit of a nightmare to work with, much to my displeasure.

So I'm trying to get that class under tests, and get it to the point where I have a usable interface to access it. However I'm not seeing a clear path between the current state and the end result.

After giving it some thought, my current plan is to first wrap all of the uses of members (up to the necessary depth) in the class, which will unfortunately turn it into a bit of a god class. Once every access is done through a proper set of functions, I hope to get the whole assembly of the class hooked into the test framework. After that I hope to be able to start extracting the different interfaces or classes hiding in the mess that will have been produced by the wrapping step.

Does that sound like a sensible approach? Is there another approach that would work out better in a case like this?

Iker
  • 865
  • 1
  • 6
  • 11
  • 1
    Possible duplicate of [I've inherited 200K lines of spaghetti code -- what now?](http://programmers.stackexchange.com/questions/155488/ive-inherited-200k-lines-of-spaghetti-code-what-now) –  Feb 26 '16 at 22:11
  • 2
    I don't think it's a duplicate because I'm asking about specific steps in a specific situation (highly coupled class as a result of not hiding implementation details), as opposed to general advice. If that's not clear from the question, I'd appreciate advice on how to better express it. – Iker Feb 27 '16 at 10:34
  • I thought about it before voting as a duplicate, and I think the top answer does address your concerns (keep in mind it is a huge answer). But I have also never been a fan of "question A is more general than question B, so B is a duplicate of A" which is official Stack Exchange policy. But I think it is possible to provide a _better_ answer here, so I retracted my vote. –  Feb 27 '16 at 17:34

1 Answers1

1

There is no silver bullet to such a problem. By its very nature you can find many solutions and the suitability of each solution will depend on how the rest of the code is structured and the functions it performs.

There are several ways to proceed and what follows is just one technique that I have used before myself when faced with a very similar problem. For the sake of brevity, I will constantly refer to your code as application but it can be a service or a library too.

Prerequisites

Before you proceed make sure you have the done the following:

  1. Understand what your application is supposed to do. This goes well beyond just the requirements specification. The more you understand the finer points of your application's behaviour the better off you will be. Also, do not fear to challenge the spec (if you have a direct line to your users). You may find that sometimes people don't really care about a particular functionality/behaviour that you thought was mandatory. Sometimes you may even find functionality/behaviour that people hate. This extends not just to human users but also to system users (e.g., programs using your API). However, gather consensus before proceeding with this approach.

  2. Start writing deterministic tests for your application. You should aim to cover at least the broad "specs". Once you have these tests written you can refactor easily. Tests that are written as code should be preferred (e.g., integration/functional tests). However, if you cannot code test cases then document the manual testing procedure so that you (or even better someone else) can replicate it in an error free way.

  3. Start a log book. Your SCM can help you track changes and recover a previous version. So make changes in small increments and test them. Once you and an independent tester both are sure your changes are good then check-in and move on to the next bit. Good change descriptions and notes in your log book can help you follow your tracks in case something bad happens in the future. If others join your effort they can study your notes to read your mind.

Ok, what now?

Once you are familiar with the application and have the test cases ready you should try to attack the usages of your BadClass. Try to replace this class's access by a better pattern. For example, if your class's members are being accessed by another class try to see if the callee instead should have that member available as a class field or method parameter. See if instead of making these members visible the BadClass can provide a trait/function to be used --try to see if that trait should be placed in a different class altogether. In general, you are replacing the usage with a better pattern instead of trying to fix the BadClass.

If you know your application or domain well then you can perhaps even figure out if the usage is required or not (may be you can just chuck away that functionality or replace it with something better).

When attempting this approach, avoid trying to solve too many problems at once. Aim for smaller improvements first (low hanging fruits). See if that simplifies the problem to an extent where you can introduce a better pattern.

Once the BadClass is no longer referenced, you can chuck it away.

Apoorv
  • 1,128
  • 1
  • 6
  • 15