1

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.

6
  • 4
    Yep, you don't apply or follow design patterns. You solve first, and then if you notice that your solution already has a name, use that name for clarity.
    – Useless
    Apr 30, 2020 at 15:51
  • 1
    It certainly seems I was barking up the wrong tree. Thanks for the reference and the advice.
    – hpabst
    Apr 30, 2020 at 15:53
  • Not a problem! Patterns are frequently presented as solutions to be applied. What they actually are is names given to solutions which have been observed to recur independently. They're great for consistent naming, and communication, they're just not a toolbox.
    – Useless
    Apr 30, 2020 at 15:58
  • Do you need to enable/disable transformations dynamically? Because it seems to me the simplest solution from you descriptions is to just to delete transformations from the code when you don't need them anymore.
    – JacquesB
    Apr 30, 2020 at 19:05
  • See also this book; the main idea is to find and/or introduce "seams" into the code - places where you can replace an inconvenient external dependency with a fake - so that you can introduce tests in some form (not necessarily TDD-like tests). Make the code temporarily ugly so that it's testable; the tests then serve as a safety net for refactoring. May 1, 2020 at 0:17

3 Answers 3

3

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.

1
  • Also consider testing your tests wit mutation testing e.g. with pitest.org because code coverage should not be measured without it
    – JohannesB
    May 5, 2020 at 15:08
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 =)

0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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