2
\$\begingroup\$

Some code im working with for a stock system , i have two ways to do this, looking for review on if any of them are correct and if so which one is better OO than the other

public interface Order {
    public void processed(AllocationResponse response);
}

the two solutions i have in my head are

public class Warehouse {

    public void allocate(Order order){
        AllocationResponse response = calculateAllocation(order.getQuantity());
        order.processed(response)
    }

    private AllocationResponse calculateAllocation(int quantity){
    // code
    }

}

public class AllocationResponse {

    int quantityAllocated = 0;
    int quantityBackOrdered = 0;
}

Order order = // get order
new Warehouse().allocate(order)

or

public class Warehouse {

    public AllocationResponse allocate(int quantity){
        return calculateAllocation(quantity);
    }

    private AllocationResponse calculateAllocation(int quantity){
        // code to generate response object
    }

}

Order order = // get order
AllocationResponse response = new Warehouse().allocate(order.getQuantity());
order.processed(response)
\$\endgroup\$
  • \$\begingroup\$ please add a tag to show us what language you are using - C#? Java? \$\endgroup\$ – Adam Dec 31 '11 at 9:03
  • 2
    \$\begingroup\$ "If you think you understand quantum mechanics, you don't understand quantum mechanics." --Richard Feynman. \$\endgroup\$ – Mike Nakis Dec 31 '11 at 9:12
  • \$\begingroup\$ @Mike did I miss something? The language wasn't obvious to me after looking at the code... although the use of camelCase can count as a hint.. \$\endgroup\$ – Adam Dec 31 '11 at 9:24
  • 1
    \$\begingroup\$ @codesparkle my comment was not referring to you. It was just a humorous (albeit mischievous) remark for the OP. When you do OOP right, you know that you are doing it right. If you are wondering whether you are doing it right, most probably you are not doing it right. This can usually be said without looking at the code. In any case, the code included in the post is not enough to be able to tell. What we actually need is a diagram of the design, not a listing of the code. \$\endgroup\$ – Mike Nakis Dec 31 '11 at 10:33
  • \$\begingroup\$ Impossible to answer with the information given. Creating a new warehouse is a little weird, though, if a "warehouse" is supposed to represent an actual warehouse. Unless you build a lot of warehouses. \$\endgroup\$ – Dave Newton Dec 31 '11 at 17:24
3
\$\begingroup\$

