3
\$\begingroup\$

I have a use case which says:

Your REST service will consume a JSON object that contains a list of emails for subscribers at the start of the subscription period, and a list of subscribers at the end of the subscription period. Your service must return a list of subscribers who are new, subscribers who stayed, and subscribers who unsubscribed.

Example JSON input:

{
    "start" : [
        "test1@test.com",
        "test2@test.com"
    ],
    "end" : [
        "test1@test.com",
        "test3@test.com"
    ]
}

Example JSON output:

{
    "new" : [
        "test3@test.com"
    ],
    "stayed" : [
        "test1@test.com"
    ],
    "unsubscribed" : [
        "test2@test.com"
    ]
}

The way I implemented it is:

Controller

@POST
@Timed
@Consumes(MediaType.APPLICATION_JSON)
public SubscriptionResponse subscriptionDetail(SubscriptionRequest request) {
    return new SubscriptionResponse(request);
}

Response

public SubscriptionResponse (SubscriptionRequest request) {
        //Get the common subscriptions
        List<String> common = new ArrayList<String>();
        common.addAll(request.getEnd());
        common.retainAll(request.getStart());
        //Filter out unsubscribed
        List<String> unsubscribed = request.getStart();
        unsubscribed.removeAll(common);
        //Get the new subscription
        List<String> newSub = request.getEnd();
        newSub.removeAll(common);
        unsubscribed.removeAll(common);

        this.newSub = newSub;
        this.stayed = common;
        this.unsubscribed = unsubscribed;
    }

Is it the right way of implementing it? What are the pros (if any) and cons?

\$\endgroup\$
2
\$\begingroup\$

This is one way of implementing it. Whether it's right or not depends on many conditions, and we could only speculate. Here are some example problems anyway, that could be serious issues:

  1. The current solution is inefficient for large lists, because for example, for list1.retainAll(list2) the worst case complexity is \$O(N M)\$ where \$N\$ and \$M\$ are the sizes of the two lists. (For more details, see the source code of ArrayList.retainAll. You could make that \$O(N)\$ by converting list2 to a Set.

  2. The current solution modifies request.getStart() and request.getEnd(). Maybe that's not ok, then the current solution should be adjusted to work with copies instead.

On the other hand, a good thing about the current solution is that it's short and idiomatic, and it doesn't waste memory (at the expense of CPU).

Lastly, a minor issue is that the comments are pointless, they only state the obvious. The code would be easier to read if you made those lines empty.

\$\endgroup\$
1
\$\begingroup\$

There is potentially one overlooked problem here: Does SubscriptionRequest.getStart/End() always return a new List every time?

If not, you'll have a problem here:

List<String> unsubscribed = request.getStart();
unsubscribed.removeAll(common);

List<String> newSub = request.getEnd();
newSub.removeAll(common);

These will modify the backing list inside your SubscriptionRequest method argument, which is most likely an undesirable outcome.

Fixing them is just a matter of wrapping it in a new ArrayList object (using Java 7's diamond operator):

List<String> unsubscribed = new ArrayList<>(request.getStart());
unsubscribed.removeAll(common);

List<String> newSub = new ArrayList<>(request.getEnd());
newSub.removeAll(common);

edit Oh yeah, you have one extra unsubscribed.removeAll(common);.

\$\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.