I recently came across two interesting methods that were part of a bigger class that I had to redesign:
class OrderLine
{
/**
* @var float
*/
private $quantityOrdered;
// ...
/**
* @param float $quantity
*/
public function processDelivery($quantity)
{
$this->quantityDelivered += $quantity;
$this->quantityOpen = $this->quantityOrdered - $quantity;
if ($this->quantityOpen < 0) {
$this->quantityOpen = 0;
}
}
/**
* @param float $quantity
*/
public function undoDelivery($quantity)
{
$this->quantityDelivered -= $quantity;
if ($this->quantityDelivered < 0) {
$this->quantityDelivered = 0;
}
$this->quantityOpen += $quantity;
if ($this->quantityOpen > $this->quantityOrdered) {
$this->quantityOpen = $this->quantityOrdered;
}
}
}
Of course, I've already cleaned up the code here to allow you to better understand it.
What happens here is: we have an order line, which keeps track how much of a certain product has been "ordered", and then how much of it has been "delivered" so far. It also keeps track of how much is currently still "open". Changes to these "delivered" and "open" quantities happens when we "process" a delivery, or "undo" a delivery.
I was reminded of a recent blog post by Nicolò Pignatelli where he quoted a question from another programming website. Adopted to the situation at hand:
Which variable type would you use for representing a quantity?
[ ] Integer
[ ] Float
[ ] String
It's a trick question, because all the answers are wrong. Nicolò advises not to use a primitive type value, but to design a value object that can represent a quantity. Nevertheless, in this code base it's floats everywhere.
I'm still modified-copycat-ing Nicolò's blog post here, since he's asking some really good questions that can guide your thinking about value objects:
- Does it make sense to add or subtract two [quantities]? Maybe.
- Does it make sense to multiply or divide two [quantities]. Don’t think so.
- Does it make sense to allow negative [quantities]? Probably not.
You have to ask yourself these questions, since these properties would hold for a float
, but not for quantities. Quantities can (sometimes) be added, but in the domain of selling and stock they should certainly not be divided (maybe multiplied against a tariff of some sorts, but then the result is not a quantity anymore). It also makes no sense to order a negative quantity, since that would be more like giving a quantity away.
This exposes quite nicely the need for modelling; the need for "codifying" the knowledge we have about quantities in our domain.
We may start out by wrapping these floats in a simple object called Quantity
:
final class Quantity
{
/**
* @var float
*/
private $quantity;
public function __construct(float $quantity)
{
$this->quantity = $quantity;
}
}
By the way, I find that it really helps to write unit tests along the way. You don't have to do pure TDD, but at least make tests part of your workflow. It'll be great, since every unit test method can help you document every one of your design criteria or decisions.
It's also a good idea to not just start re-implementing the algorithm you find in existing code, like the code above. Think about what's going on. Follow your intuition. For example, those if
statements that end up "capping" certain quantities (to 0, or to the ordered quantity), are kind of iffy (pun intended).
Unraveling the meaning of these different quantities by thinking about them, and remembering something the domain expert said, I realized that:
- These quantities are related.
- These quantities aren't all of the same kind.
- The
if
statements hide some important domain knowledge.
The open quantity is what remains to be delivered. So it's the result of subtracting all the quantities that have been delivered from the initially ordered quantity. However, this doesn't take into consideration that we can "over-deliver", that is, we can deliver more than was ordered, and that's perfectly fine. That's why we need to cap the open quantity to 0: if we over-deliver, there's just nothing more that needs to be delivered, and we don't need to give something back. This explains the first if
statement.
The other way around, if we undo a delivery, we'd have to increase the quantity open by the quantity of this particular delivery. However, even if we have been over-delivering in the past, we should never end up with an open quantity that is more than the quantity that was originally ordered. Over-delivering doesn't owe us anything. This explains the second if
statement.
Regarding valid ranges for the quantities involved, we should note that, taking all the above into consideration:
- The quantity so far delivered can be 0 or more, with no upper bound, since we can over-deliver as much as we want.
- The quantity that remains open can be 0 or more, up to the quantity that was initially ordered.
- The delivery quantity itself, which we either process or undo, should be more than 0 (we wouldn't want to bother with deliveries of 0), with no upper bound (again, because we can over-deliver).
Furthermore, we can't just add and subtract any of these quantities (like we can with floats):
- We can add/subtract delivery quantities from either delivered or open quantities.
- We can add/subtract delivery quantities from themselves (although at the moment, there's no valid use case for that).
So no adding/subtracting of open quantities, delivered quantities, etc.
The cool thing about value objects is that we can enforce all of these rules in code. With floats, you can do all kinds of invalid things, and you need to have all those if
statements outside of them to verify that you still end up with the correct values.
Let's start with four classes, each to wrap a single float. There's more to say about quantities-stored-as-floats (it's not ideal), but for now let's accept the float as the base type.
use Assert\Assertion;
final class DeliveredQuantity
{
private $quantity;
public function __construct(float $quantity)
{
Assertion::greaterOrEqualThan($quantity, 0);
$this->quantity = $quantity;
}
}
There will be three other classes, DeliveryQuantity
, OpenQuantity
, OrderedQuantity
. Each with different assertions in the constructor, to make sure we don't provide nonsensical values to it.
What's most interesting is what we can do with these different quantities. For example, as explained earlier, the quantity so far delivered can be increased or decreased when processing or undoing a delivery respectively. When subtracting the delivery quantity, we need to take care of setting the delivered quantity back to 0, if it were to go below 0. It's nice to have this if
statement inside the class. We can't accidentally forget it, because the object will always take care of it.
final class DeliveredQuantity
{
// ...
public function add(DeliveryQuantity $other): DeliveredQuantity
{
return new DeliveredQuantity(
$this->quantity + $other->asFloat()
);
}
public function subtract(DeliveryQuantity $other): DeliveredQuantity
{
$remainingQuantity = $this->quantity - $other->asFloat();
if ($remainingQuantity < 0) {
$remainingQuantity = 0;
}
return new DeliveredQuantity($remainingQuantity);
}
}
Upon each operation (add()
, subtract
()) we make sure to create a new instance of the value object, wrapping the new value, instead of simply modifying the existing value object. That way, we won't have any surprising side-effects in objects that keep a reference to the existing value object.
Having these different quantities represented by their own classes, helps make the relations between them more visible. For example, as I mentioned before, the "quantity open" will be the quantity ordered, minus the quantity delivered, but never less than 0:
final class OrderedQuantity
{
// ...
public function calculateQuantityOpen(DeliveredQuantity $deliveredQuantity): OpenQuantity
{
$quantityOpen = $this->quantity - $deliveredQuantity->asFloat();
if ($quantityOpen < 0) {
$quantityOpen = 0.0;
}
return OpenQuantity::fromFloat($quantityOpen);
}
}
All the types are in the right place: we can ask OrderedQuantity
what its OpenQuantity
will be, given a DeliveredQuantity
. This is what I mean by the relation between these different types of quantities: the types make this relation explicit.
Closing remarks
When I first started reworking this piece code, I was inclined to introduce a Quantity
value object, so I could get rid of the floats right away. But my initial approach was too generic. It turned out, these are really all different quantities, and it's impossible to model them all as one type. For modelling in general, this is a nice guideline: don't make things generic from the start. Start with duplicating then modifying the code for all the different concepts, and only generalize later on, if you really have to. In most cases, you don't really need the generic solution, you just need a way to reuse part of the code. I find that sometimes a trait can be useful for that.
Just one last comment: if you take a look at the original code and how it deals with the open quantity for an order line, it turns out that it manipulates an object property for that. It subtracts a delivery quantity and if it ends up below 0, it will set it back to 0. The funny thing is, with the new setup using value objects, we don't directly manipulate the quantity open anymore: it has become a derived value, returned by calculateQuantityOpen()
. Turning a manually updated value into a derived value is a powerful refactoring, which lowers the maintenance burden a lot: both reading and updating the code will become much easier with one variable less to keep track off.
Why don't use integer for quantity?
Indeed, that would be the proposed solution: an integer for the quantity, and another integer for the precision. For example, 150 with a precision of 2 actually means 1.5.
Just curious , i notice the use of
Assertion::greaterOrEqualThan($quantity, 0);
.Is this an external library? Would you use this in production environments? Also why do you prefer this over a if statement with an InvalidArgumentException for example?There is also https://github.com/webmozar...
Yeah, I go back and forth between that. I didn't think a lot about this statement when I wrote it to be honest. It's more like a convenient shortcut here. But I believe that having an
InvalidArgumentException
, or even a more "domain-specific" exception often makes more sense. In fact, I often create such exceptions myself, which is helpful when unit testing (you can expect a more specific exception type thanInvalidArgumentException
).I saw @mbadolato:disqus had already answered your other question. Also, yes, these assertions and their exceptions are there in production too. I find it's very helpful actually, to expose subtle issues. And the reality is: these exceptions won't usually be thrown because all the bad data has been filtered out already at the point when you call the method/construct the object. In the case of this article for example, there should have been (UI-level or controller-level) validation in place to prevent entering a negative quantity.
It's is the https://github.com/beberlei... library and contains a bunch of useful assertions to use.
It does throw an InvalidArgumentException if the assertion doesn't hold true, so you're essentially getting a bunch of pre-made if statements, with clear intentions defined via the assertion name.