Refactoring a long method that simply populates

Question

I am refactoring a method which is over 500 lines (don't ask me why)

The method basically queries a list of maps from the database and for each map in the list does some computation and adds the value of that computation to the map. There are however too many computations and puts being done that the code has reached over 500 lines already!

Here's a sample preview:

public List<Hashmap> getProductData(...) {
   List<Hashmap> products = productsDao.getProductData(...);
   for (Product product: products) {
       product.put("Volume",new BigDecimanl(product.get("Height")*
              product.get("Width")*product.get("Length"));
   //over 10 more lines like the one above
      if (some condition here) {
         //20 lines worth of product.put(..,..) 
      } else {
         //20 lines worth of product.put(..,..)
      }
      //3 more if-else statements like the one above
      try {
         product.put(..,..)
      } catch (Exception e) {
         product.put("",..)
      }
      //over 8 more try-catches of the form above
   }

Any ideas on how to go about refactoring this?

Answer 1

This depends on the business logic, but the most straightforward way to go is to split the long method into smaller ones:

for (Product product: products) {
   handleProduct(product);
}


private void handleProduct(Product product) throws Exception {
  if (condition1) {
     handleProduct1(product);
  }
}

This will make much more sense if each condition matches to a specific business case and has a meaningful name.

Answer 2

It's difficult to provide a detailed answer without more information but one approach you could consider is:

• Remodel a product using a Product class hierarchy rather than representing each product as a Map of (name, value) pairs. The current Map representation means that many errors are not going to be detected until run-time; better to prefer stronger typing so you can perform more checks at compile-time.
• Define a template method on the top-level Product class or interface that performs much of the processing currently defined in your 500 line method. For example, you could pass in a visitor to the template method as per the below code extract.

Example

/**
 * Our visitor definition, responsible for processing each product.
 */
public interface ProductVisitor {
  void processProductA(ConcreteProductA a);

  void processProductB(ConcreteProductB b);
}

/**
 * Top-level product definition.
 */
public interface Product {
  void process(ProductVisitor v);
}

/**
 * A conrete product implementation.  Makes a callback to the visitor
 * allowing itself to be processed.
 */
public class ConcreteProductA implements Product {
  public void process(ProductVisitor v) {
    v.processProductA(this);
  }
}

/**
 * As per the previous product implementation but makes a *different*
 * callback to the visitor.
 */
public class ConcreteProductB implements Product {
  public void process(ProductVisitor v) {
    v.processProductB(this);
  }
}

The main advantage with this approach is that you avoid lots of explicit if-then blocks in your code making the design more loosely coupled. The disadvantage is it can make code less readable.

Answer 3

A simple idea that comes to my mind for dividing a method is "find smaller tasks with sense" so here are some hints:

Make a method for the item-level

processCollection(collection) {
// startup code
for (Item i: collection)
  processCollectionItem(i, ...other args...);
// final code
}

Try to cut each commented block in its own method

// does a
instr 1
instr 2
instr 3
instr 4
// does b
instr 5
instr 6
instr 7
instr 8

converts to

a(...);
b(...);

Try to see if some bunch of lines are a concrete expression of some general pattern

map.put(a[0], b[0]);
map.put(a[1], b[1]);
...

converts to

putKeysAndValues(a, b);

with

putKeysAndValues(a, b) {
  for (int i=0; i<a.length; i++)
     map.put(a[i], b[i]);
}

Answer 4

you can shorten the method by moving the conditional puts to a separate method and invoke it from the current method.

eg.

...
if (some condition here) 
 { method1(map);} 
  else 
{method2(map);}

Also if there are common tasks, pull them in separate methods as well.

Answer 5

Assuming you can't change the dao... This is how I would approach it, not necessarily correct:

public List<Hashmap> getProductData(...) {
   List<Hashmap> products = productsDao.getProductData(...);
   try{
     addVolume(products);
     ...
     doCondition1(products);
     ...
   } catch (E1 e){
     ...
   } ... {
   }
}

public addVolume(List<HashMap> products) throws E1, E2, ... {
  for(HashMap product : products){
    product.put("Volume",new BigDecimanl(product.get("Height")*
              product.get("Width")*product.get("Length"));
  }
}

... do other things ...

public doCondition1(List<HashMap> products) throws E1, E2, ...{
  for(HashMap product : products){
    if(condition1){
      ...
    }else{
      ...
    }
  }
}

...condition 2, condition 3, ...

Of course, you can put the loop inside your

getProductData(...)