2
\$\begingroup\$

In this dummy structure, we have a race car manager class. this class contains a method named execute(). There is a List which contains all participating racing cars.

This is the Car class, the List's type is based off:

public Car {

    private boolean driverBeltOn;
    private boolean engineOn;

    public boolean engineReady() {
        return this.engineOn;
    }

    public boolean driverBeltOn() {
        return this.driverBeltOn;
    }

    public void warn(String message) {
        // Send warning to car
        // ....
        // .....  (<;D>-<;
        // ......
    }
}

The job of the execute method, is to loop through all cars, and make sure they are ready for the race, but if at-least one of the two conditions is false, then the car will not participate in the race.

If one condition is false, then the car should be notified, about why it can't participate.

Basic solution:

public void execute() {
    for (Car car : carList) {
        if (car.engineOn()) {
            if (car.driverBeltOn()) {
                // Can go..
            }
            else {
                car.warn("Please wear a damn belt!");
            }
        }
        else {
            car.warn("How are you planning to participate in a race with an off engine??");
        }
    }
}

What is the best way to handle things like this, where you have to check multiple conditions, and each condition should do something if it's false in the check?

Example of a bad solution, because it's impossible to let the car know what's wrong:

public void execute() {
    for (Car car : carList) {
        if (car.engineOn() && car.driverBeltOn()) {
            // Can go..
        }
        else {
            car.warn("Something went wrong...");
        }
    }
}

Unless you add multiple checks inside the else block, which could be double work/bad practice.

What is the best practice do handle this? Or if inside if for this case is totally fine?

