Kindly review this class

Question

What I can think of improving is:

• Show stack on screen to fail gracefully i.e. improved catch part of try..catch
• Make the try part shorter to improve readability
• Make imports with * to improve readability so you don't have to import the same package twice

Source code:

//source is probably public domain since prv.se is a public authority:
package se.prv.pandora.arendeprocess.actionhandler;
import java.util.List;
import org.apache.log4j.Logger;
import se.prv.framework.forms.IFormData;
import se.prv.framework.forms.IFormPattern;
import se.prv.framework.general.Action;
import se.prv.framework.general.ISessionHandler;
import se.prv.pandora.arendeprocess.daos.AnsokanDAO;
import se.prv.pandora.arendeprocess.entity.Ansokan;
import se.prv.pandora.arendeprocess.exceptions.PandoraDAOException;
import se.prv.pandora.arendeprocess.forms.NamnsokningFormPattern;
import se.prv.pandora.arendeprocess.general.PandoraActionHandler;
import se.prv.pandora.arendeprocess.obj.AnsokanInfo;
import se.prv.pandora.arendeprocess.obj.ArendeProcessSessionData;
import se.prv.pandora.arendeprocess.obj.ArendeSearchAdmin;
import se.prv.pandora.arendeprocess.obj.ArendesokInfo;
import se.prv.pandora.arendeprocess.util.ArendeComparatorManager;
import se.prv.pandora.arendeprocess.util.ArendeProcessListComparator;
import se.prv.pandora.arendeprocess.util.ArendeSokListComparator;
import se.prv.pandora.arendeprocess.util.LookupHelper;
import se.prv.pandora.arendeprocess.util.MenuManager;
import se.prv.pandora.arendeprocess.util.PandoraAnsokanHandler;
import se.prv.pandora.arendeprocess.util.PandoraFieldConstants;
/**
  * @author Niklas Rosencrantz (adbnro)
  * 
  */
public class ArendesokningActionHandler extends PandoraActionHandler {
   private ArendeProcessSessionData sessionData;
   private AnsokanDAO ansokanDAO;
   private int initialSort = ArendeProcessListComparator.INKOM_FIRST;
   private final static Logger logger = Logger.getLogger(ArendesokningActionHandler.class);
   protected IFormData getFormData() {
      return null;
   }
   protected IFormPattern getPattern() {
      return NamnsokningFormPattern.getInstance();
   }
   public ArendesokningActionHandler(){
      ansokanDAO = LookupHelper.lookup(se.prv.pandora.arendeprocess.daos.AnsokanDAO.class);
   }
   protected void performAction(ISessionHandler sessionHandler, Action action) {
      String returnPage = null;
      List<AnsokanInfo> ansokanInfoList = null;
      List<AnsokanInfo> sortedList = null;
      ArendesokInfo arendesokInfo = null;
      ArendeComparatorManager compManager = null;
      ArendeSearchAdmin admin = new ArendeSearchAdmin();
      PandoraAnsokanHandler ansokanHandler = new PandoraAnsokanHandler();
      try {
         sessionData = (ArendeProcessSessionData) sessionHandler.getSessionData();
         MenuManager menuManager = sessionData.getMenuManager();
         returnPage = menuManager.getLatestDestination(action.getActionTarget());   
         arendesokInfo = sessionData.getArendesokInfo();    
         if(arendesokInfo != null) {
            if(arendesokInfo.getArendeComparatorManager() != null) {
               compManager = arendesokInfo.getArendeComparatorManager();
            } else {
               compManager = new ArendeComparatorManager();
            }   
         } else {
            arendesokInfo = new ArendesokInfo();
            compManager = new ArendeComparatorManager();
         }  
         IFormData formData = sessionData.getFormData();
         //ansokanInfo.setEditPersonInfo(null); 
         if(action.getActionCommand().equalsIgnoreCase("choose")){  
            // TODO: Ska valideras
            //saveGrunduppgifter(formData, ansokanInfo, ansokan);   
            returnPage = sessionData.getLatestAction().getCurrPage();
            //"arendeprocess_grunduppgifter.jsp"
         } else if(action.getActionCommand().equalsIgnoreCase("search")){
            logger.info("i search");
            String search_arende = formData.getValue(PandoraFieldConstants.FIELD_SEARCH_ARENDE);
            List<Ansokan> ansokningar = ansokanDAO.search(search_arende);
            sessionData.setArendesokningLista(ansokanHandler.getAnsokanInfoFromAnsokan(ansokningar));
            returnPage = sessionData.getLatestAction().getCurrPage();
         }
         compManager.setLastSortNr(initialSort);
         ArendeSokListComparator comp = new ArendeSokListComparator(initialSort);   
         sortedList = sortFormList(sessionData.getArendesokningLista(), comp);
         admin.setResultList(sortedList);
         admin.setNbOfHits(sortedList.size());  
         arendesokInfo.setArendeSearchAdmin(admin); 
         arendesokInfo.setAnsokanInfoList(ansokanInfoList);
         arendesokInfo.setArendeComparatorManager(compManager); 
         sessionData.setArendesokInfo(arendesokInfo);
         action.setReturnPage(returnPage);
      } catch (Exception e) {
         logger.error("ArendesokningActionHandler: performAction() ", e);
         throw new PandoraDAOException(e, "performAction()", "Error. Transaction must be rolled back: " + e.getMessage());
      }
   }
   List<AnsokanInfo> sortFormList(List<AnsokanInfo> resultList1, ArendeSokListComparator comp) {
      java.util.Collections.sort(resultList1, comp);

      return resultList1;
   }
}

Answer 1

Make imports with * to improve readability so you don't have to import the same package twice

I disagree with that:

• Most people use IDEs to develop and view Java code, and any half-decent IDE is able to "fold" imports to save screen real-estate.

• "*" imports have the problem that otherwise harmless changes external to your class can lead to compilation errors. Consider this:

  import java.util.*;
  import com.foobar.foolib.*;
  
  public MyClass {
      List list = ...;
  }
  

  Today, that may compile fine. But if the "foobar" folks decide to add a class called "com.foobar.foolib.List" in the next version of the library, then when change my dependencies, my code will now give compilation errors because List is now being imported from two different places. (OK, the "foobar" guys have made a particularly bad choice of classname here ... but it is also my fault for using "*" imports.)

Show stack on screen to fail gracefully i.e. improved catch part of try..catch

Do you really want to show stacktraces to end users? I actually think you have probably got the logging right.

---

Here are some comments of my own:

• It is not a good idea to always initialize local variables. If you think it is a good idea to declare them all at the start of the method, then it is better to leave off the initializers. That way, the compiler can tell you if you use a variable before it is initialized. By contrast, if you dummy initialize with some nonsense value (e.g. null), the compiler can't tell you that you have missed out the real initialization.

• I have some concerns with this:

  } catch (Exception e) {
     logger.error("ArendesokningActionHandler: performAction() ", e);
     throw new PandoraDAOException(e, "performAction()", 
          "Error. Transaction must be rolled back: " + e.getMessage());
  }
  

  Three concerns:

  1. You are catching Exception, and that is usually a bad idea. (Whether it is in this case is hard to determine ...)
  2. There is something fishy about throwing PandoraDAOException from a class that doesn't appear to be a DAO class.

  3. There is something fishy about a non-DAO class saying "Transaction must be rolled back". Surely, this class should not know about transactions.

     It strikes me that that all three of this fall under the general concern of separation of responsibility. This class seems to be doing things / knowing things that shouldn't really be its responsibility.