CODEWITHSUNDEEP
java
rocketboy
rocketboy

Reputation: 9741

Utilize an existing method for more while refactoring

I have run into such cases on 3 different occasions now. Most of the times when refactoring some code.

Lets say I have:

//code block A
List<Bar> foo = doSomething(inputParams);
//code block B

Now, I need to refactor the code such that I want to use doSomething() method's process to do something else too. Lets say,create a map too (Bar.id -> Bar.name). Is there an elegant way to do this without passing a mutable map to doSomething() and not duplicating the code to another doSomethingDifferent()?

Dummy implementation:

doSomething(){
   List<Bar> ret = new ArrayList<Bar>();
   while(//condition){
     ret.add(new Bar());
   }
   returrn ret;
}


doSomethingDifferently(){
   Map<Integer, String> ret = new HashMap<Integer, String>();
   while(//condition){
     Bar b = new Bar()
     ret.put(b.getId(),b.getName());
   }
   returrn ret;
}

Summary:

Is there a better way than the possible solutions below?

Solution 1:(repeated code)

List<Bar> foo = doSomething(inputParams);
Map<Integer,String> foobar = doSomethingDifferent(inputParams); //Very similar to doSomething

Solution 2:(hard to read)

Map<Integer,String> foobar = new HashMap<Integer,String>();
List<Bar> foo = doSomething(inputParams, foobar); //mutate the map

Upvotes: 1

Views: 87

Answers (2)

zapl
zapl

Reputation: 63955

What your example methods do is different enough to have different method names. E.g. createListFrom(param), createMapFrom(param). Combining that with the same name is just confusing. And I would not count calling doSomething in place and doSomethingElse in another place repetition. The broader concept is maybe repeating.

One way to approach this is to move duplicate code into another method. Same / similar code is

  • while(condition) {}
  • Bar b = new Bar()
  • add b somehow to some sort of collection.

The generic version of your methods could look like

private void doSomethingGeneric(? param, GenericWayToHandle handler) {
    while (condition) {
        Bar b = createBar();
        handler.doSomethingWith(b);
    }
}

"Short" example how you could implement that

public List<Object> doSomethingList(int param) {
    ListHandler handler = new ListHandler();
    doSomethingGeneric(param, handler);
    return handler.list;
}

public Map<Object, Object> doSomethingMap(int param) {
    MapHandler handler = new MapHandler();
    doSomethingGeneric(param, handler);
    return handler.map;
}

private void doSomethingGeneric(int param, CollectionHandler handler) {
    for (int i = 0; i < param; i++) {
        handler.handle("Hello");
    }
}
private interface CollectionHandler {
    void handle(String string);
}

private static class MapHandler implements CollectionHandler {
    public final Map<Object, Object> map = new HashMap<Object, Object>();

    @Override
    public void handle(String string) {
        map.put(string, string);
    }
}

private static class ListHandler implements CollectionHandler {
    public final List<Object> list = new ArrayList<Object>();

    @Override
    public void handle(String string) {
        list.add(string);
    }
}

It's unfortunately pretty ugly to handle for each situations like that and Java 8 will simplify that via closures.

The other way to approach your problem is to use the output of one method to derive another version from it.

e.g. (very similar to what you posted)

List<Bar> bars = createBarList(inputParams); // doSomething
Map<Integer,String> foobar = deriveMap(bars); // doSomethingSimilar

where deriveMap would just iterate over the list and create a map. That's the place where this code would be now

for(Bar b: input) 
    ret.put(b.getId(),b.getName());

It's ultimately up to you what those methods do and how they should be used. The name of a method can help a lot to express the intention & help to use them correctly. Don't give that up by merging functionality into magical functions that behave unpredictably when you don't know the source code.

One other other thing: broader refactoring can often get rid of weird structures. Maybe you can encapsulate the whole (list & map of Bars)-thing into it's own class. Specialized data-structures are very often a good candidate to be moved out of a class that uses them. A class that is responsible to handle a map and a list of things could be seen as responsible for doing more than one thing -> Single responsibility principle

Upvotes: 1

Tala
Tala

Reputation: 8928

You're returning different data structures..

List<Bar> foo = doSomething(inputParams);
Map<Integer,String> foobar = doSomethingDifferent(inputParams);

Are you sure they do similar things? If so, you could extract common part or change them to return same type and after that it will be easily seen what you can do not to duplicate code.

Upvotes: 3

Related Questions