Is there a design pattern that I can use to simplify lots of nested methods

Question

I'm trying to refactor some legacy code that's quite a mess. We're talking a class with 7000 lines and a handfull of methods that are violating the Single Responsibilty Principle so much it hurts to look at. I've managed to break it up a little, but I find I'm mainly just shuffling the code around into separate classes - the complexity is still there. My methods are getting huge lists of parameters because everything is so tangled.

My aim is to add unit testing, but also make the code more maintainable as a result.

I'm working through the code from the "lowest points" (those that touch a database) and trying to refactor them out into classes that do specific things. The database access is nice and easy, I can pull that in to a "DataAccess" class.

However, I'm having trouble with some of the other methods. There's some areas that are doing a single general thing, say - loading some data. My problem is these methods all call each other in one long chain.

For example, here's some psudocode demonstrating the what I've got:

class SensorDataLoader
{
    //injected dependency for data access
    private readonly DataAccess dataAccess;

    // loads all sensors
    public void LoadAllSensorData(int dataSetId, LoadParametersObject loadParameters, DataStoreObject dataStore, Configuration configuration, Report report)
    {
        foreach (var sensorDataStore in dataStore.sensorsToLoad)
        {
            if (!sensorDataStore.HasDataToLoad) continue; // these parameters have lots of complexity. I've cut out a lot of stuff like this that access the configuration objects, or the data store objects.

            LoadSensorData(dataSetId, sensorDataStore, configuration.SensorConfiguration);
        }
    }

    // load a single sensor (i.e. all it's 'tracks')
    public void LoadSensorData(int dataSetId, SensorDataStoreObject sensorDataStore, SensorConfiguration sensorConfiguration, Report report)
    {
        foreach (var trackDataStore in sensorDataStore.tracks)
        {
            // here I've tried to create some dependencies that I can mock to test later methods
            var trackNumberAllocator = new TrackNumberAllocator(trackDataStore.trackId, sensorConfiguration);
            var postProcessor = new TrackPostProcessor(sensorConfiguration, trackDataStore.trackId, trackNumberAllocator);

            if (sensorConfiguration.someCondition)
            {
                LoadTrackData(dataSetId, trackDataStore, postProcessor, sensorConfiguration, report);
            }
            else
            {
                report.Report(...);
            }
        }
    }

    // loads a single 'track' in the sensor
    public void LoadTrackData(int dataSetId, TrackDataStoreObject trackDataStore, PostProcessor postProcessor, SensorConfiguration sensorConfiguration, Report report)
    {
        //get the data
        var data = this.dataAccess.GetData(dataSetId, sensorConfiguration);

        postProcessor.PostProcess(data);

        postProcessor.CalculateOffsets(data);

        report.Report(...);
    }
}

I've got three methods, that area all loading a sensor (if I split them into separate classes I'm basically going to have to have a separate class for every single method in this code!)

I want to test the methods in isolation, so I can test each one's functionality, but they're chained together.

After struggling to untangle this code, I've also realized that the structure of these classes is also really coupled with the strucutre of both the configuration objects, and 'data store' objects.

All these methods take a really complex object, and mutate it in some way. It's really tricky to untangle them because of it.

Are there any design patterns that could help me with this? I think I'm being a little too naive in simply trying to partition them off into separate classes and something more drastic is needed to separate concerns but I'm struggling to find a design pattern that fits.

Answer 1

Considering only your example given, I think this result is pretty fine and does not violate SRP, because you have a class SensorLoader with the responsibility of gathering all data from sensors.

I would do the following, given the code example:

• Make only the LoadAllSensorsData method public;
• Create unit tests passing stubs (or mocks) for the DataStore and Configuration objects;
• By doing the above, you'll be testing the main method, and as consequence you will test the other private methods that are called internally;
• Your code example follows the SRP and the DIP (dependency injection - code must depend on abstractions)

Regarding other parts of your legacy code, if you have complex code with lots of coupling between different objects, you could (after breaking it into separate classes like you did in the example) use patterns like Mediator if necessary, but I guess if you did the refactoring in the same way as the Sensor stuff, it will be just fine.

Answer 2

Nested methods can make refactoring difficult. One way around this is to replace the method calls with the method contents. Then you are sure that nothing has been broken, and I often find one big function easier to refactor. Then you can decide how to break the code up into smaller functions which are easier to unit test.