Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

Please review the following code. Methods getFirsts and getSeconds return a list of objects which implement CommonInterface and both of them are private. Is it a good or bad design.

@Override
public final List<? extends CommonInterface> getObjects(final CommonEnum type) {
    if (type == null) {
        return new ArrayList<CommonInterface>();
    }
    switch (type) {
        case FIRST:
            return getFirsts();
        case SECOND:
            return getSeconds();
        default:
            return null;
    }
}
share|improve this question
3  
If this list returned through getFirsts or getSeconds are mutable, you have to realize that you are exposing the lists to mutation to any caller of "getObjects" (which is public). – corykendall Oct 19 '12 at 22:41
my idea was to expose only one method in the interface and hide the if else/ switch logic in one method, to prevent such statements in other places in the code and to get by the caller the list of appropriate objects only by CommonEnum value. I am asking because I often using such structures so I'm curious the opinions. – Łukasz Rzeszotarski Oct 19 '12 at 22:46
2  
Cory's point is that by returning the actual private list you are allowing callers to modify it directly. If this is what you want, perhaps you might want to rethink that part of your design. Also, why is null an acceptable value for type? It would help to have a concrete example with how you use this idiom. – David Harkness Oct 20 '12 at 4:32
@corykendall: I think your comment would be worth an answer. – palacsint Oct 20 '12 at 6:38

1 Answer

up vote 6 down vote accepted

It's hard to say anything since it seems rather a pseudo-code. Anyway, two notes which you might find useful:

  1. I guess you could replace the switch-case structure with polymorphism. Two useful reading:

  2. Are you sure that returning null in the default is fine? I'd consider returning an empty list (as it returns when type == null) or throwing an exception (IllegalStateException, for example) if it's a programming error. (See: The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies)

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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