• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

requesting code review

 
Bartender
Posts: 1810
28
jQuery Netbeans IDE Eclipse IDE Firefox Browser MySQL Database Chrome Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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.

 
Java Cowboy
Posts: 16084
88
Android Scala IntelliJ IDE Spring Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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:

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:

Spring Data JPA interprets the method name and from that it automatically understands what query needs to be done.
 
Bartender
Posts: 1166
17
Netbeans IDE Java Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
The code does exhibit one of my pet hates -


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.
 
Bartender
Posts: 10780
71
Hibernate Eclipse IDE Ubuntu
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
 
Bartender
Posts: 1210
25
Android Python PHP C++ Java Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Bartender
Posts: 1810
28
jQuery Netbeans IDE Eclipse IDE Firefox Browser MySQL Database Chrome Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
Bartender
Posts: 1810
28
jQuery Netbeans IDE Eclipse IDE Firefox Browser MySQL Database Chrome Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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


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
Bartender
Posts: 1810
28
jQuery Netbeans IDE Eclipse IDE Firefox Browser MySQL Database Chrome Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
Bartender
Posts: 1810
28
jQuery Netbeans IDE Eclipse IDE Firefox Browser MySQL Database Chrome Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Karthik Shiraly wrote:
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)

Here's the context listener that goes with it:
 
Jesper de Jong
Java Cowboy
Posts: 16084
88
Android Scala IntelliJ IDE Spring Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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.
 
Ranch Hand
Posts: 10198
3
Mac PPC Eclipse IDE Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Bartender
Posts: 1810
28
jQuery Netbeans IDE Eclipse IDE Firefox Browser MySQL Database Chrome Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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?
 
Rancher
Posts: 4801
50
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
Bartender
Posts: 1810
28
jQuery Netbeans IDE Eclipse IDE Firefox Browser MySQL Database Chrome Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Rancher
Posts: 4801
50
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Ranch Hand
Posts: 10198
3
Mac PPC Eclipse IDE Ubuntu
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Bartender
Posts: 1810
28
jQuery Netbeans IDE Eclipse IDE Firefox Browser MySQL Database Chrome Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Bartender
Posts: 1810
28
jQuery Netbeans IDE Eclipse IDE Firefox Browser MySQL Database Chrome Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Ranch Hand
Posts: 10198
3
Mac PPC Eclipse IDE Ubuntu
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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.
 
Consider Paul's rocket mass heater.
reply
    Bookmark Topic Watch Topic
  • New Topic