Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a Factory class that gives out instances of ResponseImpl class. It accepts one Destination class and up to four Source classes. At least one of the four Source classes should be not null.

So, instead of asking calling program to pass nulls in the arguments to Factory class, I have overloaded the method to accept at least one up to four Source classes. But, something tells me that this way of doing it is not a good idea. Although this works, I feel like this can be handled better.

Could you let me know how to make the following piece of code better?

public static Response initResponse(final Destination destination, final Source1 source1) {
    return initResponse(destination, source1, null, null, null);
}

public static Response initResponse(final Destination destination, final Source1 source1, final Source2 source2) {
    return initResponse(destination, source1, source2, null, null);
}

public static Response initResponse(final Destination destination, final Source1 source1, final Source2 source2, final Source3 source3) {
    return initResponse(destination, source1, source2, source3, null);
}

public static Response initResponse(final Destination destination, final Source1 source1, final Source2 source2, final Source3 source3, final Source4 source4) {
    try {
        if(source1 == null && source2 == null && source3 == null && source4 == null) {
            throw new ABODataException("Atleast one source has to be not null");
        }
        return ResponseImplManager.getInstance(destination, source1, source2, source3, source4);
    } catch (Exception e) {
        throw new ABODataException("Unable to instantiate Response:"+e.getMessage());
    }
}
share|improve this question
add comment

2 Answers

up vote 7 down vote accepted

It's a bit problematic to refactor when the parameters are from four different types of classes. I have a couple of alternative approaches for you though, pick one if you like one.

One option would be to make your classes Source1, Source2, etc. implement one common interface. Perhaps this would even simplify your ResponseImplManager.getInstance? If they would be of the same interface (or shared superclass would also be possible of course), then you could use a method like this:

public static Response initResponse(final Destination destination,
    final SourceInterface... sources) {
    // sources is treated as an array here
}

Or you could send a list of sources

public static Response initResponse(final Destination destination,
    final List<SourceInterface> sources)

Another option would be to use a Builder pattern

Whichever way you go in, I got one more suggestion. Use the two-parameter constructor for Exceptions to provide "Caused by" information, this will give you a more detailed stack trace

    } catch (Exception e) {
        throw new ABODataException("Unable to instantiate Response", e);

        // or use this:
        throw new ABODataException("Unable to instantiate Response: " + e.getMessage(), e);
    }
share|improve this answer
2  
I was going to add a edit about using variable arguments too but then I noticed that the classes were not the same and then reached the same conclusion as your. a big +1 for you. –  Bruno Costa Jan 31 at 17:09
1  
Looks like using Builder pattern is the way to go. I didn't think about that. –  would_like_to_be_anon Feb 3 at 20:55
1  
@would_like_to_be_anon That's what you have us for, to think for you after you've done the initial thinking :) –  Simon André Forsberg Feb 3 at 20:57
add comment

It seems to me that you are not quite making good use of exceptions and as long as I believe it is possible that you are disrespecting some principles. First things first, you really should be using IllegalArgumentException because that is the java standard exception class for invalid arguments. You shouldn't also throw a exception and then catch it so your throw statement should be outside the try block. Why should you ever raise a error to treat it imeadiatly? You should avoid it instead!

public static Response initResponse(final Destination destination, final Source1 source1, final Source2 source2, final Source3 source3, final Source4 source4) {
    if(source1 == null && source2 == null && source3 == null && source4 == null) {
        throw new IllegalArgumentException ("Atleast one source has to be not null");
    }
    try {
        return ResponseImplManager.getInstance(destination, source1, source2, source3, source4);
    } catch (Exception e) {//if possible catch the specific type of the exception that is thrown by getInstance thought I don't see it as a huge issue as you don't have any other method calls
        throw new ABODataException("Unable to instantiate Response:"+e.getMessage());
    }
}
share|improve this answer
add comment

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.