2
\$\begingroup\$

This is working code presently visible here. Basically, this is still a work in process, but I do not want to go much further down the road without peer review. I've limited the source to the servlet. I am managing multiple servlet requests with parameters. I'm looking for feedback on that pattern--and anything else that stands out.

public class MyServlet extends HttpServlet {

// use this for usda reservoir station values later
public final SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd");

// manage form requests and add web service reference object
AwdbWebService m_webService = null;
String countyName = null;
String action = null;

public final Logger logger = Logger.getLogger(getClass().getName());
@Resource(name = "jdbc/mydb", lookup = "jdbc/mydb")
private DataSource dataSource;



@Override
public void init(ServletConfig config) throws ServletException {
    super.init(config);
    logger.info("Init");
    System.out.println(getClass().getName() + ".init");
}


public void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException 
{
    action  = req.getParameter("ACTION");
    if("READ-FindStation" .equals(action)){
        countyName = req.getParameter("countyName");
        req.setAttribute("theCounty",countyName);
        RequestDispatcher dispatcher = req.getRequestDispatcher("/awdbRetrieveStationInventory-simple.jsp");
        dispatcher.forward(req,resp);

    }
    else
    {

    PrintWriter writer = resp.getWriter();

    writer.println("<html>");
    writer.println("<head><title>MyServlet</title></head>");
    writer.println("<body><h1>MyServlet</h1>");

    writer.println("<h2>DataSource</h2>");
    Connection conn = null;


    try {
        writer.println("Datasource: " + dataSource + "<br/><br/>");
        conn = dataSource.getConnection();
        Statement stmt = conn.createStatement();
        ResultSet rst = stmt.executeQuery("select 1");
        while (rst.next()) {
            writer.println("Resultset result: " + rst.getString(1) + "<br/><br/>");
        }
        rst.close();
        stmt.close();
        conn.close();
        writer.println("SUCCESS to access the datasource");


    } catch (Exception e) {
        e.printStackTrace(writer);
        e.printStackTrace();
    } finally {
        if (conn != null) {
            try {
                conn.close();
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    }
    writer.println("</body></html>");

    }  //end-else


}


public void initializeUSDAWebService()
{
     try
     {
       URL wsURL = new URL("http://www.wcc.nrcs.usda.gov/awdbWebService/services?wsdl");
       AwdbWebService_Service lookup = new AwdbWebService_Service(wsURL,new QName("http://www.wcc.nrcs.usda.gov/ns/awdbWebService","AwdbWebService"));
       m_webService = lookup.getAwdbWebServiceImplPort();

     }
     catch (Exception e)
     {   
         System.out.println("Problem creating usda web service client: "
             + e.getMessage());
             e.printStackTrace();

     }   
}

/**
 * Use Case 1: Get Inventory of Stations
 * This example will get an inventory of all stations in Oregon
 * for SNOTEL stations that have Snow Water Equivalent
 * and return  list of stations   
 */

 public List<StationMetaData> getStations(String countyName)
 {
       initializeUSDAWebService();

       // TODO: create another jsp page to fill in networkCds and
       //       element codes. Leave as static retrieval for first
       //       pass of development.

        List<String> stationIds = null;
        List<String> stateCds = null;
        List<String> networkCds = Arrays.asList("SNTL");
        List<String> hucs = null;
        List<String> countyNames = Arrays.asList(countyName);
        BigDecimal minLatitude = null;
        BigDecimal maxLatitude = null;
        BigDecimal minLongitude = null;
        BigDecimal maxLongitude = null;
        BigDecimal minElevation = null;
        BigDecimal maxElevation = null;
        List<String> elementCodes = Arrays.asList("WTEQ");
        List<Integer> ordinals = Arrays.asList(1);
        List<HeightDepth> heightDepths = null;

        /*
         * If (logicalAnd) is true, the getStations() call will return only
         * stations that match ALL of the parameters passed in, otherwise it’ll
         * return stations that match ANY of the parameters passed in.
        */   

        boolean logicalAnd = true;
        List<String> stationTriplets = m_webService.getStations(stationIds,
            stateCds, networkCds, hucs, countyNames, minLatitude,
            maxLatitude, minLongitude, maxLongitude, minElevation,
            maxElevation, elementCodes, ordinals, heightDepths, logicalAnd);

       // plditallo - avoid null object returns to the calling jsp
       // List<StationMetaData> stations = null;

        List<StationMetaData> stations = Collections.emptyList();

        try
        {
         stations = m_webService.getStationMetadataMultiple(stationTriplets);
        }
        catch (Exception e)
           {   
               System.out.println("Problem retrieving usda stations from usda method: "
                   + e.getMessage());
               e.printStackTrace();

           } 

        return stations;
    }

 /**
     * Use Case 2: Get period of record for one station.
     * This will return period of Data that are SNOW WATER EQUIVALENT (element
     * code = WTEQ) for a given station and date range.
     * Note: Always use an ordinal of 1, and heightDepth of null
     * (height depth is only used for soil sensors)
     * @param p_stationTriplet The station to get data for, ex: "471:ID:SNTL"
     * @param p_beginDate The begin date - a String format "yyyy-MM-dd"
     * @param p_endDate The end date - a String format "yyyy-MM-dd"
     * @return An Array of Data Objects
     */

    public Data[] getPeriodOfRecord(String p_stationTriplet, String p_beginDate,     String p_endDate){               
        Data[] values = m_webService.getData(Arrays.asList(p_stationTriplet),
            "WTEQ", 1, null, Duration.DAILY, true, p_beginDate, p_endDate, true)
            .toArray(new Data[0]);
        return values;
    }

    /**
     * Use Case 3: Get past seven days' data.
     * This will return the last seven days of SNOW WATER EQUIVALENT (element
     * code = WTEQ)Data, relative to today, for a given station.
     * Note: Always use an ordinal of 1, and heightDepth of null
     * (height depth is only used for soil sensors)    
     * @param p_stationTriplet The station to get data for, ex: "471:ID:SNTL"
     * @return An Array of Data Objects
     */
    public Data[] getLastSevenDaysData(String p_stationTriplet){

        String today = dateFormat.format(new Date());
        Calendar lastWeek = GregorianCalendar.getInstance();
        lastWeek.add(Calendar.DAY_OF_YEAR, -7);
        String sevenDaysAgo = dateFormat.format(lastWeek.getTime());

        Data[] values = m_webService.getData(Arrays.asList(p_stationTriplet),
            "WTEQ", 1, null, Duration.DAILY, true, sevenDaysAgo, today, true)
            .toArray(new Data[0]);

        return values;
     }
 }
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$
public final SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd");

This stupid class is not thread-safe. So better forget it or use a ThreadLocal wrapper.

e.printStackTrace(writer);

Are you sure the user should see it? If it's just temporary, write a method like handle(Exception e) doing this (an add an TODO there).

    e.printStackTrace();

What is then your

public final Logger logger = Logger.getLogger(getClass().getName());

good for? Additionally, reduce visibility as much as possible.

List<String> stationIds = null;
List<String> stateCds = null;
...

Avoid nulls, especially for collections. An empty collection is much easier to work with.

You seem to be avoiding a class like

class Station {
    String stationId;
    String stateCd;
    ...
}

Why?

If (logicalAnd) is true, the getStations() call will return only stations that match ALL of the parameters passed in, otherwise it’ll return stations that match ANY of the parameters passed in.

Boolean arguments are better replaced by

enum MatchType {ANY, ALL}

Isn't this a bit too long a parameter list?

List<String> stationTriplets = m_webService.getStations(stationIds,
        stateCds, networkCds, hucs, countyNames, minLatitude,
        maxLatitude, minLongitude, maxLongitude, minElevation,
        maxElevation, elementCodes, ordinals, heightDepths, logicalAnd);

If you don't want to pass List<Station>, then consider grouping related parameters together.

String p_stationTriplet

Hungarian notation doesn't get used in Java.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ Kill it with fire and bury it out back. I hate Hungarian notation. \$\endgroup\$
    – RubberDuck
    Sep 26, 2014 at 0:34
  • \$\begingroup\$ Now,now, @RubberDuck, hungarian notation was once ALL the rage, back when! :) You wait and see, one day it come back from the smoke and ash of discontent! :) \$\endgroup\$
    – plditallo
    Sep 27, 2014 at 8:46
  • \$\begingroup\$ @plditallo Obligatory link to Joel Spolsky's "Making Code Look Wrong". Maybe someday people will learn to use it correctly... Maybe. =;)- \$\endgroup\$
    – RubberDuck
    Sep 27, 2014 at 10:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.