This is a bit of a design question involving inner classes in Java (Java 8). All of the example code is below my text
As an example, let's say I have some machinery that involves pumping fuel from an oil geyser to some sort of burner, which I can control using an external API called OilAPI.
I have a Controller class which is doing the work and decides which burner needs to get oil from which geyser, but I don't want the logic of using the API's classes like Geyser and Burner to leak out into the Controller (also since the API does still undergo some changes over time).
Now, to encapsulate it, so I create a class called FuelFacility which contains all of the logic of the OilAPI.
The thing is, I've put the classes Pump and Engine as inner classes inside of FuelFacility.
First of all, this is for the syntax of being able to go Pump.activate() instead of FuelFacility.activatePump(...) or whatever.
Further is that in order to connect a Geyser and Burner in the Oil API, you need both the Geyser and Burner objects, but I don't want to expose them externally, so in order to have some sort of "Connect Pump and Engine" method, I have to allow either the Pump to access the Engine's Burner variable, the Engine to access the Pump's Geyser +variable, or the FuelFacility to access both these variables. In the example below, I have a Engine.connectToPump(pump) method, which is basically the way it works in my actual code.
My teammates thought that this is a bit strange; they said that the accessing the private variables across the classes breaks encapsulation, and especially that a programmer looking at the code from the "outside" (i.e., from the point of view of working in the Controller class) would assume that once you have obtained the Engine and Pump objects, they would no longer depend on e.g. which OilAPI object the original FuelFacility is using (although this should remain final, as I've done below) nor on each other.
Now, since then, I've managed to change their minds a bit - that basically this is just a way of doing things that they're not used to, but that it's not bad practice.
However, now I am busy altering some other code to work in a similar fashion to this, and I just want to make sure before I continue, is what I am doing good practice? Is there a better way of doing things? Advice is appreciated!
CODE:
Oil API (not under my control):
public class OilAPI {
private final Pipes pipes = new Pipes();
public static class Geyser {}
public static class Burner {}
public static class Pipes {
public void createConnectionBetweenGeyserAndBurner(Geyser g, Burner b) {
// Connects geyser and burner
}
}
public Geyser getGeyserWithId(String id) {
// Actually retrieves a specific instance
return new Geyser();
}
public Burner getBurnerWithId(String id) {
// Actually retrieves a specific instance
return new Burner();
}
public void activateGeyser(Geyser g) {
// do stuff
}
public void activateBurner(Burner b) {
// do stuff
}
public void createConnectionBetweenGeyserAndBurner(Geyser g, Burner b) {
pipes.createConnectionBetweenGeyserAndBurner(g,b);
}
}
Fuel Facility (class I created to encapsulate the Oil API):
public class FuelFacility {
private final OilAPI oil;
FuelFacility(OilAPI oil) {
this.oil = oil;
}
public Pump getPumpForId(String id) {
OilAPI.Geyser geyser = oil.getGeyserWithId(id);
return new Pump(geyser);
}
public Engine getEngineForId(String id) {
OilAPI.Burner burner = oil.getBurnerWithId(id);
return new Engine(burner);
}
public class Pump {
private final OilAPI.Geyser geyser;
private Pump(OilAPI.Geyser geyser) {
this.geyser = geyser;
}
public void activate() {
oil.activateGeyser(geyser);
}
}
public class Engine {
private final OilAPI.Burner burner;
private Engine(OilAPI.Burner burner) {
this.burner = burner;
}
public void connectToPump(Pump pump) {
oil.createConnectionBetweenGeyserAndBurner(pump.geyser, burner);
}
public void activate() {
oil.activateBurner(burner);
}
}
}
Controller (owned by me, and sits inside of our codebase):
public class Controller {
public static void main(String[] args) {
// We actually get these from a database
String engineId = "engineId";
String pumpId = "pumpId";
OilAPI oil = new OilAPI();
FuelFacility facility = new FuelFacility(oil);
FuelFacility.Engine engine = facility.getEngineForId(engineId);
FuelFacility.Pump pump = facility.getPumpForId(pumpId);
engine.connectToPump(pump);
}
}
Having inner classes access each others' private fields isn't necessary bad in itself. It seems that your main goal is to protect Controller from changes to OilAPI. In this design, FuelFacility, Pump, and Engine are so close to OilAPI, Geyser, and Burner that I'm not sure you actually protect Controller all that much. FuelFacility should be designed more for what Controller needs than what OilAPI does. In your example, you don't call activate on the Pump or Engine but I'm assuming you'd ultimately want to do that. First, I'd start by declaring some interfaces:
public interface PumpEngineConnection {
public void activate();
}
public interface FuelFacility {
public PumpEngineConnection connect(String pumpId, String engineId);
}
Controller works through these interfaces and does not know what implementations that it actually uses. Then you can make a OilAPIFuelFacility implementation of FuelFacility. The PumpEngineConnection implementation that it returns will be one specifically designed to work with OilAPIFuelFacility. You could do this with an inner class:
public class OilAPIFuelFacility implements FuelFacility {
private final OilAPI oil;
public OilAPIFuelFacility(OilAPI oil){ this.oil = oil; }
@Override
public PumpEngineConnection connect(String pumpId, String engineId){
Geyser geyser = oil.getGeyserWithId(pumpId);
Burner burner = oil.getBurnerWithId(engineId);
oil.createConnectionBetweenGeyserAndBurner(geyser, burner);
return this.new GeyserBurnerConnection(geyser, burner);
}
private class GeyserBurnerConnection implements PumpEngineConnection {
private final Geyser geyser;
private final Burner burner;
private GeyserBurnerConnection(Geyser geyser, Burner burner){
this.geyser = geyser;
this.burner = burner;
}
@Override
public void activate() {
OilAPIFuelFacility.this.oil.activateGeyser(this.geyser);
OilAPIFuelFacility.this.oil.activateBurner(this.burner);
}
}
}
Each GeyserBurnerConnection implicitly gets a reference to the OilAPIFuelFacility that created it. That's reasonable because it only makes sense to use a PumpEngineConnection with the FuelFacility that created it. Likewise, it's perfectly reasonable for GeyserBurnerConnection to reference the oil member from OilAPIFuelFacility.
That said, it may make more sense for GeyserBurnerConnection to be a package-private class in the same package as OilAPIFuelFacility. It may be that different versions of the OilAPI can use the same GeyserBurnerConnection class.
Finally, the Controller could look something like this:
import com.example.fuelfacility.FuelFacility;
import com.example.fuelfacility.PumpEngineConnection;
public Controller {
private final FuelFacility fuelFacility;
public Controller(FuelFacility fuelFacility){
this.fuelFacility = fuelFacility;
}
public void example(){
String pumpId = "pumpId";
String engineId = "engineId";
PumpEngineConnection connection = fuelFacility.connect("pumpId", "engineId");
connection.activate();
}
}
Note that it's completely ignorant of what implementation of FuelFacility and PumpEngineConnection it actually uses. In practice, we'd pass in OilAPIFuelFacility with a dependency injection framework or an external Main class.
I realize that your example is probably simplified from what you actually need to do. Nonetheless, you should really think in terms of what Controller needs and not what OilAPI does.
Finally, I should note that I generally agree with your colleagues' concerns about your design. Consider this snippet:
OilAPI oil1 = new OilAPI();
OilAPI oil2 = new OilAPI();
FuelFacility fuel1 = new FuelFacility(oil1);
FuelFacility fuel2 = new FuelFacility(oil2);
Engine engine = fuel1.getEngineForId("engineId");
Pump pump = fuel2.getPumpForId("pumpId");
engine.connectToPump(pump);
What happens? Then oil1 is used to connect a Pump which was retrieved through oil2. Depending on the internals of OilAPI that would likely be a problem.