Let’s say I have the following:
package me.my.pkg;
public interface Something {
/* ... couple of methods go here ... */
}
and:
package me.my;
import me.my.pkg.Something;
public class SomeClass implements Something {
/* ... implementation of Something goes here ... */
/* ... some more method implementations go here too ... */
}
That is, the class implementing an interface lives closer to the package hierarchy root than does the interface it implements but they both belong in the same package hierarchy.
The reason for this in the particular case I have in mind is that there is a previously-existing package that groups functionality which the Something interface logically belongs to, and the logical (as in both “the one you’d expect” and “the one where it needs to go given the current architecture”) implementation class exists previously and lives one level “up” from the logical placement of the interface. The implementing class does not logically belong anywhere under me.my.pkg.
In my particular case, the class in question implements several interfaces, but that feels like it doesn’t make any (or at least no significant) difference here.
I can’t decide if this is an acceptable pattern or not. Is it or is it not, and why?
Yes, it’s acceptable, as long as the class and the interface are both definitely in the correct packages.
Organising classes into a hierarchical set of packages is a bit like organising classes into an inheritance hierarchy. It works pretty well most of the time, but there are plenty of legitimate cases which just don’t fit.
Note that Java doesn’t really even have a package hierarchy – the hierarchical nature doesn’t extend further than the folder structure. I can’t speak for the Java language designers, but I doubt they made this decision by accident!
I sometimes find it helps to think of the ‘package hierarchy’ as an aide to help programmers find the package they’re looking for.
Although formally legitimate, this may indicate a code smell. To find out whether this is so in your case, ask yourself three questions:
- Are you gonna need it?
- Why did you package it that way?
- How do you tell if your package API is convenient to use?
Are you gonna need it?
If you don’t see the reason to invest extra effort into code maintainability, consider leaving it as is. This approach is based on YAGNI principle
programmer should not add functionality until deemed necessary… “Always implement things when you actually need them, never when you just foresee that you need them.”
Considerations provided below assume that investing effort into further analysis is justified, say you expect the code to be maintained in the long term, or are interested in practicing and honing skills to write maintainable code. If this doesn’t apply, feel free to skip the rest – because you ain’t gonna need it.
Why did you package it that way?
First thing worth noting is that neither of official coding conventions and tutorial give any guidance on that, sending strong signal that this kind decisions is expected to be at discretion of a programmer.
But if you look at the design implemented by developers of JDK, you will notice that “leaking” sub-packages API into higher level ones isn’t practiced there. For an example, look at concurrent util package and to its sub-packages – locks and atomic.
- Worth noting that using subpackages API inside is considered OK, for example ConcurrentHashMap implementation imports locks and uses ReentrantLock. It just doesn’t expose locks API as public.
Okay, core API developers seem to avoid that, and to find out why it is so ask yourself, why did you package it that way? The natural reason for that would be desire to communicate to package users some sort of hierarchy, sort of layers, so that subpackage corresponds to lower level / more specialized / narrower usage concepts.
This is often done in order to make API easier to use and understand, to help API users operate within particular layer, avoiding to bother them with concerns belonging to other layers. If this is the case, exposing lower level API from sub-packages would sure go against your intent.
- Side note, if you can’t tell why do you package it this way, consider getting back to your design and thoroughly analyzing it until you understand this. Think of it, if it’s hard to explain even to you, even now, how harder would it be to those who will maintain and use your package in the future?
How do you tell if your package API is convenient to use?
For that, I strongly recommend to write tests that exercise API provided by your package. Asking someone to review wouldn’t hurt either, but experienced API reviewer most likely will ask you to provide such tests anyway. Thing is, it is hard to beat watching a real usage example when reviewing API.
- One may look at the class with single method having single parameter and dream wow how simple, but if test to invoke it requires creation of 10 other objects, invoking like 20 methods with 50 parameters total, this breaks a dangerous illusion and reveals true complexity of the design.
Another advantage of having tests is these may greatly simplify evaluation of various ideas you consider to improve your design.
It sometimes happened to me that relatively minor implementation changes triggered major simplifications in tests, reducing amount of imports, objects, method invocations and parameters. Even prior to running tests, this serves a good indication of the way worth pursuing further.
Opposite happened to me too – that is, when large, ambitious changes in my implementations intended to simplify API were proven wrong by respective minor, insignificant impact on tests, helping me drop the fruitless ideas and leave code as is (with maybe just a comment added to help understand the background of design and help others learn about the wrong way).
For your concrete case, first thing that comes to mind is to consider changing inheritance to composition – that is, instead of making SomeClass directly implement Something, include an object implementing that interface inside the class,
package me.my;
import me.my.pkg.Something;
public class SomeClass {
private Something something = new Something() {
/* ... implementation of Something goes here ... */
}
/* ... some more method implementations go here too ... */
}
This approach may pay back in the future, preventing a risk for some subclass of SomeClass to accidentally override a method of Something, making it behave in a way that you didn’t plan for.