requesting code review

[J. Kevin Robbins]

I've written many DAOs using this pattern, and they work, but I work alone. There is no senior developer here and no code reviews, so if there is a better way of doing this, I'll never learn what it is. Please feel free to nitpick as my goal is to continue improving as a developer. Some names have been changed to protect the innocent.

package com.mypackage.daos;

import com.mypackage.jkr.obj.DataSourceHolder;
import java.sql.CallableStatement;
import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.ResultSetMetaData;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
 * DAO for <deleted> using stored procedure sp00041. 
 * Requires 2 parameters as Strings: start date and end date in yyyyMMdd format
 *
 * @author Kevin Robbins
 */
public class DemoDAO {

    private final static Logger LOGGER = Logger.getLogger(DemoDAO.class.getName());
    private final static String SQL = "call mylibrary/sp00041(?,?)";

    public List<List<String>> getBuildCureStock(String startDate, String endDate) {
        final List<List<String>> records = new ArrayList<List<String>>();
        final DataSourceHolder dsh = DataSourceHolder.getInstance();
        final Connection conn = dsh.getConnection();
        ResultSet rset = null;
        CallableStatement cstmt = null;
        try {
            cstmt = conn.prepareCall(SQL);
            cstmt.setString(1, startDate);
            cstmt.setString(2, endDate);
            rset = cstmt.executeQuery();
            final ResultSetMetaData rsetMeta = rset.getMetaData();
            final int colCount = rsetMeta.getColumnCount();
            while (rset.next()) {
                final List<String> cols = new ArrayList<String>();
                for (int i = 1; i <= colCount; i++) {
                    cols.add(rset.getString(i));
                }
                records.add(cols);
            }
        }
        catch (SQLException sqle) {
            LOGGER.log(Level.SEVERE, "SQL Exception in DemoDAO", sqle);
        }
        //<editor-fold defaultstate="collapsed" desc="finally block">
        finally {
            if (rset != null) {
                try {
                    rset.close();
                }
                catch (SQLException sqle) {
                    LOGGER.log(Level.SEVERE, null, sqle);
                }
            }
            if (cstmt != null) {
                try {
                    cstmt.close();
                }
                catch (SQLException sqle) {
                    LOGGER.log(Level.SEVERE, null, sqle);
                }
            }
            if (conn != null) {
                try {
                    conn.close();
                }
                catch (SQLException sqle) {
                    LOGGER.log(Level.SEVERE, null, sqle);
                }
            }

        }
        //</editor-fold>
        return records;
    }
}

[Jesper de Jong]

Hi Kevin, your code looks OK to me, I don't see any big issues, but I do have some comments.

You're using plain old JDBC to call a stored procedure and then you read the resulting records that the stored procedure returns, and then you return them as a list of lists of strings. Looking a bit further, each record exists of two strings which both represent dates (in the format yyyyMMdd).

First of all, returning a List<List<String>> isn't very informative. Just by looking at the method signature you have no idea what this method returns (it's a list of lists of strings, but what does the content of the return value mean?). I would make a class to hold a record and then return a list of instances of that class instead. Since the two values are dates, I would use Date objects instead of strings. If you're using Java 8, then definitely use the new date and time API, and otherwise consider using Joda Time. For example:

import java.time.LocalDate;
import java.time.format.DateTimeFormatter;

// This should have a meaningful name, "Record" is too general
public class Record {

    private final LocalDate startDate;
    private final LocalDate endDate;

    public Record(LocalDate startDate, LocalDate endDate) {
        this.startDate = startDate;
        this.endDate = endDate;
    }

    public LocalDate getStartDate() {
        return startDate;
    }

    public LocalDate getEndDate() {
        return endDate;
    }
}

public class DemoDAO {  
    public List<Record> getBuildCureStock(LocalDate startDate, LocalDate endDate) {
        // ...
        cstmt.setString(1, DateTimeFormatter.BASIC_ISO_DATE.format(startDate));
        cstmt.setString(2, DateTimeFormatter.BASIC_ISO_DATE.format(endDate));
        // ...
    }
}

Are you absolutely tied to using stored procedures? If not, then I would consider using JPA / Hibernate or Spring Data JPA.

I especially like Spring Data JPA; the only thing you need to do is declare an interface, and Spring Data JPA will automatically generate the implementation to do the query for you. You would only have to write something like this:

@Entity // Record is a JPA entity
public class Record { ... }

@Repository
public interface DemoRepository {

    List<Record> findByStartDateGreaterThanAndEndDateLessThan(LocalDate startDate, LocalDate endDate);
}

