7
\$\begingroup\$

I am implementing a framework for validating orders. User creates a number of orders (Amazon orders for example), before he submits the orders and pays for shipment, whenever he revises anything, I need to run a couple of checkers against the orders and show corresponding warnings and errors produced by the checkers.

The Requirement: There will be a lot of predefined checkers. But the checkers applied to a particular purchase will be only a small subset, and will vary according to, for example, who the user is, what he is purchasing, where he is going to ship the items to, etc. The parameter that required for checkers to run is also configurable and provided externally

The Design: It appears to me that the requirement fits well into a strategy pattern, where the checkers are the strategies. The framework communicates with external configuration server by checkers' name.

To communicate the name with configuration server a bit more reliably, I decided to model the checker using enum so as to avoid explicitly introducing a string name property to every strategy.

The code of each checker can be possibly a lot of lines, so I introduce a Rule class to represent the actual checking logic. The checker therefore becomes a wrapper of Rule.

To separate the logic of warning/error handling and the logic of the Rule itself, I am trying to make Rule stateless. And I add RuleVisitor to redirect the Rule result to a visitor

Below is my code. Is it too complicated for the problem I am solving? Is it well-written? Any comment is welcomed.

//A very simple model of Order, I will not include any implementation here
public interface Order {
    String orderID ();
    String item ();
    int quantity ();
    double price ();
    String shipmentAddress ();
}

/**
 * A Rule is the actual logic of a checker.
 * It runs against the given Order, using the given RuleParameter 
 * and reports its result to RuleVisitor 
 */
public interface Rule {
    void run (Order order, RuleParameter parameter, RuleVisitor visitor);
}

//The parameter of a Rule that is provided from external configuration
public interface RuleParameter {
    double warnLimit ();
    double errorLimit ();
}

public interface RuleVisitor {
    void error (String reason);
    void warning (String reason);
    void allGood ();
}

/**
 * Context is the proxy between the validation framework and 
 * the outside world. The major responsibility is to capture
 * all the information required and show it to user
 */
public interface Context {
    void warn (String orderID, Checker checker, String reason);
    void error (String orderID, Checker checker, String reason);
    void allGood (String orderID, Checker checker);
}

/**
 * The main class to layout the validation flow
 */
public class OrderManager {
    private final Context context = new Context () {
        @Override
        public void warn(String orderID, Checker checker, String reason) {
            //There will be a lot of stuff in reality, building warning object,
            //rendering, caching warning/errors etc.
            System.out.println(String.format (
                "%s has raised a warning for order %s. Reason: %s", 
                checker, orderID, reason));
        }
        @Override
        public void error(String orderID, Checker checker, String reason) { }
        @Override
        public void allGood(String orderID, Checker checker) { }
    };

    public void process (Collection<Order> orders) {
        for (Order order : orders) {
            Collection<Checker> checkers = getEnabledCheckers (order);
            for (Checker checker : checkers) {
                RuleParameter parameter = getParameter (order, checker);
                checker.check(order, parameter, context);
            }
        }
    }

    public Collection<Checker> getEnabledCheckers (Order order) {
        //the list of checker names applicable, should come from config
        Collection <String> enabledCheckerNames =
            Arrays.asList("OrderValueChecker", "QuantityChecker");
        Collection <Checker> checkers = new ArrayList<> ();
        for (String name : enabledCheckerNames) {
            checkers.add(Checker.valueOf(name));
        }
        return checkers;
    }

    public RuleParameter getParameter (Order order, Checker checker) {
        //again, should be coming from external configuration
        return null;
    }
}

/**
 * The definition of all available checkers. A Checker
 * is a warp of a Rule instance. The actually validation 
 * logic is in the Rule instead of Checker. 
 * Checker is defined to name the available Rules in a 
 * cleaner manner
 */
public enum Checker {
    OrderValueChecker (new OrderValueRule ()),
    QuantityChecker (new QuantityRule ());

    private final Rule rule;