A partial answer to the partial question (with no partiality to C#, Java, or quantum mechanics):

The invariants:

  • Some unnamed caller needs to effectively call:

    order.processed(warehouse.calculateAllocation(order.getQuantity()));
    

    where calculateAllocation is private (so, presumably invisible to the caller).

[So, let's imagine the int getQuantity() method in the Order interface.]

  • The Warehouse implementation depends on AllocationResponse.

  • The unnamed caller must minimally depend on the existence of Order and Warehouse and needs some way to call calculateAllocation "for an order".

Comparison of the variants:

Option 1 pros:

  • The unnamed caller has no further dependencies beyond the above minimum.

Option 1 cons:

  • Warehouse now depends on Order, Order.getQuantity and Order.processed.

Option 2 pros:

  • Warehouse has no further dependencies.

Option 2 cons:

  • calculateAllocation is now completely exposed to abusive calling (via allocate) with not so much as an Order in sight. They might as well be one public function. Yet calculateAllocation was likely defined private for good reason -- otherwise:

    order.processed(new Warehouse().calculateAllocation(order.getQuantity()));
    

    would have been Option 0.

  • The unnamed caller now depends on Order.getQuantity, Order.processed, and AllocationResponse.

Note: The dependency on AllocationResponse could be reduced (at least in one sense) at some cost of readability (you judge?) and unique line numbers for breakpoints (typically just where and when you need one) with:

    order.processed(new Warehouse().allocate(order.getQuantity()))

Now, those are just the apparent trade-offs, but they may well be outweighed by how these design options fit in with other present and foreseeable code.

For example,

Option 1 would be more favorable in a wider system that might:

  • define Warehouse-based classes that specifically refine the behavior of allocate(order), possibly with no change or a different change to calculateAllocation(int). That is, refining allocate(order) SOLELY indirectly by refining calculateAllocation(int) would be the same deal between Options 1 and 2.

  • have many different callers of the exact same idiom:

    warehouse.allocate(order)
    
  • require other exposure to Order, Order.getQuantity and Order.processed in the Warehouse module (helping to justify/amortize the dependency cost).

Option 2 would be more favorable in a wider system that might:

  • require or desire other exposure to Order.getQuantity and Order.processed, and possibly AllocationResponse in the unnamed caller (to justify/amortize the dependency cost) other than just using the idiom:

    order.processed(warehouse.allocate(order.getQuantity()))
    

Repeated use of this idiom would argue for giving it its own method -- under Option 2, a local method of the unnamed caller, so that would be pretty much the same deal between Options 1 and 2.

In some "binary" sense, the Order interface is already imported by the unnamed caller, so the exposure to the methods is implicitly already there "for free" or more like "for better or worse". Here "binary" has BOTH its meanings of "either on or off" and "down to the compiled bits". This "binary" distinction is important when designing interfaces for libraries intended for public consumption "to be supported forever". But in a subtler practical sense, for an interface that's only used experimentally or in a small "in house" code base, isolating function calls to a small fraction of even the modules that import the interface can make it considerably less painful to "break the contract" later by evolving those functions. In this sense, calls to each distinct Order method from the unnamed caller is a "deeper" dependency than merely importing the Order interface.

  • require or desire other exposure to calculateAllocation/allocate(int) outside Warehouse. That seems unlikely. Otherwise, calculateAllocation would not have been declared private, or it would have been changed to public as a knee-jerk Option 0 solution as soon as this design problem arose, with none the wiser -- now THAT's what I'd call "doing it all wrong".
\$\endgroup\$
1
\$\begingroup\$

I love being downvoted :) There is no correct OO design for this problem. The problem cannot fit into the OO structure because it involves relationships. It isn't possible to have two abstractions such as "warehouse" and "order" interact by a method such as "place" (which places an order on a particular warehouse). This is proven so don't waste your time arguing. The issue is known as the "covariance problem".

To solve this problem properly you must de-abstract one of the entities: either the warehouse or orders must become invariant (fixed data types). You can still use information hiding but you cannot derive new instances.

In this kind of problem in the real world BOTH concepts would always be de-abstracted.

The classical solution, and the most commonly used, is known as a relational database. Here all the data structures are tables of concrete data.

That is the correct solution. Throw out the OO. It's useless here: a whole lot of mumbo jumbo religious garbage. Just use plain old data structures and the program will be done in one tenth the time, it will work, it will be easy to maintain, and it will operate quickly.

I hope it's clear: you've been lied to in School by a bunch of pseudo academics that don't know any theory and try to teach ideas that theoreticians threw out two decades ago. If you want to learn about abstraction and good programming, learn a language like Ocaml or Haskell designed by people with a good knowledge of the mathematics underlying it, not someone that could barely design a toaster that works.

\$\endgroup\$
0
\$\begingroup\$

My preference:

public class Warehouse {

    public void allocate(int quantity){
         // code
    }
}

public class Order {

    public void process(Warehouse warehouse)
    {
         processed(warehouse.allocate(getQuantity());
    }
}

Basically, you need to call:

order.processed(warehouse.allocate(order.getQuantity()))

I suggest that this belong in order.

Why?

  1. Tell Don't Ask: It is preferable to ask Order to process rather then extracting the information and then processing it somewhere else
  2. Coupling: Two of the three calls are to order. This suggests that the code is coupled to order and probably belongs in Order.
  3. Minimize interface: The processed and getQuantity() functions become internal to Order (as long as they aren't used elsewhere), thereby simplifying the interface of those objects.

Possible issues:

  1. Order may already be too complicated, adding process(Wharehouse) may bloat that. I'd suggest that is probably a better way to split that code.
\$\endgroup\$
0
\$\begingroup\$

Both methods look too convoluted.

I guess that your problem is that the warehouse sometimes miss some capacity to fulfill the full order and can only partially fill the order.

I would decouple your code:

I guess you have the clients calling a submit(order) method somewhere, but you should have them check the capacity first, e.g. wareHouse.checkCapacity() (and maybe obtain a lock on some specified capacity if many clients are placing orders concurrently). It's important that your interface should let the clients know what is going on instead of just returning a partial order. The client code should decide what to do when there is not enough capacity; in both your solutions it's a mismatch between the warehouse and the order.

\$\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.