3

I've been given the task of refactoring some legacy code, the main reason is to add unit tests (but I'm a firm believer that testable code is better code, so we're trying to reduce technical debt too). The current code is a big mess, there's a handfull of classes, one is 6000 lines, and has methods 300 lines long etc.

First thing I've been doing is splitting out data access for mocking, that's a nice independent thing. However, I've noticed that these classes, buy their nature, tend to have one 'entry' point, say a single method ProcessData.

Theoretically this could be the only public method. But I definately want to test these methods independently:

public class DataProcessor {

    public void ProcessData(...) {
        // this is the 'enty point'
        if (IsAlreadyProcessed())
            return;

        for (...) {
             ProcessSubData(...)
        }
    }

    public void ProcessSubData(...) {
        if (IsDataTypeA)
            ProcessWithMethodA(...);
        else if (IsDataTypeB)
            ProcessWithMethodB(...);
    }

    public bool IsAlreadyProcessed(...) {
        // do some complex stuff here I want to test
        ...
    }

    public void ProcessWithMethodA(...) {
        // something I definately want to test separately
    }

    public void ProcessWithMethodB(...) {
        // something I definately want to test separately
    }
}

Testing the 'root' method will be tricky, and involve some very hefty mock objects given the scope of all it's doing. Ideally I want to test the other methods, then just test that ProcessData calls them.

Basically, it has a dependency on itself - should I inject a mock version of DataProcessor into itself so I can test only sections of it?

The other option would be to split out the other Process methods into a separate class, and have that mockable and injectable into the DataProcessor - but to do this for each 'level' that these methods call each other would require a few or more extra classes that only exist to explode it out so it can be tested.

2
  • I think it's probably best to split it into separate classes
    – Joe
    Nov 23, 2017 at 14:53
  • 1
    IMHO this is not a QA question but softwareengineering.stackexchange.com question. It is about how code should be (re) designed. But it is a good question, not sure how we can get a response from SE community. Nov 29, 2017 at 22:53

2 Answers 2

1

Apparently, you have a case of violation of the Single Responsibility Principle, because an object (since one cannot test a class) of the type DataProcessor requires multiple setups for different operations.

It's a fairly common state in legacy spaghetti code. Fowler's book Refactoring may help you in this task.

Besides that, I'd add the suggestion to go slowly - breaking only the necessary to make the next step. This way, you will starting to guess where you can abstract and speed up your coding.

Injecting a DataProcessor object would make it more complex (testing != production, requiring adding a or multiple if-else).

0

It would be better to refactor the code. You should keep the DataProcessor class contract intact so that classes depend on it, keep working with minimal change.

Just improve internal design of the DataProcessor class. Keep SOLID principles in mind. Don't hesitate to extract new concepts and classes out of it, and code to a contract.

Then you will reach to a testable situation.

One of main ideas behind unit-testing is to reach a better code quality. So in this job always refactor the code under test for a better design.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.