3

I was developing a simple class, called Simulator, that applies a list of Simulations to a certain input. For each input the simulations can produce an output or not, based on some conditions that the input has to fulfill with respect to each simulation. The result produced by the Simulator is a list of output.

Here is the code.

class Simulator {
    final List<Simulation> simulations;

    // Some initialization code...

    List<Ouput> execute(Input input) {
        return simulations
            .stream()
            .filter(s -> s.processable(input))
            .map(s -> s.prepareOutput(input))
            .collect(Collectors.toList());
    }
}

As you can see, first I verify if an input is processable by a Simulation, filtering the simulations for which it is not, and then applying these simulations to the input.

From a object oriented point of view, I am exposing internals of Simulation class. The check operation done by the processable method should be hide inside the prepareOutput method.

However, having processable visible to Simulator, I can apply a more functional approach, which is very convenient.

Which approach is better? Is there any other solutions I am missing?

Holger
  • 243,335
  • 30
  • 362
  • 661
riccardo.cardin
  • 6,393
  • 1
  • 45
  • 77
  • 2
    if you think you are exposing internals, so why do you do it then? Some other questions to think about: What happens if I call any of your simulations with `someSimulation.prepareOutput(null)`? Is that even processible? Or what happens if I call `prepareOutput` with an input-parameter which is not processible by a given simulation? Do I need to know or check its processability from outside? Is it returning `null` or does it throw an exception? Is `Optional` an option? – Roland Mar 27 '17 at 10:52

2 Answers2

4

Since your class Simulation is already exposing the operation prepareOutput, which can fail, there is no additional exposure when you offer the method processable to detect in advance, whether the prepareOutput operation would fail. Actually, it is a good API design to offer such a check, as long as it isn’t too expensive to calculate in advance.

You may still consider offering the bulk processing operation within the Simulation class.

public class Simulation {
    public Output prepareOutput(Input input) {
        …
    }
    public static List<Output> prepareWhenPossible(List<Simulation> list, Input input) {
        return simulations.stream()
            .filter(s -> s.processable(input))
            .map(s -> s.prepareOutput(input))
            .collect(Collectors.toList());
    }
}

It’s important to make it clear to the caller that it will skip elements for which the operation isn’t possible, instead of implementing an “all or nothing” behavior.

This still doesn’t preclude exposing processable, if it is cheap to implement. It isn’t as if this was an otherwise impossible operation, as it is always possible to just call prepareOutput and drop the result to find out whether the operation is possible. Having a processable method for that purpose is much cleaner.

Holger
  • 243,335
  • 30
  • 362
  • 661
  • I think you hit the problem: if the `processable` method is available to other types, then it is important that the method `prepareOutput` invokes it internally. Under this perspective, `prepareOutput` should return an `Optional` anyway. – riccardo.cardin Mar 27 '17 at 12:35
1

If you need to hide processable, why not do it a bit different:

 Optional<Output> prepareOutput(Input input) {

      boolean isProcessable = processable(input); // processable is private

      if(isProcessable){
            // prepare Output
            return Optional.of(Output);   
      }
      return Optional.empty();

 }

And then something like this:

  List<Ouput> execute(Input input) {
    return simulations
        .stream()
        .map(s -> s.prepareOutput(input))
        .filter(Optional::isPresent)
        .map(Optional::get)
        .collect(Collectors.toList());
 }
Roland
  • 18,065
  • 1
  • 39
  • 71
Eugene
  • 102,901
  • 10
  • 149
  • 252
  • 1
    But this only hides `processable` technically, not semantically, as everyone could go and write a method `boolean processable(Simulation s, Input i) { return s.prepareOutput(i).isPresent(); }`, so all that hiding `processable` achieves, is making this test potentially less efficient. – Holger Mar 27 '17 at 11:49
  • 1
    @Holger thank you, good point. I thought its just about hiding that only method. plus one for your answer for the bulk processing.. – Eugene Mar 27 '17 at 13:07