Spring Data JPA interprets the method name and from that it automatically understands what query needs to be done.

[Richard Tookey]

The code does exhibit one of my pet hates -

      catch (SQLException sqle) {  
            LOGGER.log(Level.SEVERE, "SQL Exception in DemoDAO", sqle);  
        }  
  

You catch an exception that you class as severe, you log it and then continue as if nothing had happened. To my mind, if you have a severe exception then you cannot continue. I would expect to see the exception logged and then re-thrown or converted to another exception and then thrown.

[Winston Gutkowski]

J. Kevin Robbins wrote:Please feel free to nitpick as my goal is to continue improving as a developer.

OK.

The first thing is that, if you're on version 7 or later, you may be able to save yourself a bit of code with a "try-with-resources" block.

Second: What you seem to have implemented is, for want of a better term, a "stringizer"; and I'm not sure precisely what that gives you (have a look at the StringsAreBad page) - ie, you've simply converted a native form (a ResultSet) into a matrix of Strings, which you'll presumably have to convert back to whatever type each 'field' actually represents later on.

Third: It seems to rely on being able to read ALL the rows in one go, which might lead to a VERY big program if your query returns millions of rows.

Fourth: The inner List relies on the order (and number) of fields being the same every time. Given the nature of rows in a database, this may not be a problem, but a List<HashMap<String, String>> - where the HashMap contains "column name' + value" - might be a bit more generic.

However, finally: It all seems a bit "procedural" to me. My understanding of a DAO is that it's an "interim" between a data-access layer and an actual Java object (eg, a Customer), and is actually provided to you by the DA layer. And I'd say that ResultSet already fits that bill pretty well.
All your code seems to have done is add another layer (and actually a more abstract one) between that and whatever 'Pojo' you end up with.

I suppose it might be easier to use if you're just slapping data out to a screen, but I can't really see what else it buys you. But maybe I'm just being thick.

And you did ask.

Winston

[Karthik Shiraly]

final Connection conn = dsh.getConnection();  
        ResultSet rset = null;  
        CallableStatement cstmt = null;  
        try {  
            cstmt = conn.prepareCall(SQL);  

Normally, I'd put the DataSource.getConnection() call too inside try block.

Not sure what happens in this case, where it's getting connection from something called com.mypackage.jkr.obj.DataSourceHolder.
If that class too is swallowing the exception, then this code is going ahead as if it's no big deal, which I think is wrong.

On the other hand, if that class is throwing some exception - presumably an unchecked one - then this class is
1) inconsistent in exception handing, because it propagates some exceptions while swallowing others 2) possibly throwing exceptions but has not documented them

[J. Kevin Robbins]

First, thanks for the feedback. Now let me explain a little more.

Jesper de Jong wrote:First of all, returning a List<List<String>> isn't very informative. Just by looking at the method signature you have no idea what this method returns (it's a list of lists of strings, but what does the content of the return value mean?). I would make a class to hold a record and then return a list of instances of that class instead.

In some of my apps I use beans to hold a record and return a list of beans. I've also used a map of beans in some cases. I've often struggled with the best type of objects to return. I've never found any good guidelines on this, so if you looked at my code base you would find that I'm all over the map as I've tried different things. In general it seems that lists or beans are easier to iterate over using JSTL, maps get a bit trickier. As I work on this current project, I'm thinking of changing that DAO to return a String as a JSON response because I'm going to use DataTables to display the result and it plays very well with JSON. As I get more comfortable with Ajax I'm using JSON responses more and more.

Jesper de Jong wrote:Since the two values are dates, I would use Date objects instead of strings. If you're using Java 8, then definitely use the new date and time API, and otherwise consider using Joda Time.

Here's the story on that. I use the jQuery UI DatePicker in the GUI so it gives me a String from the alt field, and all our dates in the AS400 are stored as Strings, never as a Date data type, so there doesn't seem to be any advantage to converting the Strings to dates unless I need to manipulate them such as subtracting dates or calculating business days in a date range. In those cases I've got a little utility class that I wrote to handle that. And btw, I'm stuck on Java 5 until we upgrade the web server. Yeah, I know.

Jesper de Jong wrote:Are you absolutely tied to using stored procedures? If not, then I would consider using JPA / Hibernate or Spring Data JPA.

Not absolutely, but stored procedures is what our AS400 programmer knows, and I know nothing about JPA/Hibernate or Spring Data, so we use what we're comfortable with. I would agree that it's time to stretch that comfort zone. After I get the web server upgraded I would like to start using JPA and/or Hibernate. Spring Data JPA looks to be pretty slick.