    Checker (Rule rule) {
        this.rule = rule;
    }
    public void check(final Order order, final RuleParameter parameter, final Context context) {
        //visitor is to direct the Rule result to the given
        //context with enriched information, such as Checker and orderID
        RuleVisitor visitor = new RuleVisitor () {
            @Override
            public void error(String reason) {
                context.error(order.orderID(), Checker.this, reason);
            }
            @Override
            public void warning(String reason) {
                context.warn(order.orderID(), Checker.this, reason);
            }
            @Override
            public void allGood() {
                context.allGood(order.orderID(), Checker.this);
            }
        };

        rule.run(order, parameter, visitor);
    }
}

/**
 * A demo implementation of a Rule to examine if an order is too valuable.
 * If it is a very valuable order, warn the user to add an insurance
 * If it is an extremely valuable order, block the user from placing it
 */ 
public class OrderValueRule implements Rule {
    @Override
    public void run (Order order, RuleParameter parameter, RuleVisitor visitor) {
        double value = order.quantity() * order.price ();

        final double limitForError = parameter.errorLimit();
        if (value > limitForError) {
            visitor.error(String.format(errorReasonTemplate, limitForError));
            return;
        }

        final double limitForWarning = parameter.warnLimit();
        if (value > limitForWarning) {
            visitor.warning(String.format(warningReasonTemplate, limitForWarning));
            return;
        }

        visitor.allGood();
    }

    private final String errorReasonTemplate = "Order of value larger than %f is not allowed";
    private final String warningReasonTemplate = 
    "Order value is larger than %f. Please consider adding an insurance for shipment";
}

public class QuantityRule implements Rule {
    //implementation omitted
    ...
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to Code Review! Looks like an interesting first question, with good context provided. I hope you get some good reviews! \$\endgroup\$
    – Phrancis
    Dec 18 '14 at 16:06
1
\$\begingroup\$

Below is my code. Is it too complicated for the problem I am solving?

It looks good, but I'm afraid, it's really a bit overcomplicated. For me, starting with a complicated design is always worse than starting with an oversimplified one, but YMMV. However, the structure is fine, I can imagine having fun working with it.

double warnLimit ();

You surely know that the space before the opening paren is not Java standard. Your choice.

public Collection<Checker> getEnabledCheckers (Order order) {
    //the list of checker names applicable, should come from config
    Collection <String> enabledCheckerNames =
        Arrays.asList("OrderValueChecker", "QuantityChecker");

In case it does not come from config, you should start with a list of enums rather than strings. Alternatively, every enum could say if it applies to a given order.

Or the order could have a type (Amazon, etc.) and you'd check the type (as the first or maybe the only criterion) in order to determine applicability.

public enum Checker {
    OrderValueChecker (new OrderValueRule ()),

This is one of two parts I dislike. Enum can have instance-specific methods, and I'd try to put some logic there. Even when the whole logic gets too complicated, you gave up IMHO too early. Recently, I wrote a simple enum-based checker and with a couple of helper methods everything fits in without the enum becoming too big.

public interface Context {
    void warn (String orderID, Checker checker, String reason);
    void error (String orderID, Checker checker, String reason);
    void allGood (String orderID, Checker checker);
}

Why do you pass the orderID instead of the order?

public interface RuleVisitor {
    void error (String reason);
    void warning (String reason);
    void allGood ();
}

This is just a stripped down version of Context. Maybe it's be needed, but probably only because of the distinction between Checker and Rule. I guess, you could have in class Checker something like

public abstract void check(final Order order, final RuleParameter parameter, final Context context);

While I love early returns and dislike repetitions, I'd replace

    final double limitForError = parameter.errorLimit();
    if (value > limitForError) {
        visitor.error(String.format(errorReasonTemplate, limitForError));
        return;
    }

by something like

    if (value > parameter.errorLimit()) {
        visitor.error(String.format(errorReasonTemplate, parameter.errorLimit()));
    } else if ...

The visitor could do the formatting itself, if you need it often.

All that said, it's nice. All I wrote are just hints; no warnings, no errors found. ;)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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