Hidden conditions

One way of making code easy to understand and reason about is to try to ensure that it does exactly what you expect it to do. Sometimes however, there are conditions secretly hidden in places of the code that leaves you scratching your head and forces you into a quest for answers down the deepest of rabbit holes. For example, let’s assume that the application that you develop handles purchase orders. When you scroll through the code you see a call to a method named cancel that is part of the PurchaseOrder class. The natural thing to assume is that calling this function cancels the order, since that is exactly what the name says. However, things looks a bit odd since the call is made from inside of removeItem. How can it be that removing an item from an order unconditionally cancels it? Did you just find a bug in the code?

/* orderHandling.cpp */
void OrderHandling::handleItemRemoved(PurchaseOrder& order, const Item& removedItem) {
  order.removeItem(removedItem);
}

/* purchaseOrder.cpp */
void PurchaseOrder::removeItem(const Item& itemToRemove) {
  m_items.erase(itemToRemove);
  this.updateOrderSum();
  this.cancel(); // Why is the order cancelled?
}

To understand what is going on you take a look at the cancel method:

void PurchaseOrder::cancel() {
  if (m_orderItems.size() > 0) {
    return; // All items must be removed before cancelling
  }
  // Code that cancels the order goes here
  this.m_state = PurchaseOrderState::cancelled;
}

Aha! There is a condition hidden in the cancel method that makes calling it do nothing if there are any items in the order list. Sneaky! (In real world applications these conditions are usually many layers down and sometimes spread out over several different functions).

When you show the code to a senior colleague he informs you that the purchase order is supposed to be cancelled if all items are removed and this is how the business logic was implemented. Now things makes sense, but it required that you put in quite a lot of time and effort. Wouldn’t it be better if the code was cleaner?

If you paid attention to the code earlier you noticed that there is something called OrderHandling. This is a good place to add business logic such as cancelling of orders when the last item is removed. Let’s change the code a bit:

void OrderHandling::handleItemRemoved(PurchaseOrder& order, const Item& removedItem) {
  order.removeItem(removedItem);
  if (order.isEmpty()) {
    order.cancel();
  }
}

This should make it more clear. Now with this change in place we can remove the call to cancel from the removeItem method:

void PurchaseOrder::removeItem(const Item& itemToRemove) {
  m_items.erase(itemToRemove);
  this.updateOrderSum();
}

And finally, after checking and testing that there is no other place in the code that is depending on the condition in the cancel method to only cancel out empty orders it can be updated as well:

void PurchaseOrder::cancel() {
  m_state = PurchaseOrderState::cancelled;
}

Nice!

The next time you find a hidden condition like this secretly lurking in the depths of the code. Pull it out into the light! Make it really visible and obvious under what conditions certain functions should be called.

Happy coding!

Lämna en kommentar

Din e-postadress kommer inte publiceras. Obligatoriska fält är märkta *