\$\endgroup\$
3
  • \$\begingroup\$ Even though this apparently is reviewable, please make sure that your code is compilable next time. Your driverBeltOn method is not correct. \$\endgroup\$ Commented Sep 4, 2014 at 23:42
  • \$\begingroup\$ @SimonAndréForsberg A whole return keyword...fixed for you. \$\endgroup\$
    – Trent
    Commented Sep 5, 2014 at 2:41
  • 1
    \$\begingroup\$ Just so it's said...if a car's current state can make it undriveable, then it should be checking its state (and generally throwing an exception if it's in an invalid state) when you try to drive it. That seems an answer to a whole different question, though. \$\endgroup\$
    – cHao
    Commented Sep 5, 2014 at 4:48

5 Answers 5

6
\$\begingroup\$

If you don't want to return early (because you want to go through all checks to warn about all problems), then you can use a flag to keep track of any problems:

for (Car car : carList) {
    boolean goodToGo = true; 
    if (!car.engineOn()) {
        car.warn("How are you planning to participate in a race with an off engine??");
        goodToGo = false;
    }
    if (!car.driverBeltOn()) {
        car.warn("Please wear a damn belt!");
        goodToGo = false;
    }
    if (goodToGo) {
        // Vroooomm....
    }
}

Btw, I guess you forgot a return statement here:

public boolean driverBeltOn() {
    this.driverBeltOn;
}

And I recommend to rename your methods to isDriverBeltOn and isEngineReady, as it's a fairly common practice to prefix boolean getters with "is...".

\$\endgroup\$
3
\$\begingroup\$

There is one option you apparently have not considered: Return early.

for (Car car : carList) {
    if (!car.engineOn()) {
        car.warn("How are you planning to participate in a race with an off engine??");
        continue;
    }
    if (!car.driverBeltOn()) {
        car.warn("Please wear a damn belt!");
        continue;
    }
    // Can go..
}

I like this method better because it reduces indentation, and handles the edge-cases first.

The code to handle the edge-cases are normally much smaller than the // Can go.. part so by doing this you get a better overview of your method.

Note however that this is primarily my opinion, but it is something I have seen often and something that has been said often in other reviews.

\$\endgroup\$
2
  • \$\begingroup\$ You could also change the continues for if/else if and avoid the early returns if that makes someone unhappy. \$\endgroup\$
    – Juanma
    Commented Sep 5, 2014 at 12:26
  • \$\begingroup\$ Depends on the reaction, really. If that someone starts twitching and mumbling about "goto in disguise", it might be more entertaining to keep them. \$\endgroup\$
    – cHao
    Commented Sep 6, 2014 at 0:34
2
\$\begingroup\$

For completion's sake, I'm going to offer something different, provided your use case warrants the extra complexity:

Formatter fmtWarn;
/**
 * Writes a formatted message to {@link #fmtWarn} when <var>condition</var> is true.
 * @return <var>condition</var>
 */
boolean warn(boolean condition, String format, Object... os) {
  if ( condition ) {
    fmtWarn.format(format, os);
  }
  return condition;
}

public void execute() {
  for ( Car car : carList ) {
    if ( isReady(car) ) {
      // ...
    }
  }
}

boolean isReady(Car car) {
  /* Note the "bit-wise or" - this makes us check all conditions regardless of individiual results. */
  return warn(!car.engineOn(), "How are you planning to participate in a race with an off engine??") | 
         warn(!car.driverBeltOn(), "Please wear a damn belt!") |
         // ...
         ;
}

You could take it a step further and define a Requirement class that checks a predicate and takes a callback function for failure, and so on, but at that point you have to wonder how much effort it takes to build the pneumatic hammer, and how much to just hammer in the nails with that rock there.

\$\endgroup\$
1
  • \$\begingroup\$ A Requirement class? Now we're talking! :) \$\endgroup\$ Commented Sep 6, 2014 at 0:48
1
\$\begingroup\$

Let the car class decide if it is ready, not the external class! So, you get:

interface Drivable {
    boolean isReady();
}

class Car implements Drivable {
    private boolean engineReady = false;

    // ... more properties

    public boolean isReady() { 
        return engineReady; // && otherProperty1 && otherProperty2 
    }

}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ But by using &= in setEngineReady() it can change drivable from true to false but not from false to true. \$\endgroup\$ Commented Sep 5, 2014 at 5:18
  • \$\begingroup\$ Good point, I did not think this through. Yeah, there is no way to avoid checking all of the state variables. Even if drivable is initially set to true, and then potentially unset by setEngineReady it can never be turned back on with a simple and operation: it needs to AND all of the state variables. There is another solution, but it is not very idiomatic of Java: drivable can be an integer and each property can set a bit in the drivable int. Then isReady() returns true if and only if the drivable variable is equal to a pre-determined number. \$\endgroup\$ Commented Sep 5, 2014 at 5:26
  • 1
    \$\begingroup\$ An integer solution that you talk about can be done using an EnumMap<DrivableRequired> where DrivableRequired is an enum of ENGINE, BELT etc... \$\endgroup\$ Commented Sep 5, 2014 at 10:29
  • \$\begingroup\$ A car can only be ready if it's engine is on, and a car generally doesn't care too much if you have your safety belt on. The rules (be it track rules, or road rules) require a safety belt. So although you are correct that the car should decide if it is ready or not it doesn't always mean that there are other conditions that would make a car not able to go \$\endgroup\$ Commented Sep 6, 2014 at 3:44
0
\$\begingroup\$

I completely agree with @Nich Hristov that your design is upside down and the car should decide if it is ready.

Having said that, here is another solution:

public interface Validator<T> {

    /**
     * @return one or many error messages if it fails validation, and the
     * empty list if it is valid.
     */
    public List<String> validate(T t);
}

public static class CompositeValidator<T> implements Validator<T> {
    private final List<Validator<T>> innerValidators;

    public CompositeValidator(List<Validator<T>> innerValidators) {
        this.innerValidators = innerValidators;
    }

    @Override
    public List<String> validate(T t) {
        return innerValidators.stream()
                .flatMap(validator -> validator.validate(t).stream())
                .collect(Collectors.toList());
    }
}

public static class DriverBeltValidator implements Validator<Car> {

    @Override
    public List<String> validate(Car car) {
        return (car.isDriverBeltOn()) ? Collections.emptyList() : Arrays.asList("...belt");
    }
}

Continuing with the same interface, but a bit more fancy:

public static class PredicateValidator<T> implements Validator<T> {
    private final Predicate<T> predicate;
    private final List<String> messages;

    public PredicateValidator(Predicate<T> predicate, List<String> messages) {
        this.predicate = predicate;
        this.messages = messages;
    }

    public PredicateValidator(Predicate<T> predicate, String message) {
        this(predicate, Arrays.asList(message));
    }

    @Override
    public List<String> validate(T t) {
        return predicate.test(t) ? Collections.emptyList() : messages;
    }
}

private static Validator<Car> carValidator = new CompositeValidator<>(Arrays.asList(
        new PredicateValidator<>(Car::isDriverBeltOn, "...belt"),
        new PredicateValidator<>(Car::isEngineReady, "...engine")));

private static void validateAndWarnCars(Collection<Car> cars) {
    for (Car car : cars) {
        List<String> warnings = carValidator.validate(car);
        warnings.forEach(warning -> car.warn(warning));
    }
}

After having written that, I realized that maybe you don't care so much about the error messages, but only about the actions to take when something is wrong. If you really only care about the actions, then you can change List<String> to List<Runnable> everywhere above and instead of returning the list, you just execute the Runnables (not in a different thread). Even better would be to generalize the interface further:

public interface Validator<T, S> {

    public List<S> validate(T t);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.