0

Which one is better?

A

if ( complexDataCheck(...) ) {
  doJob(...)
}

vs

B

doJob()

func doJob(...) { 
  if not(complextDataCheck(...)) return error; 
  ... 
}

In A, the function does one and only one thing which is good, but OTOH it may be called with invalid input.

In B, the function does multiple things (assuming the validation part is a non-trivial part of the logic).

I want to know what are other pros and cons of the above and if there is a third better option?

6

Both can be useful, but neither is better.

Which way this is handled has to be determined by gathering data and making decisions. Making the decision is a complex process that depends on requirements, what assumptions are safe to make and how the functions will be used.

Start with your "A" case:

function isValid(data)
    ...Do expensive validity check, return result...

function doJob(data, x)
    if not isValid(data)  // Expensive
        raise exception
    ...Do job, return result...

This favors correctness by forcing the data through a validity check each time doJob() is called. This is fine when doing it once...

DataType data = ...
Number x = ...
doJob(data, x)        // Expensive, but only once

...but sacrifices performance when done repeatedly on the same data:

for x in 1..50
    doJob(data, x)   // Expensive 50 times

If this meets your performance requirements, you've got all of the safety and performance you need and there's no need to do anything further.

If it doesn't, there has to be a trade-off to regain the lost performance. You might do this by providing a separate, non-validating version of doJob() to optimize out the repeated validation as might happen in your "B" case:

if not isValid(data)  // Expensive, but only once.
    raise exception
// At this point, we know data is valid and can use it safely.
for x in 1..50
    doJobUnvalidated(data, x)

You've traded some safety (leaving validation to callers) for performance (validating only once).

When providing code for others (or even yourself) to call, there's no substitute for finding out the intended uses before starting development and how it's actually used after you've released it. If the 1..n case turns out to be common, that's good reason provide a function that does it safely:

function doJobRepeatedly(data, n)
    if not isValid(data)
        raise exception
    for x in 1..n
        doJobUnvalidated(data, x)

The onus for correctness is still on you as a caller to doJobUnvalidated(), but it concentrates understanding of the rules into your library and there's one implementation. This is much better than spreading knowledge and implementations across each place in calling code where it needs to happen.

Finally, and I can't stress this enough, make sure the assumptions made by your functions are documented so callers understand what they're getting into. There's nothing more frustrating than having your program go kaboom, debugging the failure and tracing it to an undocumented assumption in someone else's code.

|improve this answer|||||
  • Note that for some languages with static type systems, it's entirely possible to have both the efficiency of validating once as well as the safety of functions that don't operate on invalid data. Simply have your validate() function return a new type, validData, that contains all the data it was handed but is guaranteed to be valid. Then make all your other related functions only work with the validData type. – 8bittree Jun 24 '19 at 17:28
1

There is a third option: don't return.

In your first example the client using your function is responsible

  • for being sure data is valid before passing it,
  • for dealing with the result of calling the function, and
  • for knowing when to call the function

In your second example the client is responsible

  • for dealing with the result and
  • for knowing when to call the function.

The third option is to only be responsible

  • for knowing when to call the function

Pass it whatever data and let it tell whatever handler about whatever results.

If the function is a method hanging off an object the handler could have been set when the object was constructed elsewhere so our client doesn't have to know that either.

The handler can be an output port that sends results to some user interface. Done this way the client doesn't care what happened. Success, failure, nothing, it's all the same to the using client that only cares that it called this when it was supposed to.

Same trick works for the validating logic if you don't want the function to have to deal with that. Just set a validater during construction.

Of course without a return this isn't strictly speaking a function anymore. It's a method or a subroutine. But it still works.

|improve this answer|||||
0

GRASP, which, IMO, is far superior to and more understandable than SOLID, has the concept of an "Information Expert".

Using the principle of information expert, a general approach to assigning responsibilities is to look at a given responsibility, determine the information needed to fulfill it, and then determine where that information is stored.

This will lead to placing the responsibility on the class with the most information required to fulfill it.[4]

  1. Does the function have most of the information? (simplest example, a test for null or empty String) If so, let it do the check. Given your name, "complexDataCheck()", probably not.
  2. Otherwise, the class or function that does have most of the information should do the check.
|improve this answer|||||
-1

The best option is to make your function incapable of being called 'wrongly'

The best way to achieve that is to define the result for all inputs rather than to throw errors for those conditions where a naive approach would be incongruous.

For example:

Get a customer by id where the id doesn't exist? Return null.

Price per item when there are zero items? Return the total price.

etc

|improve this answer|||||
  • I don't understand your 2nd bullet. – user949300 Jun 22 '19 at 18:52
  • 1
    If passing wrong inputs is the calling code's fault, then the authors of the calling code should be made acutely aware of that. Throwing exceptions is a good way of getting that message out. – Nick Alexeev Jun 22 '19 at 19:47
  • @Nick with my method there is no wrong input to be passed, no fault to be attributed and no error to be thrown – Ewan Jun 23 '19 at 8:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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