[J. Kevin Robbins]

Richard Tookey wrote:The code does exhibit one of my pet hates -

      catch (SQLException sqle) {  
            LOGGER.log(Level.SEVERE, "SQL Exception in DemoDAO", sqle);  
        }  
  

You catch an exception that you class as severe, you log it and then continue as if nothing had happened. To my mind, if you have a severe exception then you cannot continue. I would expect to see the exception logged and then re-thrown or converted to another exception and then thrown.

This is something that I definitely need to learn more about. I usually don't do anything with exceptions except log them. How do you recover from a SQLException? What's the advantage of re-throwing it just to be handled somewhere else? What's the advantage to converting it to a different type of exception?

If you can recommend reading material on this topic it would be greatly appreciated.

[J. Kevin Robbins]

Yes I did ask. I have thick skin, thanks for the feedback. Since I work alone I consider the folks here to be my co-workers. This is the best way for me to learn.

Winston Gutkowski wrote:The first thing is that, if you're on version 7 or later, you may be able to save yourself a bit of code with a "try-with-resources" block.

I wish. I actually misspoke above, we're all the way up to 6. Woo-hoo. Running on RedHat 4. I've been begging for new web servers for two years.

Winston Gutkowski wrote:Second: What you seem to have implemented is, for want of a better term, a "stringizer"; and I'm not sure precisely what that gives you (have a look at the StringsAreBad page) - ie, you've simply converted a native form (a ResultSet) into a matrix of Strings, which you'll presumably have to convert back to whatever type each 'field' actually represents later on.

Good point and interesting link. I need to re-read that a couple of times to digest it. I went this direction because these records are just going to be displayed as a spreadsheet-style report. But now I'm thinking of returning a JSON string and passing it to DataTables.

Winston Gutkowski wrote:Third: It seems to rely on being able to read ALL the rows in one go, which might lead to a VERY big program if your query returns millions of rows.

Another good point but not a problem with this report. This is only one record per day and I'll use DatePicker to limit the user to no more that one year of data, so at most I'll have 365 records. However, back to the point of using objects (beans) instead of strings. What if it were thousands or millions or records? Then I'd end up creating millions of objects within the while loop. That seems worse that Strings. How does one deal with that?

Winston Gutkowski wrote:Fourth: The inner List relies on the order (and number) of fields being the same every time. Given the nature of rows in a database, this may not be a problem, but a List<HashMap<String, String>> - where the HashMap contains "column name' + value" - might be a bit more generic.

Another good point. I have used HashMaps in some cases but I get easily confused when trying to use them with JSTL, so I'm sort of intimidated by that. Another weakness that I need to overcome.

Winston Gutkowski wrote:However, finally: It all seems a bit "procedural" to me. My understanding of a DAO is that it's an "interim" between a data-access layer and an actual Java object (eg, a Customer), and is actually provided to you by the DA layer. And I'd say that ResultSet already fits that bill pretty well.
All your code seems to have done is add another layer (and actually a more abstract one) between that and whatever 'Pojo' you end up with.

I suppose it might be easier to use if you're just slapping data out to a screen, but I can't really see what else it buys you. But maybe I'm just being thick.

That's essentially what I'm doing. The user selects start and end dates and I display the results in a table. Very basic. The jsp submits the dates to a servlet which instantiates the DAO which returns the results to the servlet which sends the results back to the page. Maybe my terminology isn't correct (DAO versus DA) but I think of this DAO as the data access layer that separates the servlet from the data.

[J. Kevin Robbins]

Karthik Shiraly wrote:

final Connection conn = dsh.getConnection();  
        ResultSet rset = null;  
        CallableStatement cstmt = null;  
        try {  
            cstmt = conn.prepareCall(SQL);  

Normally, I'd put the DataSource.getConnection() call too inside try block.

Not sure what happens in this case, where it's getting connection from something called com.mypackage.jkr.obj.DataSourceHolder.
If that class too is swallowing the exception, then this code is going ahead as if it's no big deal, which I think is wrong.

On the other hand, if that class is throwing some exception - presumably an unchecked one - then this class is
1) inconsistent in exception handing, because it propagates some exceptions while swallowing others 2) possibly throwing exceptions but has not documented them

This gets back to my inexperience in dealing with exceptions. It's becoming obvious to me that this is an area ripe for improvement. FYI, this is the DataSourceHolder which I've gone over here before. It's not pretty and it's been pointed out to me that it's not a real Singleton, but I haven't gotten around to refactoring it because of the amount of testing that would be involved. I wrote this because when I took over this job, every servlet was doing initial context lookup in the init method and that seemed really inefficient to me, so I came up with this. It seemed like a good idea at the time. (I wrote this several years ago when I was still very green; please be gentle)

