Should data sanity checks be separated from methods that insert the data?

Question

I have a simple example of two setups of the same block of code, but i am not sure what would be considered the "better" option, although subjective, i read about methods often doing too much than which they describe.

So example is where i want to add data and do a sanity check first... the two choices would be:

public void Add(Data data)
{
    if(sanitycheck(data))
    {
       // ADD to list
    }
}

The second option - this seperates the logic into 2 methods:

public void Process(Data data)
{
   if(sanitycheck(data))
   {
      Add(data);
   }
}
private void Add(Data data) //private method
{
  // ADD to list
}

I feel like the first one does "more than it says", but the second one could be unnecessary and cutting the methods a little too thin for their purpose.

This example is crude compared to real life situations but i am curious which is generally a more smarter approach and best practice as code complexity ramps up?

Answer 1

From the client POV, the only thing you are doing is renaming Add to Process(and of course, you could avoid the renaming if you used some variation for the new method, like InternalAdd or by overloading the method).

From the internal POV, the reasons to create methods are reducing the complexity of code and code reuse. It might made sense if Add code is very complex, or if Add could be called by other methods without doing the sanity checks. Since the check seems rather simple, the later question should be the important one: is it a bug if, at any time, Add is called without having done the checks? If the answer is yes, almost the only result of dividing the method is a risk of introducing a bug.

Now, the option I prefer is sticking to the principle of Single Responsability(whatever it is). This would mean moving the data validation (sanitycheck) to a different class.

Some of the advantages of the above are:

• Your classes are smaller and simpler.

• You can validate the data at multiple stages, hopefully as soon as possible. No sense if doing any processing if you can check the data at the GUI stage and notify the user.

• You can reuse your validation code in another project even if the processing part is completely different.

Answer 2

The problem with your first method, and I suspect the reason you perceive the problem but which you're unable to find the exact motivation for, is that it operates at two entirely different levels of abstraction. It may not be the best-known of design principles, but I tend to think that the requirement than any method operate at a single level of abstraction is pretty important -- it's related to the single responsibility principle (as described in SJuan76's answer) but is much easier to understand and spot violations of.

Another comment regarding your question: you suggest that your second approach may "cut the methods too thin", but in my experience there is no such thing: even if a method simply calls two other methods, it has value because it provides an opportunity to name the combination of those methods and because it provides a point where a change in the process can be cleanly introduced -- e.g. if a third method becomes necessary -- say you need to send an event to some subscribed listeners when new data is added, that would add yet more complexity into a single method using your first design, but is a simple and elegant change in your second.

Answer 3

I do not agree with Jules or SJuan76. Validating data and starting any action on it are not separable responsibilities. Your problem is you are not throwing!

Processing data may fail for a number of reasons and the data being bogus in the first place is just one of them. For any unfixable problem you encounter, throw an exception. Not just any exception but a typed one that tells the caller exactly what the problem is.

Answer 4

I especially wonder what sanity checks should be carried out. To start, the class Data should do already a number of sanity checks to ensure that data actually makes sense. But I can understand there's cases where extra limitations apply, eg when Data can have missing values but you don't want these added.

Personally, I use a third way of doing this. In pseudo code:

public void Add(Data data)
{
   if( not(sanitycheck(data)))
   {
      throwException()
   }

//add code to add here
}

Hence:

• if extra limitations apply, check these first
• if these limitations are not fulfilled, tell caller what is wrong
• otherwise, carry out

The extra limitations have to be correctly documented though. But this way there's little reason for creating an extra private void.

This is different though when the Add method can also be called from other places with different sanity checks. That would be the only reason for me to create a separate private method.