5
\$\begingroup\$

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 {}
\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review! I have (partially) rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Pimgd Apr 21 '17 at 13:20
  • \$\begingroup\$ @Pimgd, but it was a typo! :C I slightly changed just the names of some stuffs because code privacy stuffs, you know... \$\endgroup\$ – imTachu Apr 21 '17 at 13:35
  • \$\begingroup\$ 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. \$\endgroup\$ – Pimgd Apr 21 '17 at 13:43
  • \$\begingroup\$ I replaced my comment about BadProductFeedback with an example of how to make use of a generic interface \$\endgroup\$ – Pimgd Apr 21 '17 at 13:55
6
\$\begingroup\$

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(""));
\$\endgroup\$
3
  • \$\begingroup\$ Sorry about the BadProductFeedback, it was a typo while preparing the question. I have given more detail on the interface usage also. \$\endgroup\$ – imTachu Apr 21 '17 at 13:10
  • \$\begingroup\$ 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? \$\endgroup\$ – imTachu Apr 21 '17 at 18:17
  • \$\begingroup\$ @LorenaSalamanca ifPresent is better, I forgot it was available \$\endgroup\$ – Pimgd Apr 21 '17 at 19:17
2
\$\begingroup\$

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: ", ""));
}
\$\endgroup\$
0
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.