public final class DataSourceHolder {

    private DataSource dataSource;
    private static DataSourceHolder dataSourceHolder;

    private DataSourceHolder() {}

    //this method should be called from the context listener and
    //stored in the dataSourceHolder static variable
    public synchronized static DataSourceHolder getInstance() {
        if (dataSourceHolder == null) {
            dataSourceHolder = new DataSourceHolder();
        }
        return dataSourceHolder;
    }

    //after calling the above from the context listener
    //call this method from the listener to set the datasource
    public void setDataSource(DataSource dataSource) {
        this.dataSource = dataSource;
    }

    //use this from servlets or DAOs to get a connection object
    public Connection getConnection() {
        Connection conn = null;
        try {
            conn = dataSource.getConnection();
        }
        catch (SQLException sqle) {
            sqle.printStackTrace();
        }
        return conn;
    }
}

Here's the context listener that goes with it:

/**
 *
 * @author ZA15426
 * Looks up the datasource for the as400 and sets it in the context and
 * creates an instance of DataSourceHolder.
 * There are two ways to use this information.
 * The first, have all servlets pass a ServletConfig object to their DAO's
 * and the DAO's can use that to look up the datasource and get a connection.
 * This method is provided for backward compatibility with existing apps.
 * Example:
 * DataSource ds = (DataSource) this.getServletConfig().getServletContext().getAttribute("as400DS");
 *
 * The second and preferred method is to have a servlet, DAO, or business object
 * get the instance of DataSourceHolder, get a connection, and instantiate the
 * DAO by passing the connection object.
 * Example:
 * DataSourceHolder dsh = DataSourceHolder.getInstance();
 * Connection conn = dsh.getConnection();
 * MyDao dao = new MyDao(conn);
 *
 */
public class GyContextListener implements ServletContextListener {

    private final static Logger LOGGER = Logger.getLogger(GyContextListener.class.getName());

    @Override
    public void contextInitialized(ServletContextEvent event) {
        LOGGER.info("GyContextListener:Context initialized");
        try {
            //look up jndi data source
            ServletContext sc = event.getServletContext();
            Context ic = (Context) new InitialContext();
//            if(ic == null) {
//                LOGGER.info("GyContextListener:Failed to get initial context");
//                return;
//            }

            Context ctx = (Context) ic.lookup("java:/comp/env");
            if(ctx == null) {
                LOGGER.info("GyContextListener:Failed to get environment context");
                return;
            }
            //for testing
            //DataSource ds = (DataSource) ctx.lookup("localas400tst");
            //for production
            DataSource ds = (DataSource) ctx.lookup("localas400");
            if(ds == null) {
                LOGGER.info("GyContextListener:Failed to get datasource");
                return;
            }
            //set the datasource in the context
            sc.setAttribute("as400DS", ds);
            //initialize the singleton DataSourceHolder object
            DataSourceHolder dataSourceHolder = DataSourceHolder.getInstance();
            dataSourceHolder.setDataSource(ds);
        } catch (NamingException ne) {
            LOGGER.log(Level.SEVERE, null, ne);
        }
        LOGGER.info("GyContextListener:datasource acquired and set in context as as400DS");
        LOGGER.info("GyContextListener:DataSourceHolder object initialized");
    }

    @Override
    public void contextDestroyed(ServletContextEvent sce) {
        //nothing to do here
    }
}

[Jesper de Jong]

J. Kevin Robbins wrote:This is something that I definitely need to learn more about. I usually don't do anything with exceptions except log them. How do you recover from a SQLException? What's the advantage of re-throwing it just to be handled somewhere else? What's the advantage to converting it to a different type of exception?

What you have now is definitely not very good, because if an exception happens then it's logged but the method just returns an empty list. A user of the application just gets empty search results on the screen - (s)he has no idea if this is because there was no data for the specified search criteria, or if an error happened.

Your DAO should just let the exception propagate up and it should then be handled at the appropriate level, so that an error message can be displayed to the user.

[Joe San]

J. Kevin Robbins wrote:
This is something that I definitely need to learn more about. I usually don't do anything with exceptions except log them. How do you recover from a SQLException? What's the advantage of re-throwing it just to be handled somewhere else? What's the advantage to converting it to a different type of exception?

It definitely is not a good idea to handle SQLException in a layer that sits above the DAO layer in the call chain. I would rather throw a domain specific exception (for example., DataAccessException) to the layer above! The advantage is that your business layer or the client of your DAO is independent of any java sql packages.

