0

I am currently writing an XML converter for a supply chain project. We use Requests and Orders.

The converter has multiple method that currently do same but are separately implements for requests and orders.

I have therefore created an abstract class to improve maintainability of the code and used a generic type: 

public abstract class AbstractConverter<T extends BusinessObject>

Then I have the specific implementations for the actual converters

public class OrderConverter extends AbstractConverter<Order> public class RequestConverter extends AbstractConverter<Request>

As I said, I have several methods in the two specific classes that basically do the same, so I naturally want them in the abstract class. I have now added the following method to the abstract class: 

protected Comment createComment(T obj) { String remark; if (obj instanceof Order) { remark = ((Order) obj).getRemark(); } else if (obj instanceof Request) { remark = ((Request) obj).getRequestRemark(); } else { throw new IllegalArgumentException("This method does not support objects of the type " + obj.getClass().toString()); } return new Comment(remark); }

My question now is: is this the way to go or is there a more elegant way to use generics in this context?

I need this solved but I also want to use good style.

Improve this question
1
  • What does "basically" mean? There is no "a little bit pregnant". They are the same, then pull them into the abstract class or they are not, then implement them separately (see wero's answer). Increasing complexity and UNmaintainability by introducing if/else/instanceof blocks is IMHO really a bad idea. Commented May 25, 2016 at 9:07

2 Answers 2

Reset to default
3

The natural object oriented solution is to make createComment an abstract method 

protected abstract Comment createComment(T obj);

and let the subclasses implement it:

public class OrderConverter extends AbstractConverter<Order> { protected Comment createComment(Order order) { return new Comment(order.getRemark()); } } public class RequestConverter extends AbstractConverter<Request> { protected Comment createComment(Request request) { return new Comment(request.getRequestRemark()); } }
Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

2

I'd suggest extracting the getRemark method to an interface which both Request and Order implements.

That way you can simply check if the incoming generic object is an instance of the interface.

protected Comment createComment(T obj) { if (obj instanceof Remarkable) { return new Comment(((Remarkable) obj).getRemark()); } throw new IllegalArgumentException("This method does not support objects of the type " + obj.getClass().toString()); }
Improve this answer

5 Comments

... and you can even skip that instanceof if it is possible to have BusinessObject also implement that Remarkable interface.
@DaDaDom then there's no use in having an interface. If all BusinessObject can return remarks, just use an abstract method in that class.
Thanks for this input. Unfortunately I cannot do this, as I cannot change the Order and Request Classes as this would mean a very large refactorinf of existing code.
The question is also more a general one, as I have a few dozen methods that are currently implemented twice and I don't see the point of having two separate implementation if the code does basucally the same
@Fish-Guts That sounds like bad coding. You even admitted wanting to have more elegant code. I'd extract all methods that are "almost" duplicated either into interfaces, or if applicable, into the abstract class directly. It drastically improves readability, and usefullness of your objects.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.