My function takes an optional parameter, type, which is used to filter through a collection. However, the filter should only apply if the parameter is passed in (in other words, non-null). If the filter is not passed in, it should not be applied. How can I write my logic to reflect this? What I have now is -

if( other conditions && ( Type == null || (Type != null && Order.Type == Type ) ) )
    return val += Order;

But this feels clunky and wrong. Is there a way I can simplify my logic here?

  • 1
    What language? If you are using a language with short-circuiting Boolean operators, you could write it as (Type == null || Order.Type == Type) Jun 17 '14 at 19:30
  • The language does have short circuiting, so that would work. Jun 17 '14 at 19:31
  • Okay, I added this as an answer. Jun 17 '14 at 19:33

The OP has stated in a comment that the language used has short-circuiting Boolean operators, so this could be written as:

if( other conditions && ( Type == null || Order.Type == Type ) )
    return val += Order;

It's often cleaner to avoid the null altogether by creating two functions: one with the filter and one without. The former can call the latter, like:

if (Order.Type == Type)
  return nonFilterFunction(...)

This style follows the Single Responsibility Principle by having one function do only filtering and one function doing the other stuff. It allows the compiler and runtime assertions to help you out in verifying programmers are using your code correctly. It's also easier to reason about because in one function you don't have to worry about Type at all, and in the other one, you can assume or assert that Type is never null and not have to worry about it afterward.

The main drawback is one extra function in your API. Also, this style is difficult if you have a lot of optional parameters, because you couldn't create a new function for every combination, but there are usually other ways to handle that situation.

  • An advantage of using parameters to select choices, which is lost by using distinct functions, is that a single wrapper which passes caller-supplied options to a wrapped method can handle many different usage cases. If the different usage cases were segregated into different methods, it would be necessary to create a separate wrapper for each.
    – supercat
    Jun 17 '14 at 23:21
  • I already mentioned if you had a lot of combinations of options, you might want to handle it differently. Still, being able to "handle many different usage cases" in one function isn't necessarily an advantage. Smaller, single purpose functions are almost always more simple and clear, even if you must create more of them. Jun 18 '14 at 13:10
  • The issue isn't just when there are lots of combinations. Even when there are just two options, I would consider avoidable code duplication smelly. Consider, for example, [Try]ParseRecord. If throw-on-error is controlled by a parameter which can be passed to [Try]ParseSubRecord, then subrecord parsing can throw an exception which the main caller will be able to see. If the methods are completely separate, then all the code will have to be duplicated, creating a significant possibility that the two methods may end up unintentionally differing in what they accept.
    – supercat
    Jun 18 '14 at 15:46

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.