[J. Kevin Robbins]

Joe Harry wrote:

J. Kevin Robbins wrote:
This is something that I definitely need to learn more about. I usually don't do anything with exceptions except log them. How do you recover from a SQLException? What's the advantage of re-throwing it just to be handled somewhere else? What's the advantage to converting it to a different type of exception?

It definitely is not a good idea to handle SQLException in a layer that sits above the DAO layer in the call chain. I would rather throw a domain specific exception (for example., DataAccessException) to the layer above! The advantage is that your business layer or the client of your DAO is independent of any java sql packages.

Can you please give me an example of this? Are you talking about throwing it back to the servlet so the servlet can send an error message to the jsp?

[Dave Tolls]

Just an observation, but your DataSourceHolder opens a new Connection each time, which is fine.
However it is never closed, at least as far as I can see.
This can be fixed easily enough by handling it as you do that Statement and ResultSet objects.

Also, you are doing the same thing in getConnection as in the DAO, absorbing an exception that probably should be thrown (or wrapped and thrown as mentioned above).
In this case getConnection should probably throw it rather than attempt to handle it. It's what I would expect from something that produces a Connection.

[J. Kevin Robbins]

Dave Tolls wrote:Just an observation, but your DataSourceHolder opens a new Connection each time, which is fine.
However it is never closed, at least as far as I can see.

Look back at the original code post where I get the connection from the DataSourceHolder and then close it in the finally block. Does that not work the way I think it does? If I'm not really closing the connections in the finally block of all my DAOs then I'm surprised that I've never exhausted the connection pool. I'm going to debug my way through one now and see if that portion of the finally block executes.

Dave Tolls wrote:Also, you are doing the same thing in getConnection as in the DAO, absorbing an exception that probably should be thrown (or wrapped and thrown as mentioned above).
In this case getConnection should probably throw it rather than attempt to handle it. It's what I would expect from something that produces a Connection.

Yeah, I've got to find a source for learning more about this. I don't understand the advantage of re-throwing exceptions so I'm missing some basic concept here. I just checked Effective Java where it talks a bit about exceptions in chapter 9, but it's not very helpful. I need to find a good learning source for this concept.

[Dave Tolls]

J. Kevin Robbins wrote:

Dave Tolls wrote:Just an observation, but your DataSourceHolder opens a new Connection each time, which is fine.
However it is never closed, at least as far as I can see.

Look back at the original code post where I get the connection from the DataSourceHolder and then close it in the finally block. Does that not work the way I think it does? If I'm not really closing the connections in the finally block of all my DAOs then I'm surprised that I've never exhausted the connection pool. I'm going to debug my way through one now and see if that portion of the finally block executes.

Ah, phooey.

I'll go get my eyes tested!

[Joe San]

J. Kevin Robbins wrote:
Can you please give me an example of this? Are you talking about throwing it back to the servlet so the servlet can send an error message to the jsp?

Yes, I assume that there is a POJO layer in-between your Servlet and your DAO. This POJO layer is where your business logic should be encapsulated. You catch the SQLException in your DAO layer, wrap that into your application specific exception and hand it over to the business layer. Now depending upon the business functionality, you can decide what to do with the exception in your POJO business layer.

[J. Kevin Robbins]

Dave Tolls wrote:
Ah, phooey.

I'll go get my eyes tested!

Oh good. I was about to feel really foolish if I couldn't even close a connection properly. I'm glad I got that part right. Now the exceptions on the other hand...

[J. Kevin Robbins]

Joe Harry wrote:Yes, I assume that there is a POJO layer in-between your Servlet and your DAO. This POJO layer is where your business logic should be encapsulated. You catch the SQLException in your DAO layer, wrap that into your application specific exception and hand it over to the business layer. Now depending upon the business functionality, you can decide what to do with the exception in your POJO business layer.

Not always. Many times the page is just a report that displays data in a grid-style format. Some of the apps have an object that I call a ChartBuilder, other times it's just servlet to DAO and back to servlet for display. It almost sounds like I would benefit from a POJO just for exception handling for those cases, or is that overkill?

[Joe San]

J. Kevin Robbins wrote:
Not always. Many times the page is just a report that displays data in a grid-style format. Some of the apps have an object that I call a ChartBuilder, other times it's just servlet to DAO and back to servlet for display. It almost sounds like I would benefit from a POJO just for exception handling for those cases, or is that overkill?

It depends on the size of the application. If there are multiple business process implemented, it would be ideal to have a layer between your Servlets and DAO.