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 {}

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(""));

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.