If I have a method that needs verification in order to proceed, do I call it inside the method or before? [duplicate]

Question

This has been bothering me. This is more of a pragmatic problem than technical. Imagine I have a SaveOrderChanges method which, as its name implies, will save the order changes when the user submits the modified order.

But, before saving the order, I need to verify if the order submitted is actually from the current user, so I proceed to do a simple verification in the DB. I have a special method that works with this situation which I've called "VerifyOrder()".

The true question is if the SaveOrderChanges() method should include the call to that method, or should the other method be called before SaveOrderChanges() is called

imagine I have some method like this:

Do I make it like this :

public bool SaveOrderChanges(Order order)
{
  VerifyOrder(order); //Verify that the order is from the actual user
  //Some Code Logic in here
}

Or like this:

if(VerifyOrder(order))
{
  SaveOrderChanges(order); 
}

Answer 1

You should choose the latter.

In making my decision, I decided to look at this from the maintenance programmer's perspective. Here's how I'd see each case:

1. VerifyOrder(order) is inside SaveOrderChanges(Order order) so the verification is part of what SaveOrderChanges() does. You need to verify in order to save.

2. VerifyOrder(order) needs to be valid in order to run SaveOrderChanges(Order order), so the verification is not part of what SaveOrderChanges(Order order) does. Any changes to SaveOrderChanges() does not have to include VerifyOrder().

It's all a matter of semantics. By placing the verification inside the function, you are saying that the function does the verification. If you need the verification done beforehand, make sure it is not part of the function to keep separation of concerns.

Additionally, let's consider the case where you go with (1) and the verification fails. The execution will exit the verification function and still be inside SaveOrderChanges() (as your code is written). This is begging for bugs you don't want.

Answer 2

The first one; SaveOrderChanges should call VerifyOrder. Otherwise, some future coder will call SaveOrderChanges without calling VerifyOrder. An alternative that still prevents such an error is to call VerifyOrder outside SaveOrderChanges, but for both methods to be private.

I am assuming that SaveOrderChanges has a precondition that VerifyOrder will succeed; it is being called to detect bugs rather than as part of processing the order. Under this assumption, it is (probably) the responsibility of the caller to only call SaveOrderChanges on valid orders; ideally a failed call to VerifyOrder indicates a bug in the caller's code.

Answer 3

If you can really screw up the state of your data by saving an invalid order then something needs to do verification. Either the datastore needs to blow up and reject the transaction, or some code needs to check the validity of the order. I would not expose a public API that allows saving an order if it can result in data corruption.

Answer 4

If computing the verification is not expensive, I would verify before calling the method, and assert that verification passes inside the method. This ensures will catch programming errors sooner, and allows callers to handle the else case however they want.