1 
public Object getValue() { ValueItem valueItem = null; Object returnValue = null; if(this.value instanceof StringValueImpl) { valueItem = (StringValueImpl) this.value; } else if(this.value instanceof ListValueImpl) { valueItem = (ListValueImpl) this.value; } else if(this.value instanceof MapValueImpl) { valueItem = (MapValueImpl) this.value; } if(valueItem!=null) returnValue = valueItem.getValue(); return returnValue; }

ValueItem is an interface which is implemented by ListValueImpl, MapValueImpl etc .. I want return value which is an object. The code works fine but i was wondering if this can be improved in any way ?

Improve this question
5
  • 2
    public Object getValue() { return this.value; } should work just fine Commented Sep 30, 2011 at 15:43
  • What's the point of the ValueItem interface? If its implementations hold unrelated types like strings and lists, I doubt it's an abstraction of any functionality they have in common. If its purpose is just that you can call getValue() on it and receive an Object, why not just use Object instead of ValueItem for your variables? Commented Sep 30, 2011 at 15:47
  • Feel bad for whoever (if anyone) has to maintain your code.. Commented Sep 30, 2011 at 15:50
  • Do you have more implementations of ValueItem than StringValueImpl, ListValueImpl and MapValueImpl? Commented Sep 30, 2011 at 15:54
  • This looks almost like he is trying to accomplish the same goals as the Joda Beans project, except Joda's Property interface is now ValueItem. Commented Sep 30, 2011 at 16:26

3 Answers 3

Reset to default
6

What is the type of this.value? If it is ValueItem then you don't need to do any of this and can replace the method with this:

public Object getValue() { Object returnValue = null; if(this.value!=null) returnValue = this.value.getValue(); return returnValue; }

Or even shorter:

public Object getValue() { return this.value!=null ? this.value.getValue() : null; }

If this.value is not of type ValueItem but it has to contain a ValueItem, then you have a design problem at your hand.

Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

Almost, unless there are more implementations of ValueItem than StringValueImpl, ListValueImpl and MapValueImpl.
@Howard: if there are more implementations, then not mentioning them here is probably a bug anyway.
I agree that such information should be given. Nevertheless it is worth mentioning because it changes functionality if it is the case.
@Joachim:The shorter version, does it have any performance benefit over the longer?
@user384706: not really. And please believe me when I tell you that you won't find any relevant performance optimizations at that level. Most of your performance will be lost in design issues and the rest you'll have to hunt for with profilers: don't try to guess when it comes to performance. You will be wrong. Everyone is wrong when they try to guess where the performance problem is.
1

My inclination is that your getValue() isn't doing anything for you at all. You're detecting what class it is, casting it to that class, then shoving it into an Object again. ...so you'll have to do the same kind of detection on the caller's side of getValue() anyways!

Personally, I'd do it like this:

public boolean isaStringValueImpl() { return (this.value instanceof StringValueImpl); } public boolean isaListValueImpl() { return (this.value instanceof ListValueImpl); } public boolean isaMapValueImpl() { return (this.value instanceof MapValueImpl); } public StringValueImpl getAsaStringValueImpl() { return (StringValueImpl)this.value; } public ListValueImpl getAsaListValueImpl() { return (ListValueImpl)this.value; } public MapValueImpl getAsaMapValueImpl() { return (MapValueImpl)this.value; }

In addition to the regular getter:

public ValueItem getValueItem() { return this.value; }

But even with all this, I'd say that you might have a larger design issue that could be cleaned up.

Improve this answer

Comments

0

How about a generics based type safe version.

public abstract class ValueItem<T> { public abstract T getValue(); public class StringValueImpl extends ValueItem<String> { private String value; public String getValue() { return value; } } public class ListValueImpl extends ValueItem<List<?>> { private List<?> value; public List<?> getValue() { return value; } } public class MapValueImpl extends ValueItem<Map<?, ?>> { private Map<?, ?> value; public Map<?, ?> getValue() { return value; } } }
Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.