• 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 Smell

 
Ranch Hand
Posts: 515
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
So I'm sifting through some code that I didn't create BUT never the less it might still need to be refactored... Was curious if you saw this as a refactoring and if so then what would you suggest. AND what refactoring smell does it seem like...

I have a couple methods that have very similar signatures. I've created some generic methods because it really doesn't matter what the code is.. just the example matters most. The two methods are as follows...

public void processInvoice(InvoiceData invData)

public void processInvoice(InvoiceData invData, boolean otherParameter)

The first method ends up calling the 2nd with a default value of false... like so...

public void processInvoice(InvoiceData invData){
prcoessInvoice(invData, false);
}

The second method processes the data accordingly but when the conditional data comes up, it checks to see if the boolean is set and processes accordingly� something like this...

...

if (otherParameter!=false){
///do special code...
}

To me this seems like it can be done a better way. Your thoughts...
 
Ranch Hand
Posts: 862
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
It is called refactoring whether you are refactoring your code or someone elses.

One possible improvment would be to get rid of the boolean from the public interface and come up with two more meaningful method names for the action. But if the method name seems forced then that might not work (my names are forced).

For example:
public void processInvoiceMethodA(InvoiceData invData)
public void processInvoiceMethodB(InvoiceData invData)

You could also try an interface. If most methods take InvoiceData as an argument then make it an instance variable. An interface is usually a good idea regardless. If the only difference between your 2 implementations is the 'if' condition then I may still combine them into the same class.

Invoice invoice=new Invoice(invoiceData);
invoice.processInvoice();

I also wanted to mention method names. Typically I like the noun.verb() style. However, I waver on this one as sometimes if you pair method names that don't stand on their own with bad variable names the code becomes more difficult to understand.

invoice.process();// would be easy enough to understand
inv.process():// would not be easy to understand
inv.processInvoice();// would still be easy to understand despite bad variable name

However, by making your method name a verb your code can become more abstract and so potentially more versatile.

For example the following could share the same ProcessableItem interface.

invoice.process();
order.process();

Any design choice has pros and cons.
 
Ranch Hand
Posts: 239
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
A method must do only one thing. If it does anything more, split it into two methods with meaningful names.

In this case, processInvoice is doing two things:
1. Processing default data
2. Processing conditional data i.e. special code when the flag is true

Refactor the processInvoice(InvoiceData invData) it into two methods for e.g.:
1. processMainFlow(InvoiceData invData)
2. processSpecialFlow(InvoiceData invData)

The second method is called from the calling program whenever it wants special process.
 
Ranch Hand
Posts: 308
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
you mentioned the implementation details (-> default value is passed, boolean value is checked in delegate method), but in interface view this knowledge should be encapsulated. so you should not pull up the implementation-details to the interface.

i tend to try to keep interfaces tidy and keep "redundant helper"-methods out from the interface.
if i can say from the interface point of view (design by contract) that method A() can be easily simulated by B(), just by passing a different parameter i would parametrize my method A(...), so in your case i would parametrize and only place one method in interface:

public void processInvoice(InvoiceData invData, boolean otherParameter)

but just like that, otherParameter has not a very good naming (steve already mentioned importance of namings), so clients would not instantly see, what it means and could get confused. in the end you could define interface (assumed boolean flag is for validation):

interface Invoice{
...
public void process(InvoiceData invData, boolean skipValidation)
...
}
 
With a little knowledge, a cast iron skillet is non-stick and lasts a lifetime.
reply
    Bookmark Topic Watch Topic
  • New Topic