Concatenating multiple feedback messages

Question

The goal is to build a String from a given set where Feedback is an interface and DeliveryFeedback, and ProductFeedbackare its implementing classes.

private void formatOrderFeedbackZendeskBody(final ImmutableSet<Feedback> feedback) {
 StringBuilder feedbackMessage = new StringBuilder("The feedback: ");
 feedback.forEach(
 f -> {
 if (f instanceof DeliveryFeedback) {
 feedbackMessage
 .append("\n\nType: ")
 .append(FeedbackType.DELIVERY)
 .append("\nReason(s): ")
 .append(
 ((DeliveryFeedback) f)
 .getReasons()
 .stream()
 .map(Reason::name)
 .collect(joining("\n\t\t\t")));
 if (((DeliveryFeedback) f).getComment().isPresent()) {
 feedbackMessage
 .append("\nComment: ")
 .append(((DeliveryFeedback) f).getComment().get());
 }
 } else if (f instanceof ProductFeedback) {
 feedbackMessage
 .append("\n\nType: ")
 .append(FeedbackType.PRODUCT)
 .append("\nReason(s): ")
 .append(
 ((ProductFeedback) f)
 .getReasons()
 .stream()
 .map(
 ProductFeedbackInterface.Reason
 ::name)
 .collect(joining("\n\t\t\t")))
 .append("\nArticle ID: ")
 .append(((ProductFeedback) f).getArticleId());
 if (((ProductFeedback) f).getImageIds().isEmpty()) {
 feedbackMessage.append(
 "\nNo images available.");
 } else {
 feedbackMessage
 .append("\nImage URLs: ")
 .append(
 ((ProductFeedback) f)
 .getImageIds()
 .stream()
 .collect(joining(", ")));
 }
 if (((ProductFeedback) f).getComment().isPresent()) {
 feedbackMessage
 .append("\nComment: ")
 .append(((ProductFeedback) f).getComment().get());
 }
 } 
 });
}

I am getting the expected String:

Detailed feedback: 
Type: DELIVERY
Reason(s): REASON1
 REASON2
Comment: another comment
Type: PRODUCT
Reason(s): REASON3
 REASON4
Article ID: idsug
Image URLs: idsug, dfgh
Comment: comment
Type: PRODUCT
Reason(s): REASON5
 REASON6
 REASON7
Article ID: dfddddddddgh
No images available

I just think this code is not very readable and maybe there is another better way to do this that I am missing :)

Edit 1: Just to make clear, the usage of the OrderFeedback interface is only to give a type:

@JsonSubTypes({
 @Type(name = FeedbackType.PRODUCT, value = ProductFeedback.class),
 @Type(name = FeedbackType.DELIVERY, value = DeliveryFeedback.class),
})
@JsonTypeInfo(include = As.PROPERTY, property = "type", use = Id.NAME)
public interface OrderFeedback {}

• Pimgd: Welcome to Code Review! I have (partially) rolled back the last edit. Please see what you may and may not do after receiving answers.
• imTachu: @Pimgd, but it was a typo! :C I slightly changed just the names of some stuffs because code privacy stuffs, you know...
• Pimgd: I'll rollback the rollback, but this is one of the issues as to why hypothetical ("my real code is something like this, but I'm not allowed to post it") is off-topic.
• Pimgd: I replaced my comment about BadProductFeedback with an example of how to make use of a generic interface

Answer 1

Polymorphism failure. Determine who is responsible for deciding what the output is supposed to look like and go from there. If the feedback is responsible, just implement and overload toString(). If the printer is responsible, find a way to get most of the detailed information into the interface.

if (f instanceof DeliveryFeedback) {
} else if (f instanceof ProductFeedback) {

This is just wrong.

Basically, what's the point of having the interface if you're just gonna side-step the interface and take a look at the implementing class?

For example...

if (((DeliveryFeedback) f).getComment().isPresent()) {
 feedbackMessage
 .append("\nComment: ")
 .append(((DeliveryFeedback) f).getComment().get());
}
if (((ProductFeedback) f).getComment().isPresent()) {
 feedbackMessage
 .append("\nComment: ")
 .append(((ProductFeedback) f).getComment().get());
}

Why does the Feedback interface not have a getComment method? Then you could just

feedbackMessage.append(f.getComment().map(c -> "\nComment: " + c).orElse(""));

• imTachu: Sorry about the BadProductFeedback, it was a typo while preparing the question. I have given more detail on the interface usage also.
• imTachu: Great idea! I added getComment() to the interface... For the code, actually I think this looks cleaner: f.getComment().ifPresent(c -> feedbackMessage.append("\nComment: ").append(c)); WDYT?
• Pimgd: @LorenaSalamanca ifPresent is better, I forgot it was available

Answer 2

Interface over implementation

private void formatOrderFeedbackZendeskBody(ImmutableSet<Feedback> feedback) {
 // ...
}

It's strongly recommended to code against interfaces instead of implementations, so that you can easily switch the implementations when it is desirable, e.g. writing tests. This method should be accepting a Set instead:

private void formatOrderFeedbackZendeskBody(Set<Feedback> feedback) {
 // ...
}

Relying on toString()...

If you can modify the toString() implementation of your Feedback implementing classes, you should do that instead. The body of formatOrderFeedbackZendeskBody(Set) can be as simple as:

private String formatOrderFeedbackZendeskBody(final Set<Feedback> feedback) {
 return feedback.stream()
 .map(Object::toString)
 .collect(Collectors.joining("\n\n", "The feedback: ", ""));
}

... or overloaded output building methods

Alternatively, you can use overloaded methods that take in a specific Feedback implementation (one of the rare cases where you'll actually want the method parameter to use the implementation), and a helper method to do the appropriate casting. It's all about managing the resolution outside of the stream-based processing. :)

private static String print(Feedback feedback) {
 if (feedback instanceof DeliveryFeedback) {
 return print((DeliveryFeedback) feedback);
 }
 if (feedback instanceof ProductFeedback) {
 return print((ProductFeedback) feedback);
 }
 return null; // or throw an Exception?
}
private static String print(DeliveryFeedback deliveryFeedback) {
 // ...
}
private static String print(ProductFeedback productFeedback) {
 // ...
}
private String formatOrderFeedbackZendeskBody(final Set<Feedback> feedback) {
 return feedback.stream()
 .map(ThisClass::print)
 .collect(Collectors.joining("\n\n", "The feedback: ", ""));
}

Answer 3

Avoid the large indentation caused by the lambda. In this case, using a forloop results in much more readable code.