1

Here's an analogy of our concrete problem to demonstrate the issue at hand.

We need to manufacture cars, with either petrol or diesel related parts (the parts can be the engine and the exhaust - e.g. different requirements for exhaust for diesel engines).

Engine
+ engineCapacity: Integer

DieselEngine: Engine

PetrolEngine: Engine
+ octanes (Integer)

We then have the car manufacturer interface:

public interface CarManufacturer {
    Engine createEngine(Integer engineCapacity);

    Exhaust createExhaust(Engine engine);
}

and its implementors

DieselCarManufacturer
PetrolCarManufacturer

There's the CarManufacturerGateway that orchestrates the car building process, selecting a CarManufacturer from a ManufacturersStore according to some criteria and then building a car via:

final var engine = carManufacturer.createEngine(engineCapacity);
final var exhaust = carManufacturer.createExhaust(engine);

Problem definition
To create the proper exhaust, in the PetrolCarManufacturer, we need to use the petrol engine's octanes number (probably a bad analogy) from within the PetrolCarManufacturer::createExhaust.

But, the CarManufacturer interface accepts an Engine, thus we cannot access the petrol engine octanes to decide upon the exhaust configuration to create.

Solutions

  1. instanceof + downcasting: I think it's considered a bad practice and can fail during runtime
  2. using double dispatch somehow?
  3. using parameterized types on the interface, e.g. public interface CarManufacturer<T extends Engine> and then each implementor will define the concrete type of engine it makes. This also requires the ManufacturersStore to be parameterized in the same way: ManufacturersStore<T extends Engine>. The latter makes me question this option, as it reads weirdly "a car manufacturer store parameterized on the type of engines it makes".

N.B.: If the analogy fails to demonstrate the issue at hand, I can try to rewrite it with our concrete example.

Improve this question
2
  • It really depends. Casting should be a last resort. Can the "get octane" method be moved to the Engine base class (with a diesel engine returning a nonsense value like 0 or throwing an exception)? Could createExhaust ask the engine for what type (or types) of exhaust would be appropriate? Commented Sep 13, 2021 at 14:27
  • @1201ProgramAlarm I'd like to avoid "littering" the Engine interface with a detail that only makes sense for one part of the abstraction (certainly the analogy has failed me here). Commented Sep 13, 2021 at 14:44

3 Answers 3

Reset to default
2

The problem here is that you've separated your concerns so much that you're unable to now implement a clear connection between the two. The engine and exhaust creation logic is so separated that you've forgotten what kind of engine you're working with when you get to the stage of creating the exhaust.

There are two possible scenarios here: you either need that separation, or you don't. I can't judge your scenario to that level of detail. This answer tries to answer both cases as best as it can.

instanceof + downcasting: I think it's considered a bad practice and can fail during runtime

Quick thought: your question implies that the right engine type must go with the right exhaust. So if you pass the wrong engine to the wrong exhaust factory, the logical consequence is that it should fail, specifically to honor your wishes that the right engine type must go with the right exhaust.

I agree that wanton downcasting is a sign of bad OOP, and leads to LSP violations. If you don't need the separation between the engine and exhaust creation logic, then it's best to couple them somewhat tighter so you don't lose track of the concrete engine type until you've also created your exhaust.

However, there are reasonable exceptions to this. For example, try catch uses downcasting in order to figure out if a thrown exception is of a given type or not. And this was necessary because the exception throwing logic needed to be as generic as it possibly could be.
To avoid upcasting in a try catch, you would need to strongly type your throw logic, which means having to custom implement it for your custom exceptions, which is not what you'd want. The alternative is pre-emptive upcasting to the base Exception type and then figuring out the concrete type at a later stage.

If you truly need the separation between the engine and exhaust, then it's acceptable to use downcasting here, and accept that when the wrong kind of engine is passed to the wrong kind of exhaust creator, that you run into an intentional exception there.
By having implemented that separation, you've signed yourself up to handle the concrete types yourself (since the typing system can no longer help you), and if you fail to handle it correctly and have no way to elegantly back out of the process, then an exception is the only option left on the table.

