The comments of @SimonForsberg point to some issues in my solution:
depending on the order of enum constants
Signal output = inputs.values().stream() // Java8 functional programming
.max((s1, s2) -> s1.compareTo(s2))// depends on order in enum!
.get();
Instead of getting the "maximum" of the input signal we should check if there is any occurrence of Signal.ON in the input values and return Signal.OFF otherwise. This can also easily solved using Java8 streams:
Signal output = inputs.values().stream() // Java8 functional programming
.filter((s) -> s.equals(Signal.ON))// keeps only Signal.ON
.findFirst() // returns an Optional
.orElse(Signal.OFF);
constructor leakage
This means that other code gets access to the object being constructed before the constructor finished the initialization of the object. I'm doing this here:
class LightBulb implements SignalReceiver {
public LightBulb(SignalSource signalSource) {
signalSource.triggers(this); // leaks "this"
}
Problem is that signalSource might immediate call the method consume() defined in SignalReceiver implemented by LightBulb. There is no problem in this concrete example tough, but what if the code looks like this:
class LightBulb implements SignalReceiver {
private Signal status;
public LightBulb(SignalSource signalSource) {
signalSource.triggers(this);
status = Signal.OFF;
}
@Override
public void consume(Signal signal, SignalSource from) {
if(!status.equals(signal)) { // NullPointerException
System.out.println("light is " + signal);
status=signal;
}
}
}
This would raise a NullPointerException in case that an implementation of triggers() immediately calls consume(). Such bugs are hard to find.
The suggested solution is not to pass this but an anonymous inner class object or lambdalike this:
public LightBulb(SignalSource signalSource) {
signalSource.triggers((signal,source)->consume(signal,source));
}
But this is only half of the solution. You also must execute the call to your own method in a differed Thread so that the current thread gets time to finish the initialization:
public LightBulb(SignalSource signalSource) {
signalSource.triggers(
(signal,source)->
new Thread(()->consume(signal,source))
.start());
}
Though this will not guarantee that the NPE from my example is never raised. This is because it depends on the complexity of the code in the triggers() method, the systems load and most likely some other hidden conditions. But it drastically reduces the likelihood.
So the best way is not to call methods on constructor parameters at all especially if they require passing this.
But In our example the problem is that the LogicElement objects need to know how all of their inputs. So we have to choose from 3 evil options:
- passing
SignalSource objects to SignalReceiver constructor and register with them there risking leakage of this.
- passing
SignalSource objects to SignalReceiver constructor but register with them outside the constructor risking to forget that at the other place.
- not passing
SignalSource objects to SignalReceiver constructor and do both, register SignalReceiver with SignalSource objects and vice versa at that other place. The downside of this is that we must extend the interface of SignalReceiver with an additional method which is no needed for all implementations.
So yes: you should avoid calling methods on constructor parameters passing this to them but sometimes it is the lesser evil...
duplicated code
My approach has potentially duplicated code. I.e.: the And gate would look almost the same like the class Or. The solution to this is to create a generic class having the common behavior and injecting the differences.
Therefore I first create the class And:
class And implements LogicElement {
private final Collection<SignalReceiver> receivers = new HashSet<>();
private final Map<SignalSource, Signal> inputs = new HashMap<>();
And(SignalSource... signalSources) { // "vararg" parameter, accepts many sources
for (SignalSource signalSource : signalSources) {
signalSource.triggers(this);
inputs.put(signalSource, Signal.OFF);
}
}
@Override
public void triggers(SignalReceiver r) {
receivers.add(r);
}
@Override
public void consume(Signal signal, SignalSource from) {
inputs.put(from, signal);
Signal output = inputs.values().stream() // Java8 functional programming
.filter((s) -> s.equals(Signal.OFF))// keeps only Signal.ON
.findFirst() // returns an Optional
.orElse(Signal.ON);
for (SignalReceiver signalReceiver : receivers) {
signalReceiver.consume(output, this);
}
}
}
The only difference is the position of the constants Signal.OFF and Signal.ON which is not behavior but data. This means there should be only one class (defining the behavior) which is configured through the constructor:
class MultiInputLogic implements LogicElement {
private final Signal signalTorReturnForEmptyFilteredInput ;
private final Signal signalToFilter ;
private final Collection<SignalReceiver> receivers = new HashSet<>();
private final Map<SignalSource, Signal> inputs = new HashMap<>();
MultiInputLogic(Signal signalTorReturnForEmptyFilteredInput, Signal signalToFilter ,SignalSource... signalSources) {
this.signalTorReturnForEmptyFilteredInput = signalTorReturnForEmptyFilteredInput;
this.signalToFilter = signalToFilter;
for (SignalSource signalSource : signalSources) {
signalSource.triggers(this);
inputs.put(signalSource, Signal.OFF);
}
}
@Override
public void triggers(SignalReceiver r) {
receivers.add(r);
}
@Override
public void consume(Signal signal, SignalSource from) {
inputs.put(from, signal);
Signal output = inputs.values().stream() // Java8 functional programming
.filter((s) -> s.equals(signalToFilter))// keeps only Signal.ON
.findFirst() // returns an Optional
.orElse(signalTorReturnForEmptyFilteredInput);
for (SignalReceiver signalReceiver : receivers) {
signalReceiver.consume(output, this);
}
}
}
But this lead to another "issue" when we use this new class:
new LightBulb(new MultiInputLogic(Signal.OFF,Signal.ON, s1, s2)); // and
new LightBulb(new MultiInputLogic(Signal.ON,Signal.OFF, s1, s2)); // or
The order of the two new parameters of same type decides what function is implemented. This is called primitive obsession. To resolve this we need to create more data objects, e.g. as a new enum:
enum Logical{
And{
Signal getFilterSignal(){
return Signal.ON;
}
Signal getReturnSignal(){
return Signal.OFF;
},
OR{
Signal getFilterSignal(){
return Signal.OFF;
}
Signal getReturnSignal(){
return Signal.ON;
};
abstract Signal getFilterSignal();
abstract Signal getReturnSignal();
}
We change the constructor of our class MultiInputLogic:
MultiInputLogic(Logical function,SignalSource... signalSources) {
this.signalTorReturnForEmptyFilteredInput = function.getReturnSignal();
this.signalToFilter = function.getFilterSignal();
Then the usage is:
new LightBulb(new MultiInputLogic(Logical.AND, s1, s2)); // and
new LightBulb(new MultiInputLogic(Logical.OR, s1, s2)); // or
