• 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

[refactoring] Consider these 2 similar methods

 
Ranch Hand
Posts: 782
Python Chrome Ubuntu
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Greetings,
Consider these two methods:

These are methods from a DAO class.
The second method is not much different from the first except that its query has additional condition in its WHERE clause. The methods were written by different developers. The additional WHERE condition is " WHERE MONTHDIFF(joiningDate, THISMONTH()) = 0 ". "joiningDate" refers to the date the user joined the company. NOTE: I'm using pseudo SQL statements here for discussion sake.
What kind of refactoring would you do on them ? Personally, I want to consolidate them into one method and rename it.
e.g.

The 2nd param will decide whether I should run the additional WHERE condition. Is this any good ?
Regards,
Gavin
[ April 28, 2004: Message edited by: Pho Tek ]
 
Ranch Hand
Posts: 45
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
If I were the one that had to use this API, I would prefer two methods because there are two different operations. One is "give me a collection of all the employees in a department", the second is "give me a collection of the employees in this department that joined this month". I would also prefer that the methods be given names that distguish them from each other. For example, the second method could be called "getNewEmployeesByDepartment" or something like that.
You could also generalize the second so that it accepts a date as a parameter and returns the collection of employees that joined that month if you think that would be useful.
 
Pho Tek
Ranch Hand
Posts: 782
Python Chrome Ubuntu
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Thanks Greg. It's always good to get somebody's opinion.
Pho
 
author and iconoclast
Posts: 24207
46
Mac OS X Eclipse IDE Chrome
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I agree with Gregg. The boolean flag is a kind of "magic number" here. To use the combined method, you'd have to stop and think whether "true" or "false" meant to return the users that joined this month.
Now, as far as code structure, I'd aim for something like

This consolidates the JDBC code (your original goal) but preserves the richer interface. I renamed a method, too, to make its purpose more clear.
 
(instanceof Sidekick)
Posts: 8791
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I'd also stop to ask if the object that has all the users shouldn't be doing the work you are just about to do. Getting somebody else's data is always a sign to stop and think. Here are some fun references for this question:
http://www.surfscranton.com/architecture/KnightsPrinciples.htm
http://www.surfscranton.com/architecture/LawOfDemeter.htm
and one that sends lots of folks into absolute fits:
Why getter and setter methods are evil
 
Pho Tek
Ranch Hand
Posts: 782
Python Chrome Ubuntu
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Stan,


I'd also stop to ask if the object that has all the users shouldn't be doing the work you are just about to do.


Do you mean that clients should be doing the collection filtering themselves ?

BTW in my refactoring work, I am encountering a lot of redundancies like these whereby the queries are subsets of the findAll query e.g.

1) findAllFoo

2) findActiveFoos

3) findTerminatedFoos

4) findFooByCreateDate


Personally I think it's easier to filter at the SQL (or Hibernate HQL) query level.

OK.... By thinking aloud, I'm beginning to feel that perhaps only 2 & 3 can be refactored into one method. I'd rather not to use Dynamic SQL to construct the query. I prefer to use PreparedStatement; which is certainly apt for merging 2 & 3.

Regards,

Pho
 
Stan James
(instanceof Sidekick)
Posts: 8791
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Do you mean that clients should be doing the collection filtering themselves ?



No, I meant the opposite. What is it the client is going to do with all these objects? Maybe the service should be doing that instead of handing you the objects. This is from one of the links I posted above:

Consider the story of �The Paperboy and the Wallet,� from our friend David Bock. Suppose the paperboy comes to your door, demanding payment for the week. You turn around, and the paperboy pulls your wallet out of your back pocket, takes the two bucks, and puts the wallet back. As absurd as that sounds, many programs are written in this style, which leaves the code open to all sorts of problems (and explains why the paper boy is now driving a Lexus).



See how giving out a list of users is like giving the paperboy your wallet?
 
Pho Tek
Ranch Hand
Posts: 782
Python Chrome Ubuntu
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Stan,

Looking at the existing app, it performs two very distinct things:

a) Executing business services
b) Loading data for presentation purposes (i.e. passing them to Struts)

Currently both (a) and (b) are being served by the same Session Facade.
Perhaps they should be separated. But consider this, even finder methods for entity beans are colocated with the bean itself.

How would you implement (b)?

Note that we are passing back Value Objects to struts (subset of fields).

Thanks.

Pho
 
Doody calls. I would really rather that it didn't. Comfort me wise and sterile tiny ad:
a bit of art, as a gift, the permaculture playing cards
https://gardener-gift.com
reply
    Bookmark Topic Watch Topic
  • New Topic