try-catch-finally vs abstract methods

in our system we have an abstract class, let's call it BasicAction, which contains several abstract methods. Most important of them is called execute. It handles the requests from the JSP pages. The main handler works like this:

// Sample 1: 
String actionName;// The name of action to use
Map<String, BasicAction> mapping;//The mapping of names and actual handlers

BasicAction action = mapping.get(actionName);

try {
  action.execute(request);//Handle the http request from the JSP page
} catch (Exception ex) {
  // Handle here any exceptions
}

Now, everything looks fine, but basically all the derived handlers implement the same code:

// Sample 1: 
public class ConcreteAction extends BasicAction {
  @Override
  public void execute(HttpServletRequest request) {
    // The managers provide the middle layer between 
    // web presentation and database
    TrafficManager trafficManager = null;
    CargoManager cargoManager = null;
    try {
      trafficManager = new TrafficManager(); 
      cargoManager = new CargoManager();
      // Perform here all the logic required using managers
    } catch (Exception ex) {
       // handle the exception or rethrow it
    } finally {
      // Should remove all managers clearly and release the connection
      removeManager(trafficManager);
      removeManager(cargoManager);
    }
  }
}

It seems a little bit annoying to write such blocks in every handler that I have. It seems like here we imitate the enter/exit points for each handler not as supposed to. I think what we need here is to define in the BasicAction two more abstract methods called createManagers and disposeManagers. Then the main handler would look like this:

// Sample 2: 
String actionName;// The开发者_运维技巧 name of action to use
Map<String, BasicAction> mapping;//The mapping of names and actual handlers

BasicAction action = mapping.get(actionName);

try {
  action.createManagers(); // The enter point
  action.execute(request);//Handle the http request from the JSP page
} catch (Exception ex) {
  // Handle here any exceptions
} finally {
  action.disposeManagers(); // The exit point
}

After that, each deriving action handler can be defined like this:

// Sample 2: 
public class ConcreteAction extends BasicAction {
  private TrafficManager trafficManager = null;
  private CargoManager cargoManager = null;

  @Override
  public void createManagers() {
    trafficManager = new TrafficManager();
    cargoManager = new CargoManager();
  }

  @Override
  public void disposeManagers() {
    removeManager(trafficManager);
    removeManager(cargoManager);
  }

  @Override
  public void execute(HttpServletRequest request) {
    // Perform here all the logic required using managers    
  }
}

Which approach is better to use - with try-catch-finally in each handler or with standard enter/exit points.

Personally, I would go for the abstract class approach, as it sounds like the Strategy pattern. Moreover, the code is cleaner and easier to follow; plus - you are not repeating the structure over and over. But that is only my opinion, someone might suggest the other way.

Let createManagers() return a list of managers. The calling class can then call removeManager() on every one, making the disposeManagers() unneeded.

Also, you couple your BasicAction and ConcreteAction by using inheritance. This is not necessary. You can couple them by composition instead. If ConcreteAction implements a IBasicAction interface, a seperate class ActionRunner can call createManagers() and execute() on the action. You can pass a ConcreteAction instance to the ActionRunner.

Go abstract (option 2).

Common code, especially common processing flow, should be abstracted. This leaves the subclasses free to implement the differences, and means to can test the abstract code in isolation - eg with a mock/test implementation.

Note that you can take this too far, so common sense applies, but always keep your eye out for abstraction points.

I usually use the latter approach because it makes reuse more simple. Often, I need the same managers for many actions, so I can implement the create/dispose code once and then all that's left is a small execute() method.

And if that approach doesn't work, I can still override the original handler method, so I get the best of both worlds.

The abstract class would provide a guarantee of sorts that the cleanup code was always called, and it reduces duplication, so it's better than your existing structure in my opinion. Of course you can't know that a subclass' implementation of disposeManagers will kill exactly those managers that it created earlier - but you have this same problem with writing standard finally blocks.

I think I'd go one step further though. For one thing, the execute method needs those two managers to do the work, so I'd define it as

public void execute(HttpServletRequest req, TrafficManager t, CargoManager c);

Now let's assume that most of your actions use the same manager implementations. We define these method in the superclass (not making them final though) as:

public TrafficManager createTrafficManager() { return new TrafficManager(); }
public CargoManager createCargoManager() { return new CargoManager(); }

So now the superclass can create these instances itself by invoking the method, and pass them into execute. And if a subclass needs a different implementation from the default it can override the method as it desires.

Looking at the cleanup - we could take a similar approach to the above and define abstract implementations. However, if the managers need to be cleaned up, then presumably they implement a close() method or similar. In which case we can just call that - which guarantees that they'll be disposed of properly, regardless of subclasses' implementations, and it's not possible for the try and finally to get out of sync.

Or you can just call removeManager on each, if that's what your logic requires. (This could possibly be improved further but it depends on the semantics of that method, and where it "lives".)

Altogether then the BasicAction's main code looks like

BasicAction action = mapping.get(actionName);

// (It's a shame that these need the initial assignment to null due to being
//  referenced in the finally block - it's pretty ugly)
TrafficManager tMan = null;
CargoManager cMan = null;
try {
  tMan = createTrafficManager();
  cMan = createCargoManager();
  action.execute(request, tMan, cMan);//Handle the http request from the JSP page
} catch (Exception ex) {
  // Handle here any exceptions
} finally {
  if (tMan != null) {
    removeManager(tMan); // Is this necessary, did it get registered somewhere after creation?
    tMan.close(); // If they're closeable
  }
  if (cMan != null) { 
    // Of course this block could be a tiny method to further remove duplication,
    // so long as you have a common superinterface for both *Manager classes
    removeManager(cMan);
    cMan.close();
  }
}