How to refactor large class that uses "Strategies"?

Question

Problem

I have a large class (about 1500 LOC) and it uses different "strategies" to transform data from one object to another. I have here a representation of that class:

public class FooService implements FooProcessing {
    FooRequestTransformer fooRequestTransformer;
    AnotherService anotherService;
    InstanceVar1 iVar1;
    InstanceVar2 iVar2;    
...

There is an interface (external to the class) that this class uses:

interface TransformerStrategy {
    public FooRequest transform(FooResponse response);
}

Which is passed into this method (inside the FooService class):

private FooResponse getResponse(FooResponse fooResponse, TransformerStrategy transformerStrategy) {
    FooRequest fooRequest = transformerStrategy.transform();
    fooResponse = anotherService.bar(fooRequest); 
    return fooResponse;
}

The entry point is here which uses the getResponse() method and creates a TransformerStrategy anonymously:

public List<Foo> getFooForSomeFlow1(Param1 param1, Param2 param2, ...){
    FooResponse fooResponse = anotherService.baz(fooRequest);

    TransformerStrategy myTransformerStrategy = new TransformerStrategy() {
        public FooRequest transform(FooResponse fooResponse) {
            fooRequestTransformer.transform(param1, param2, iVar1, iVar2) 
        }
    }

    FooResponse fooResponse = getResponse(fooResponse, myTransformerStrategy);
    ...//other code
}

Now the problem is: there are several methods like getFooForSomeFlow1() (inside FooService) that all have their own anonymous implementation of TransformerStrategy and subsequently call getResponse(). As you can imagine, this is very messy, as well as confusing when you debug (i.e. you are stepping into getResponse() and then all of a sudden you are back in getFooForSomeFlow1())

Possible solution

One possible solution (that comes to mind) is to organize these different strategies into a "Provider" class that will group them together somehow. Strangely enough, this class already includes this type of Provider class:

protected class StrategyProvider {
    public ABCTransformerStrategy newABCTransformerStrategy(FooRequestTransformer transformer, Param1 param1, Param2 param2) {
        return new ABCTransformerStrategy(transformer, param1, param2);
    }
}

protected class ABCTransformerStategy implements TransformerStrategy {
    protected FooRequestTransformer transformer;
    protected Param1 param1; 
    protected Param2 param2;

    //...constructor here

    //...overridden transform method as above
}

In one of the comments it says, "Converted anonymous class to inner class for testing purposes". However, they only converted one of them, and left the rest of them. So it's like they started the process of refactoring and stopped in the middle.

So, I was thinking I could finish the process of refactoring and move all the anonymous classes to inner classes, and then finally move out these classes and StrategyProvider into external classes. The problem is that "converting anonymous to inner" adds more boilerplate (see ABCTransformerStrategy above; I have to pass in all the data to the constructor) and I'm not really sure how much I'm gaining by doing this refactoring process.

I have two questions:

1. Should I continue with this approach?
2. Or is there another design pattern that I can apply that would be more appropriate and really simplify this code?

Thanks

Answer 1

I tried irreputable's approach and tried "Inline Method" on the getResponse() method, but it was inlining some duplicate code (which is why we extracted that duplicate code into getResponse() in the first place). I should have clarified that in my question that there is more code in getResponse() than what I showed.
As for making a Factory, I couldn't justify introducing more boilerplate and LOC into the class, especially since I have to pass a lot of data into the Factory Method and/or Inner Class.

What we did instead was, we wrapped the the anonymous inner classes with a method like so:

private TransformerStrategy createTransformerStrategyWithABCValues(Param1 param1, Param2 param2, IVar ivar, IVar1 ivar2) {
    return new TransformerStrategy() {
        public FooRequest transform(FooResponse response) {
            return FooRequestTransformer.transform(
                    param1,
                    param2,
                    iVar1,
                    iVar2);
        }
    };
}

Now the calling method looks like this:

public List<Foo> getFooForSomeFlow1(Param1 param1, Param2 param2, ...){
    FooResponse fooResponse = anotherService.baz(fooRequest);
    TransformerStrategy myTransformerStrategy = createTransformerStrategyWithABCValues(param2, param2, iVar1, iVar2);
    FooResponse fooResponse = getResponse(fooResponse, myTransformerStrategy);
    ...//other code
}

In the process of doing this, we found that the 5 strategies had some duplication, so we were able to get that down to 3 strategies. We got rid of the StrategyProvider class as well as the inner class that was provided.

With this approach it's now somewhat easier to debug/maintain, and we were able to cut quite a bit of duplication out of the code.

Answer 2

Based on the code sample you provided, it is superficially complicated. Why not simply

public List<Foo> getFooForSomeFlow1(Param1 param1, Param2 param2, ...)
{
    FooResponse fooResponse = anotherService.baz(fooRequest);

    FooRequest  fooRequest = fooRequestTransformer
                                 .transform(param1, param2, iVar1, iVar2); 

    FooResponse fooResponse2 = anotherService.bar(fooRequest);
    ...//other code
}

Unless there's something else going on that you haven't shown us.

Answer 3

Your provider class is actually a Factory pattern, which is exactly what I was going to suggest.

By moving the logic out of the getFooForSomeFlow1 and similar methods, you've created very loosely coupled code, which is always desirable.

Simply out of personal preference, I would have one method, used to return instances, that takes an int value as the Discriminator. These int's could be static final variables inside the provider class, like:

public static final int ABCTransformerStrategy = 1;

Additionally, by making the Factory class Abstract, you will remove the need to instantiate the class every time you need to use it, making for much cleaner code.

Edit

As suggested, using Enum is a much more desirable, and semantically correct way of translating values to the concrete classes they represent.

Answer 4

If you want to provide a bunch of related objects, you can always use the Abstract Factory. It is not a simple design pattern, but is the most suitable here for you.

Take a look at this: Abstract Factory

Answer 5

Convert each method into its own class (method object pattern i.e., method-as-object), which is similar to where the original refactor was going. If you need to centralize access to all of these methods-as-objects implement an abstract factory. From your description it sounds like you are transforming from one type to another, so make the abstract factory accept two Class parameters and return to the caller the appropriate method-as-object instance for the combination of classes.