jQuery in Action, 3rd edition
The moose likes OO, Patterns, UML and Refactoring and the fly likes Code smell - if statements and instanceof 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 "Code smell - if statements and instanceof" Watch "Code smell - if statements and instanceof" New topic

Code smell - if statements and instanceof

Pho Tek
Ranch Hand

Joined: Nov 05, 2000
Posts: 782

I have a class that looks like this .

A typical execution:

Currently the do method performs a bunch of if branching based on the value in Holder.clazz:

How can I improve this code ?




Ilja Preuss

Joined: Jul 11, 2001
Posts: 14112
So, if I understand you correctly, you want to do different things based on the type of an object, but you don't want to implement those things inside the object itself?

Sounds like a job for the Visitor pattern...

The soul is dyed the color of its thoughts. Think only on those things that are in line with your principles and can bear the light of day. The content of your character is your choice. Day by day, what you do is who you become. Your integrity is your destiny - it is the light that guides your way. - Heraclitus
Lasse Koskela

Joined: Jan 23, 2002
Posts: 11962
I would also consider introducing a factory class for creating different implementations of the Holder class based on the Class you pass to the factory method. Ilja's tip about the Visitor pattern sounds good as well.

Author of Test Driven (2007) and Effective Unit Testing (2013) [Blog] [HowToAskQuestionsOnJavaRanch]
Joe Nguyen
Ranch Hand

Joined: Apr 20, 2001
Posts: 161
What is the purpose of supplying both the object and its class?

new Holder(po, PurchaseOrder.class);
Junilu Lacar

Joined: Feb 26, 2001
Posts: 6529

My bet ($0.02) is that the code in each of the if blocks smells of feature envy:

Junilu - [How to Ask Questions] [How to Answer Questions]
steve souza
Ranch Hand

Joined: Jun 26, 2002
Posts: 862
The code definitely has a smell. Pretty much any time you have an if condition based on type it should be refactored unless it is in a factory method.

It is a little hard to give a definitive recommendation without knowing a little more about what you are trying to do. In your example the Holder has no value if it just executes the other classes do method.

I would make all 3 classes implement a common interface (I'll call it the DoInterface). Forgive any syntax errors.

Your code would look as follows now:

[ edited to break long line -ds ]
[ August 21, 2004: Message edited by: Dirk Schreckmann ]

http://www.jamonapi.com/ - a fast, free open source performance tuning api.
JavaRanch Performance FAQ
I agree. Here's the link: http://aspose.com/file-tools
subject: Code smell - if statements and instanceof
It's not a secret anymore!