2

This question already has an answer here:

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); 
}

marked as duplicate by gnat, Community May 27 '15 at 20:25

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

2

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.

4

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.

  • 1
    This seems like the obvious answer. A good API interface makes it hard for the user to use your API incorrectly. If you require the user to call VerifyOrder before using SaveOrder then you are asking for problems. So when the customer comes along and says you are saving invalid order data, it isn't the user whose code isn't functioning correctly. It's yours. You let the bad data get through, regardless of whether you commented the heck out of your method description that they have to call VerifyOrder first. Your SaveOrder method still did the wrong thing. – Dunk May 27 '15 at 21:50
0

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.

0

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.

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