0

When the caller gives me a call, I need to evaluate n number of criteria which currently I'm doing like

if (a & b & c & d & e)

Day by day the conditions are growing and it's really hard to read and understand even for me (who actually wrote the code). Is there any better way of doing this?

  • 1
    Your use case is not very clear to me. If they are sanity checks, I would recommend using coded exceptions for each of these conditions. If the conditions are homogenous, I'd recommend using a container/iterable to store them and do one pass to check the sanity. – Fallen Feb 7 at 13:57
2

Line breaks can help.

if (a
    & b
    & c
    & d
    & e
) { ...

And maybe that's the best you can do. There's certainly a sense in which these conditions are irreducible complexity; you have to record them somewhere.

One thing I notice is that all the clauses are AND'd together. Insofar as that's likely to remain true, count yourself lucky! (I acknowledge that your e for example might be some kind of OR group, but it sounds like the logic is dominated by a top-level AND combination.)

If the top level of the logic tree is nicely represented as a lengthy list of ANDs*, and you want to record that list somewhere else, you could probably use a list; many languages have an easy way to fold AND over an iterable of booleans. In python it's all. This is the best I could find for Java**.

So you'd be looking at something vaguely like

import java.util.function;
import java.util.stream

private Predicate<Context>[] conditions = {
    (c)->{ return a; },
    (c)->{ return b; },
    (c)->{ return c; },
    (c)->{ return d; },
    (c)->{ return e; },
};

...

Context context = ...;
if (Stream.of(conditions).allMatch(p -> p.test(context)) {
    ...
}

* or ORs; in which case you'd be using "any".
** actually it's complicated, you'll see in my example that I found a built-in that works on streams.

1

The first thing you could do is create a new method

private boolean checkCaller(Caller caller) {
  return a &&
         b &&
         c &&
         d;
}
// condition line
 if (checkCaller(caller)) {...

First, you hide the complexity from condition line to method with good name. I suppose you could name it better than checkCaller ;-)
Second, you write each condition at new line so it would be easy to read checkCaller body.
Third, you could re-view which arguments you have to receive to this method and probably simplify something at this point. Please note if you write checkCaller(a,b,c,d) then you do something wrong.

Next possible step is to simplifying checkCaller: re-group conditions and give them good names:

private boolean checkCaller(Caller caller) {
  bool validNumber = a && c;
  bool validCountryCode = b && d;
  return validNumber && validCountryCode && e;
}

P.S. Please note that introducing new method is in first place. If you re-group and give sound names at first, you'll met similar problem after some (probably, short) time.

  • Good answer. If the regrouped conditions are complex enough, they too could be split out into separate methods, e.g. isValidCountryCode() etc... – user949300 Feb 6 at 20:29
  • 1
    An expansion on @user949300's idea: if you write validNumber() && validCountryCode() && e, not only do you not have to cache results across a few lines, you still get the can-stop-early-if-anything-is-false benefit from the original approach ADS used. – J.G. Feb 6 at 20:54

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.