How to correctly refactor this?

Question

I have trouble finding way to correctly refactor this code so that there would be as little duplicate code as possible, I have a couple of methods like this (pseudocode):

public List<Something> parseSomething(Node n){
  List<Something> somethings = new ArrayList<Something>();
  initialize();
  sameCodeInBothClasses();
  List<Node> nodes = getChildrenByName(n, "somename");
  for(Node n:nodes){
    method();
    actionA();
    somethings.add(new Something(actionB());
  }
  return somethings;
}

methods sameCodeInBothClasses() are same in all classes but where it differs is what hapens in for loop actionA() and it also adds an element to the List of different type.

Should I use Strategy pattern for the different parts inside loop?

What about the return value (The type of list differs), should the method return just List<Object> that I would then cast to appropriate type? Should I pass the class I want to return as parameter?

Answer 1

The applicable design pattern is Template Method, rather than Strategy.

About the different type of items, I would try genericizing parseSomething first, like this:

public <T> List<T> parseSomething(Node n){
  List<T> somethings = new ArrayList<T>();
  initialize();
  sameCodeInBothClasses();
  List<Node> nodes = getChildrenByName(n, "somename");
  for(Node n:nodes){
    method();
    actionA();
    somethings.add(new T(actionB());
  }
  return somethings;
}

This may not work for you straight away though. You may need to move the generic parameter to the class level.

Returning List<Object> will not work, as generic collections are invariant: for any two distinct types Type1 and Type2, List<Type1> is neither a subtype nor a supertype of List<Type2> (even if Type1 and Type2 are related, i.e. one is a subtype of the other!). See an earlier answer of mine for more details on this.

So if all else fails, the quick and dirty solution would be indeed to use a non generic List.

Answer 2

If you write it like this:

public List<T> parseGeneric(Node n) {
  List<T> result = new ArrayList<T>();
  initialize();
  sameCodeInBothClasses();
  List<Node> nodes = getChildrenByName(n, "somename");
  for(Node n:nodes){
    method();
    actionA();
    result.add(new T(actionB()));
  }
  return result;
}

And invoke it as

List<Something> somethings = parseGeneric(aSomething);

It's hard to say with all these other methods sprinkled around without a definition available, but I believe you should be able to get away with something like the above.

Answer 3

I'd create an Action interface for methods actionA and actionB. Then a Parser class with a method parseSomething:

public List parseSomething(Node n, Action a) {
  List something = new ArrayList();
  initialize();
  samecode();
  List<Node> nodes = getChildrenByName(n, "name");
  for(Node n: nodes) {
      a.actionA();
      somethings.add(new Something(a.actionB()));
  }
  return somethings;
}

Answer 4

My first idea would be to declare parseSomething in an abstract base class, with the methods that are different for each class declared as abstract and implemented in the subclasses. The inline code that creates a Something instance would need to be turned into a factory method.

The problem is that the signature of parseSomething needs to return notionally different types; e.g. List<Something> versus List<SomethingElse>. One approach would be to identify a common supertype and make the return type List<? extends SomeSuper>. Another approach would be to create parseXxx methods with the required signatures in each leaf class and have them delegate to a protected List<? extends SomeSuper> doParse(...) method in the base class.