Is there anything bad in declaring nested class inside interface in java?

Question

I have an interface ProductService with method findByCriteria. This method had a long list of nullable parameters, like productName, maxCost, minCost, producer and so on.

I refactored this method by introducing Parameter Object. I created class SearchCriteria and now method signature looks like this:

findByCriteria (SearchCriteria criteria)

I thought that instances of SearchCriteria are only created by method callers and are only used inside findByCriteria method, i.e.:

void processRequest() {
   SearchCriteria criteria = new SearchCriteria ()
                                  .withMaxCost (maxCost)
                                  .......
                                  .withProducer (producer);

   List<Product> products = productService.findByCriteria (criteria);
   ....
}

and

List<Product> findByCriteria(SearchCriteria criteria) {
    return doSmthAndReturnResult(criteria.getMaxCost(), criteria.getProducer());    
}

So I did not want to create a separate public class for SearchCriteria and put it inside ProductServiceInterface:

public interface ProductService {
    List<Product> findByCriteria (SearchCriteria criteria);

    static class SearchCriteria {
       ...
    }
}

Is there anything bad with this interface? Where whould you place SearchCriteria class?

Answer 1

I think it looks nice. It clearly signals that the SearchCriteria is intended for use with ProductServices specifically.

Some people however, would argue that nested classes look a bit odd and claim that this would be an over design and that package-scope is good enough in most cases including this.

Answer 2

It's not bad, and can be useful if you want a tighter grouping between interfaces and some utility objects, like comparators. (I've done exactly the same with an interface, and inner classes providing useful comparators that compare instances of the interface.)

it can be a little awkward for clients to use, since they must prefix the inner class name with the interface name (or use a static import), but a good IDE takes care of this for you (but the code can be peppered with Interface.SomeClass declarations, which doesn't look so nice.)

However, in the specific case, SearchCriteria looks not so tightly coupled to the interface, so it may be more usable as a regular package class.

Answer 3

I would encourage you to use classes when you have methods that may require more or less nullable arguments; it gives you the ability to provide whatever you need without having to call a method like:

someMethod("foo", null, null, null, null, null, null, ..., "bar");

Using such mecanism, the method call would be something like :

someMethod(new ObjParam().setFoo("foo").setBar("bar"));

The second method is expendable and reusable (without a tons of method overrides). And I'm not saying here that method override is bad! Quite the opposite. However with many optional arguments, I would prefer the second call.

As for inner classes, they are useful at times, but I personally follow these guidelines:

1. try to use inner classes only when the inner class should be private (ex: in the case of a custom LinkedList implementation, the Node class is a private class and is therefore an inner class.)
2. usually only if the class is not reusable and used mainly within a (very) small group of classes that I will make it an inner class
3. The "parent" and inner class becomes big enough; then both class are given their own Java source file for readability, unless the inner class should be private as for the first point.

Keep in mind that, inner class or not, the Java compiler will create a .class for every class. The more you use them, less readable your code will be. It's pretty much up to you to decide whether or not they're justified or not...

Answer 4

I'm afraid I'd like to vote for bad. Fairly bad anyway, you can do worse things...

For simplicity, a class should aim for one responsibility only. Your ProductService implementation has a criteria class definition within it, so when you wander through the code you must be aware of what part of the file you're in.

More importantly, separating makes the code of the entities involved simpler and more explicit. For me, this overrides all other concerns (ah, apart from the code being correct of course). I find simplicity & explictness are most helpful when it comes to retaining my hair, or at least that of the people who will maintain the stuff...