Refactoring advice: maps to POJOs

I currently am part of a project where there is an interface like this:

public interface RepositoryOperation {

    public OperationResult execute(Map<RepOpParam, Object> params);  
}

This interface has about ~100 implementers.

To call an implementer one needs to do the following:

final Map<RepOpParam, Object> opParams = new HashMap<RepOpParam, Object>();
opParams.put(ParamName.NAME1, val1);
opParams.put(ParamName.NAME2, val2);

Now I think that there is obviously something wrong with anything with a<Something, Object> generic declaration.

Currently this causes a caller of a OperationImpl to have to actually read the code of the operation in order to know how to build the argument map. (and this is not even the worst of the problems, but I don't want to cite them all since they are fairly obvious)

After some discussion I managed to convince my colleagues to let me do some refactoring.

It seems to me that the simplest 'fix' would be to change the interface like so:

public interface RepositoryOperation {

    public OperationResult execute(OperationParam param);  
}

After all the concrete operations will define (extend) their own OperationParam and the needed arguments would be visible to everybody. (which is the 'normal way' to do things like that IMHO)

So as I see it since the interface implementers are quite numerous I have several choices:

1. Try to change the interface and rewrite all the Operation calls to use objects instead of maps. This seems the cleanest, but I think that since the operations are a lot it might be too muc开发者_运维百科h work in practice. (~2 weeks with tests probably)

2. Add an additional method to the interface like so:

   public interface RepositoryOperation {
       public OperationResult execute(Map<String, Object> params);
       public OperationResult execute(OperationParam params);  
   }
   

   and fix the map calls whenever I come across them during functionality implementation.

3. Live with it (please no !).

So my question is.

Does anyone see a better approach for 'fixing' the maps and if you do would you fix them with method 1 or 2 or not fix them at all.

EDIT: Thanks for the great answers. I would accept both Max's and Riduidel's answers if I could, but since I can't I'm leaning a bit more towards Riduidel's.

I can see a third way.

You have a map made of <RepOpParam, Object>. If I understand you correctly, what bothers you is the fact that there is no type checking. And obviously, it's not ideal. But, it is possible to move the type-checking issue from the whole parameter (your OperationParam) to individual RepOpParam. Let me explain it.

Suppose your RepOpParam interface (which currently seems like a tagging interface) is modified as it :

public interface RepOpParam<Value> {
    public Value getValue(Map<RepOpParam, Object> parameters);
}

You can then update modern code by replacing old calls to

String myName = (String) params.get(ParamName.NAME1);

with new calls to

String myName = ParamName.NAME1.getValue(params);

The obvious collateral advantage being that you can now have a default value for your parameter, hidden in its very definition.

I have however to make clear that this third way is nothing more than a way to merge your two operations of the second way into only one, respecting old code prototype, while adding new powers in it. As a consequence, I would personnally go the first way, and rewrite all that "stuff", using modern objects (besides, consider taking a look at configuration librarires, which may lead you to interesting anwsers to this problem).

First of all, I think the interface is not perfect. You could add some generics to make it prettier:

public interface RepositoryOperation<P extends OperationParam, R extends OperationResult> {
    public R execute(T params);  
}

Now, we will need some backward compatibility code. I'd go with this:

//We are using basic types for deprecated operations
public abstract class DeprecatedRepositoryOperation implements RepositoryOperation<OperationParam, OperationResult> {
    //Each of our operations will need to implement this method
    public abstract OperationResult execute(Map<String, Object> params);

    //This will be the method that we can call from the outside
    public OperationResult execute(OperationParam params) {
        Map<String, Object> paramMap = getMapFromObj(params);
        return execute(paramMap);
    }
}

Here is how will old operation look like:

public class SomeOldOperation extends DeprecatedRepositoryOperation {
    public OperationResult execute(Map<String, Object> params) {
        //Same old code as was here before. Nothing changes
    }
}

New operation will be prettier:

public class DeleteOperation implements RepositoryOperation<DeleteParam, DeleteResult> {
    public DeleteResult execute(DeleteParam param) {
        database.delete(param.getID());
        ...
    }
}

But the calling code can use both functions now (an example of code):

String operationName = getOperationName(); //="Delete"
Map<String, RepositoryOperation> operationMap = getOperations(); //=List of all operations
OperationParam param = getParam(); //=DeleteParam   
operationMap.execute(param);

In case the operation was old one - it will use the converter method from DeprecatedRepositoryOperation. In case the operation is a new one - it will use the new public R execute(T params) function.

It sounds like you have an unnecessary and misguided abstraction. Anytime I see an interface with one method in it, I think Strategy pattern or Action pattern, depending on whether you make the decision at runtime or not.

One way to cleanup the code is have each RepositoryOperation implementation have a constructor which takes the specific arguments it needs for the execute method to run correctly. That way there is no messy casting of the Object values in the map.

If you want to keep the execute method signature, you might be able to use generics to putter tighter bounds on the values of the Map.