Multiple functions assuming value or check value in each function

Question

Let's say I have an object A with a public method bool Foo(arg). Foo is potentially a complex algorithm with nested rules (if's). I don't want Foo to have all the code, I want several intermediate functions to make my code more expressive but I face a dilemma.

Here is a simple example: Let's say Foo could be written:

public bool Foo(arg) {
  if (arg.attr1) {
    if (arg.attr2) return false;
    else if (arg.attr3) return false;
  }
  return true;
}

However what is tested is more complexe and are in fact business rules. So a more expressive way to write the code is:

bool Rule1(arg) {
  if (arg.attr1) return false;
  return true;
}

bool Ruel2(arg) {
  if (arg.attr2) return false;
  return true;
}

bool Rule3(arg) {
  if (arg.attr3) return false;
  return true;
}

public bool Foo(arg) {
  if (Rule1) return false;
  if (Ruel2) return false;
  if (Rule2) return false;
  return true;
}

My dilemma is that rule 2 and 3 are only true after validation of rule 1. Since only Foo is public no client can misuse the individual rule functions however an other developer could get a new requirement and use rule 2 or 3 without first checking for rule 1. I could however test for rule 1 inside both rule 2 and 3 but it seems redundant.

Can I assume some properties in internal functions or should I still do the necessary checks in each functions?

Edit: Not a duplicate of How to tackle a 'branched' arrow head anti-pattern?. I have already tackle the arrow effect with early return. The question is more about the use of inner method for more expressiveness and what to test in each method.

Edit The two solution may not have equivalent result. I'm sorry about this but don't worry about this assume that it'll be the case in real life.

Rule# names will be more expressive in the end with proper business name (like IsBleu, IsAntique, ...)

if (arg.attr2) are overly simplified test. Real code can compare data in data base, dates, compare with global const values...

Answer 1

My dilemma is that rule 2 and 3 are only true after validation of rule 1. Since only Foo is public no client can misuse the individual rule functions however an other developer could get a new requirement and use rule 2 or 3 without first checking for rule 1...

And? The code that other developer writes won't then work. The thing is, this is only ever a dilemma in situations where a dev team do not write automated tests for such business rules.

If you have a set of unit tests around your existing functionality of Foo and that other developer follows the principle of writing failing tests for new functionality then making them pass (without breaking your existing tests), they'll soon realise that rule 1 must be called first and will write code that does so. Otherwise a test will fail and stay failed.

Sometimes you have to approach things from a different angle in order to overcome not being able to "see the wood for the trees". In this case, a fear of someone breaking the code in future should not force you to choose a sub optimal solution now. Pick the best solution and write tests to prevent that future breakage and the wood comes into perfect focus.

Answer 2

If the outcome of Rule2 is only valid when the outcome of Rule1 is valid, executing Rule1 as part of Rule2 would make sense. That way Rule2 will never be executed on its own.

The concern that Rule1 will be executed multiple times (separately or as part of Rule2 and Rule3) is a technical problem (assuming that the execution of a rule has no side-effects) So a technical solution might be needed if that is an actual concern (e.g. when execution of Rule1 takes a lot of resources such as time or memory)

One technical solution might be to store the result of Rule1 and use the cached result on subsequent calls. This is dangerous if the parameters to the subsequent invocations change. How to solve that depends on the actual requirements.

Answer 3

I want several intermediate functions to make my code more expressive

Assuming your first example was the correct one (they aren't equivalent and the second one is buggy) let's first try to make the logic here a little more readable.

public bool Foo(arg) {
  if (arg.attr1) {
    if (arg.attr2) return false;
    else if (arg.attr3) return false;
  }
  return true;
}

According to this truth table, that

attr1 | attr2 | attr3 | Foo
  F       F       F      T
  F       F       T      T
  F       T       F      T
  F       T       T      T
  T       F       F      T
  T       F       T      F
  T       T       F      F
  T       T       T      F

is the same as this

public bool Foo(arg) {
    return !arg.attr1 || (!arg.attr2 && !arg.attr3);
}

Now let's add those intermediate functions

Rule1 | Rule2 | Rule3 | Foo
  T       T       T      T
  T       T       F      T
  T       F       T      T
  T       F       F      T
  F       T       T      T
  F       T       F      F
  F       F       T      F
  F       F       F      F

public bool Foo(arg) {
    return Rule1() || (Rule2() && Rule3());
}

This is far more readable, but if you insist on explicit return values, maybe because you love your breakpoints, you can use this form:

public bool Foo(arg) {
    if (Rule1()) return true;
    if (Rule2() && Rule3()) return true;
    return false;
}

Which I'll argue is still more readable than the original. No nesting, no elses. Just some early returns. Only problem here is Rule# and attr# are not the most expressive names. I'm against adding intermediate functions if it creates more bad names. If you can think of good names though, go for it.