Approach for rewriting a large, mission-critical method

Question

Context: I am a new hire out of university at a large software company tasked with either refactoring or re-writing a large legacy method (~500 lines, ~2000 lines expanded with private method calls) that performs a complex workflow with many responsibilities. You can imagine it applying a complex series of interdependent transformations to its input before returning the transformed data. Static code analysis indicates it has a cyclomatic complexity of 67. This class is part of a package used across multiple services not owned by my team and as such the interface cannot be modified. This method is part of a package that is being deprecated feature-by-feature over a long period of time. The goal of the refactor/rewrite is to make it so that we maintain feature parity with the original implementation, but can easily disable individual transformations over time. The method has few tests, so my first task will be to create a full test suite for the class to enforce stable behavior.

What sort of approach can I apply in order to successfully perform this refactor? My current idea is some sort of Pipeline where we take a collection of enums which describe which transformations to disable when assembling the pipeline. This way deprecating transformations in the future is a matter of adding an additional enum to the parameter. E.g. Something along the lines of

public class LegacyClass {
...
    public WorkflowResponse performWorkflow(WorkflowInput workflowInput) {
        WorkflowPipeline pipeline = this.assemblePipeline(this.featureDeprecations);
        pipeline.validate();
        return pipeline.execute();
    }

    private WorkflowPipeline assemblePipeline(EnumSet<FeatureDeprecations> featureDeprecations) {
        WorkflowPipeline pipeline = new WorkflowPipeline();
        pipeline.addTransformation(new TransformationOne());
        if(!featureDeprecations.contains(DEPRECATE_TRANSFORMATION_TWO)) {
            pipeline.addTransformation(new TransformationTwo());
        }
        pipeline.addTransformation(new TransformationThree());
        return pipeline;
    }

Is following this approach a good idea for my situation? The main downside I can see is that the code for assembling the pipeline will grow to be very complex even if I manage to encapsulate each transformation rather than having them live within the performWorkflow method.

Answer 1

Is following this approach a good idea for my situation?

Only having this 50.000 feet view presented, I would say yes. Splitting up a complex process into several small, independent steps with a clear dataflow is an excellent, praxis-proven approach for keeping such kind of processes manageable. You can (and should!) write unit tests for each of the individual transformation, and it should be possible for you to run them in isolation.

The main downside I can see is that the code for assembling the pipeline will grow to be very complex

Not if you stay away from putting business logic and flow-of-control into the assembling code. If the only kind of logic there is the feature-flag evaluation, the assembling might become longish, but not very complex.

Don't forget the most important rule for such refactorings: make sure you have enough automated regression tests in place before you start to change anything. Then run these tests frequently, whenever you factored a new transformation out, otherwise you run into trouble! Writing such tests may require some effort up-front, but it is definitely worth it.

Answer 2

That seems to be an interesting challenge indeed. As @DocBrown and you mentioned, make sure to have tests before changing the code.

I may be late to the party, but I have a technique for you to set these tests quite rapidly. It's called "Approval Testing" (it has other names like "Golden Master" or "Characterization Tests").

The recipe is:

1. Execute the method under a specific context
2. Capture the output
3. Test that the method still produce the expected output
4. Add another test to execute the method under a different context
5. Capture the output & test the method still produce this output in this context
6. Repeat until you've covered all scenarios (test coverage helps here)

This can be automated, have a look at https://approvaltests.com/

Once you get there, you can play around with the code and know instantly if you broke anything.

As for the refactoring, what you described is good. Don't rush for Design Patterns indeed, split the code into distinct responsibilities.

By working with the code, you'll know more about it and find the relevant abstractions. Keep the business logic separated from the assembly code. It should be easy to unit test your distinct responsibilities in the end.

I hope that would be helpful. Don't hesitate to ask further questions while you're refactoring this method, I'd be happy to help =)

Answer 3

This class is part of a package used across multiple services not owned by my team and as such the interface cannot be modified

Often the biggest win is in weakening these dependencies. That's not in scope for this question, and probably above your pay grade at this point, but it's worth bearing in mind in case you have the opportunity to look at it later.

You can imagine it applying a complex series of interdependent transformations to its input before returning the transformed data

This doesn't sound exactly like a pipeline, although if you can refactor it correctly into a linear (or anyway acyclic) dataflow, that's definitely an improvement in itself.

The goal of the refactor/rewrite is to make it so that we maintain feature parity with the original implementation, but can easily disable individual transformations over time

Sounds reasonable enough. It's worth noting that since disabling pipeline stages affects the input to subsequent stages, getting good test coverage may be difficult. In principle, you could test only the combinations of stages you intend to actually use, but it'd impair your ability to re-order (or re-prioritize, or otherwise revisit) the retirement sequence later.

The method has few tests, so my first task will be to create a full test suite for the class to enforce stable behavior

Always a good idea. Assuming that disabling a transformation will also change the result, you may probably want integration tests with your clients as well.

As to your sketched implementation, there are a couple of things that occur to me:

1. Enumerating the features that are disabled feels ... backwards.

   What if you want to merge two stages into a better implementation instead of retiring one? Making the whole pipeline configurable and specifying explicitly which stages to run (instead of which stages to omit, from an implicit set) seems clearer and more flexible.

   It may, of course, make sense in your specific case.

2. You have a stateful workflow wrapper that assembles and executes an identical pipeline each time.

   If you're making a configurable pipeline, it probably deserves its own factory.

   If you're intending the pipeline to be testable, it should also be internally stateless, in which case you can build it once and keep it around.

The main downside I can see is that the code for assembling the pipeline will grow to be very complex even if I manage to encapsulate each transformation

I feel like writing a relatively generic pipeline factory is actually easier than writing a complex function which occasionally disables parts of itself. Certainly the cyclomatic complexity is lower. If you just give each distinct stage a name, you can configure the whole thing with a string (or array of strings, or whatever).

All of this assumes, of course, that your pipeline is homogenous: if your stages have different input and output types, generic assembly is harder.