In this scenario, something did go awry in your application, since it's clearly trying to match an engine to the wrong exhaust creator. So if you take off the safeties (strong typing) and then don't handle things yourself, then a crash is inevitable and the right thing to have happen.

So, to summarize:

  • If you need (or cannot avoid) the loss of strong typing inbetween engine and exhaust creation, then downcasting is a valid solution to the problem.
  • Without that specific need, it is better to rewrite your code so that you don't forget the concrete type of the engine between its creation and the exhaust's creation.
  • Exceptions are not the devil. When you reach a bad situation, they are the correct response, to very clearly report to the developer that something major has gone wrong (in this case, an engine/exhaust creator mismatch)
Improve this answer
2
  • I totally agree with your approach. I was also thinking about whether I need this separation or not and to be honest, I haven't found a reason for doing so. It seems like having these two functions is on the wrong level of abstraction! That's why we're missing the strong typing. I am now going towards creating the car at the level of the concrete CarManufacturer and having the separate parts created in that abstraction level (where concrete type information is available). Thanks a lot for your answer! Quite informative and addressing the issue at its origin. Commented Sep 15, 2021 at 9:41
  • @Chris: I think you're going in the right direction with this. Glad I could help. Commented Sep 15, 2021 at 10:01
2

One option would be to introduce another layer of abstraction

public interface CarManufacturer {
    PartFactory createPartFactory(Integer engineCapacity);
}
public interface PartFactory{
    Engine getEngine();
    Exhaust getExhaust();
}

This should guarantee that the exhaust matches the engine. The downside is more interfaces and a bit more complexity.

I do not see how double dispatch would solve this problem, while you avoid the downcast, you still need to handle the case where a diesel engine is passed to a petrolManufacturer, and this would still be a runtime error.

While I agree that runtime errors should be generally avoided, the overall goal is to make it as easy to use as possible. In some cases that might very well be done by clear documentation and runtime error messages rather than by designing overly complicated interfaces.

I have used some APIs where just about every method started with something like if(obj is not SomeInternalType) throw "invalid object type, object has to be created by CreateObject method" without any real issue. After all, it is much easier to call the CreateObject method provided by the API than to create my own implementation of the interface.

Improve this answer
3
  • If one CarManufacturer implementation, is used throughout the process, e.g. PetrolCarManufacturer, it creates only the appropriate Engine and Exhaust pair. The problem I'm facing is how can I retrieve via the PetrolCarManufacturer::createExhaust the octanes number from the PetrolEngine despite the fact that createExhaust accepts an Engine argument. To fix that I would have to downcast the engine to PetrolEngine. Commented Sep 14, 2021 at 7:00
  • @Chris Yes, but you can avoid that problem by creating the engine and exhaust at the same time, or create some other interface that already know what engine is selected, so that it does not need to take it as an argument. Commented Sep 14, 2021 at 7:39
  • Tha'ts a good point, it will avoid the issue of having a different type of engine used to create an exhaust. Thank you! Commented Sep 15, 2021 at 9:37
1

How does the PetrolCarManufacturer know what octane number to give to the PetrolEngine it creates? The octane number is not part of the CarManufacturer interface, so the PetrolCarManufacturer class needs to have the knowledge in some other way (member variable, read from some configuration object, etc.).

As the PetrolCarManufacturer already needs to have knowledge of the octane number it uses to create PetrolEngine objects, it can use that exact same knowledge within the createExhaust method.

The assumption here is that a single PetrolCarManufacturer can only create Engines with all the same octane number and that all parts that need to go together are created by the same factory.

Improve this answer
1
  • You're right Bart! The difficult part is that the PetrolCarManufacturer uses an external system to actually create the PetrolEngine. We could have stored the ID of the PetrolEngine from the external system (to query about the PetrolEngine and retrieve the octanes), inside the Engine but that doesn't seem clean, right? Concluding, from within PetrolCarManufacturer::createExhaust, I need to have access to the runtime type of Engine, i.e., PetrolEngine. Could this be a design smell? I could combine both createEngine and createExhaust in a single method. Commented Sep 14, 2021 at 7:06

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.