How to refactor this code

Question

I am currently refactoring some old code. I am looking for directions on the best design pattern to use here. I am thinking of the factory pattern but I am not sure if that's the best way to go or not.

So here is a brief outline of the pseudocode. Class Foo has the core business logic in it.

Class Foo{
    private List<Things> stuff;
    private static Integer count; 
    //getter setter for stuff

    public Foo(List<Things> stuff){
        this.stuff = stuff; 
        this.count=1;
    }

    //the following 3 methods are 90% similar

    public List<Newthings> doSomeThingFirst(){
        //uses the list stuff and also increments the static member count for each entry in List<NewThings> based on certain conditions
    }

    public List<Newthings> doSomethingSecond(){
        //uses the list stuff and also increments the static member count for each entry in List<NewThings> based on certain conditions
    }

    public List<Newthings> doSomethingThird(){
        //uses the list stuff and also increments the static member count for each entry in List<NewThings> based on certain conditions
    }

    //in the future there may be doSomethingFourth(), doSomethingFifth() ... etc.

}


The caller of class Foo looks something like below.

Class SomeServiceImpl{
    public List<Things> getAllFoo(List<Things> stuff){
        Map<Integer,NewThings> fooList = new HashMap<Integer,NewThings>();
        Foo foo = new Foo(stuff);
        fooList.put(1,foo.doSomeThingFirst());
        fooList.put(2,foo.doSomeThingSecond());
        fooList.put(3,foo.doSomeThingThird());
            return new ArrayList<Things>(fooList.values());

    }
}

Let me know how do you think this code should be refactored for maintainability and reuse or is it fine as is?

Thanks for your inputs.

Answer 1

Looks like the BUILDER pattern calling the methods in order.

link http://en.wikipedia.org/wiki/Builder_pattern

Probably the STRATEGY pattern to remove the 90% duplications

link http://en.wikipedia.org/wiki/Strategy_pattern

Answer 2

 // the following 3 methods are 90% similar

Since you didn't offer actual code, that's the part that you factor out.

Answer 3

First of all You need to replace the List with the Map

Class SomeServiceImpl{
    public getAllFoo(List<Things> stuff){
        Map<Integer,NewThings> fooMap = new HashMap<Integer,NewThings>();
        Foo foo = new Foo(stuff);
        fooMap.add(1,foo.doSomeThingFirst());
        fooMap.add(2,foo.doSomeThingSecond());
        fooMap.add(3,foo.doSomeThingThird());

    }
}

Next thing is that the example is really pore with out logic and it is hard to understand what You really want to do.

The class SomeServiceImpl is doing some operations on the list but under the value 1,2,3 you have always the same result. You perform the operation while adding to list not when You retrieve the list from that map.

I think that You have choose wrong design pattern, instead fatory You should use Builder pattern.

http://en.wikipedia.org/wiki/Builder_pattern#Java

Answer 4

I'd say factor out the 90% into a public method that provides the services to the caller, and then have private helper methods to take care of doFirst(), doSecond(), etc. That way, if you add more of them, your public API won't change and break clients.

Answer 5

Command pattern could help here. This is a typical case of applying the principle of "Encapsulate that varies". The code would be:

public interface Command {
  public List<NewThings> doSomething();
}

class DoSomethingFirst implements Command {
  public DoSomethingFirst(Foo foo) {
  ...
  }
  public List<NewThings> doSomething() {
  ...
  }
}

then your service code will be

   fooList.put(1,(new DoSomeThingFirst(foo)).doSomething());
   fooList.put(2,(new DoSomeThingSecond(foo)).doSomething());
   fooList.put(3,(new DoSomeThingThird(foo)).doSomething());

thus the methods in Foo can be removed and delegate your actions to classes. This is my view more flexible than the previous approach as you can add as many actions - even at runtime.

If the Command derived classes have much of the code similar do something as:

public abstract class DoSomethingBasic implements Command {
...
// common code here.
...
} 

public class DoSomethingFirst extends DoSomething {
  public list<NewThings> doSomething() {
    super.doSomething(); //if possible
    ...
  }
}

my 2 cents.