File APIs for Java Developers
Manipulate DOC, XLS, PPT, PDF and many others from your application.
http://aspose.com/file-tools
The moose likes OO, Patterns, UML and Refactoring and the fly likes Refactoring a Method Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Engineering » OO, Patterns, UML and Refactoring
Bookmark "Refactoring a Method" Watch "Refactoring a Method" New topic
Author

Refactoring a Method

Sandeep Suram
Greenhorn

Joined: Feb 22, 2011
Posts: 7
public Long getPIdDob(String lastName, Date dob) {

try {
Long Id = jdbcTemplate.queryForLong(getPersIdDob,
new Object[] {lastName, dob, "FA"});
return Id;
} catch (Exception e) {

return 0L;
}


..........................
i'm expecting a Long from database and when i have no result set above is throwing an error saying expected 1 actual 0,

then, if i return 0 in the exception, evethough query has no resultset it is working fine.

how can i refactor this, or is there any better solution ...
Junilu Lacar
Bartender

Joined: Feb 26, 2001
Posts: 4457
    
    6

Sandeep Suram wrote:... evethough query has no resultset it is working fine.

how can i refactor this, or is there any better solution ...


Just to make sure we're on the same page, "Refactoring" means to change the design without changing the behavior. I'm not sure you were using the word in this sense though. Does the code as it is now work for your purposes or not? If not, then what exactly is the problem with it? Is returning 0 even though there were no rows in the resultset a problem for you?

Now, if you were talking about refactoring in the sense that I defined, I suggest you NOT use hard-to-read abbreviations like 'dob' and 'PId' -- spell things out for Pete's sake! Code should be readable and names like 'dob' and 'persIdDob' are borderline unreadable. What's wrong with spelling things out anyway? If you're going to be lazy about typing, use an IDE with a content-assist feature; most IDEs like Eclipse make it really easy (Ctrl-Space and Ctrl+1 are indispensable shortcuts for lazy typists like me). Also, it's bad form to have a return statement in a try-catch block, move the return outside the try-catch.


Junilu - [How to Ask Questions] [How to Answer Questions]
Jayesh A Lalwani
Bartender

Joined: Jan 17, 2008
Posts: 2342
    
  28

Also, don't use catch all exceptions and then swallow the error. Exception might be thrown for a lot of cases, no result being one of them. If you want to return 0 for a specific case(in this case no result), catch the specific exception and return 0. Any exception that cannot be handled should either be propagated up or caught and rethrown upto the topmost layer.
Sandeep Suram
Greenhorn

Joined: Feb 22, 2011
Posts: 7
@Lacar:

Yes, i meant to change the design without changing the behavior. I'm new to Java, please ignore anything inappropriate. And thank you for the suggestions.
I have changed the 'PID' 'DOB' to make readable.

This code is solving my purpose, but i somehow i felt there could be a better way to do this. When "getPersIdDob", my query string, fetches no resultset then "EmptyResultDataAccessException" is thrown. so i'm catching that exception and returning 0 (now this solved the purpose).


@Jayesh.
Thank you Jayesh, i changed my code to the following by catching a particular exception, i'm returning 0.

public Long getPersonIdFromDateOfBirth(String lastName, Date dob) {
Long personId = 0L;
try {

personId = jdbcTemplate.queryForLong(getPersonId, new Object[{ssn});

} catch (EmptyResultDataAccessException e) {

return 0L;
} catch (Exception e){
// logging
}
return personId;
}
}
Junilu Lacar
Bartender

Joined: Feb 26, 2001
Posts: 4457
    
    6

This looks like code that should be in a Data Access Object (DAO). A DAO provides a layer of abstraction between the application's domain layer and the persistence mechanism, usually a relational database. As such, I usually make the DAO API deal with domain objects, not fine grained field values. This allows you to keep the DAO API more stable even though your domain object definition might still be changing.

With this in mind, I would change the design of the API:


This is the general pattern I use to design and code a DAO and its client code. I use Spring's DAO support classes to make the code even cleaner. Note that the DAO API returns lists of Person objects from "find" methods. This gives you a very intuitive convention of using an empty list to represent a "no matches found" condition. A DAO should also generally be passed Person objects for operations like save, update, and delete.
Junilu Lacar
Bartender

Joined: Feb 26, 2001
Posts: 4457
    
    6

Here's an exercise in refactoring for you: the example code I gave is close to nice but not quite nice. Names are very important and you should choose names that communicate the exact intent. A poorly chosen name can be misleading and cause confusion and this can lead to bugs. Can you find something in the example code that might be misleading? Can you suggest a change that would make the code communicate the exact intent?
Sandeep Suram
Greenhorn

Joined: Feb 22, 2011
Posts: 7
Thank you Lacar.

In the code snippet you gave, i find "JdbcPersonDao" as misleading, Instead it could be "JdbcPersonDaoImpl" as this is an implementation.

Is this correct, please correct me if i'm wrong.
Junilu Lacar
Bartender

Joined: Feb 26, 2001
Posts: 4457
    
    6

Sandeep Suram wrote:In the code snippet you gave, i find "JdbcPersonDao" as misleading, Instead it could be "JdbcPersonDaoImpl" as this is an implementation.


Not quite what I had in mind. The "Jdbc" prefix to the interface name is meant to indicate that it is a JDBC-based implementation of the PersonDao API. Other implementations might be InMemoryPersonDao, or HibernatePersonDao. There is no need to add an "Impl" suffix when using this naming convention.

There is something else... focus on the PersonDao interface, what do the method names say to you? What do you think/expect them to do if the interface definition was all you had to go on and had no knowledge of what any actual implementations did?

Another hint: Suppose you were coding and testing the BirthdayListingService and found that it didn't seem to be working. You verified that you did in fact have rows in your database with people who should be listed as celebrating their birthdays today. What might have misled you into thinking that the code for showTodaysCelebrants should actually work?
Sunderam Goplalan
Ranch Hand

Joined: Oct 10, 2011
Posts: 73
Junilu,

Did you have this fix in mind to make the code convey the exact intent?

Instead of this,



use this



SCJP 5.0 , SCEA Java EE 5
Junilu Lacar
Bartender

Joined: Feb 26, 2001
Posts: 4457
    
    6

Assuming the code in the BirthdayListingService was the intended functionality, the method name findByBirthDate doesn't quite match up. Going just by the name and nothing else, the most logical thing to expect from findByBirthDate is a list of Person objects whose date of birth match exactly the given date.

I would favor defining another method named something like findHasBirthdayOn(Date someDate) -- this better matches the intent of the showTodaysCelebrants() method, is more grammatically correct when you read it, and is pretty much self-explanatory.

A well-chosen name in a program allows you to apply the same kind of semantics that you would with natural language. This makes for better quality and more readable code. The name findHasBirthdayOn(Date someDate) actually leads us to infer some reasonable assumptions/expectations about its behavior. For example, using our knowledge about birthdays and birth dates, we can reasonably expect that it would not return any Person whose birth date is after the given date since a person only celebrates birthdays after they are born.

You could experiment with names like whoseBirthdayIsOn(Date someDate) or simply withBirthdayOn(Date someDate) but there is also the consideration of keeping symmetry and consistency in your API, so if I have other methods that are named findWhatever, I'm anal-retentive enough as a designer to want to keep some symmetry and consistency in the method names.
 
I agree. Here's the link: http://aspose.com/file-tools
 
subject: Refactoring a Method