Help refactoring this C# function

Question

I have written functions that look like this:

bool IsDry(bool isRaining, bool isWithUmbrella) {
    if (isRaining) {
        if (isWithUmbrella)
            return true;
        else
            return false;
    }
    else
        return true;
}

I need to check, if it's raining, then the person needs to carry an umbrella in order to keep dry (don't laugh, this is just an example, our actual business rules are more serious than this).

How can I refactor this, because right now it looks clumsy.

Thanks for the help, guys! =)

While your code is on the verbose side, I would caution against coding logical expressions with more than two variables in one line. As one-liner logic expressions get more complex, refactoring them becomes increasingly error prone, as they are less readable. E.g., try adding isIndoors as another variable. Notice the one-liner becomes harder for humans to parsee on the fly. Just bear this in mind when refactoring - terseness does not imply readability. — D'Arcy Rittich, Aug 14 '10 at 13:09
Yikes, the old `if(isTrue) return true; else return false;` antipattern! — Callum Rogers, Aug 18 '10 at 14:46

score 69 · Answer 1 · edited May 23 '17 at 11:53

It looks like the business rule you're trying to enforce is:

P IMPLIES Q

This is logically equivalent to:

(NOT P) OR Q

Thus, you can simply write:

bool IsDry(bool isRaining, bool isWithUmbrella) {
    return !isRaining || isWithUmbrella;
}

On (not) thinking negatively

Depending on the predicate, it may also be simpler to first think in terms of its negation.

NOT (P IMPLIES Q)

We now substitute the identity above:

NOT ((NOT P) OR Q)

Now we can apply DeMorgan's Law:

P AND (NOT Q)

Since this is the negation, we must negate this to get back to the positive. The double negation may seem confusing at first, but going back to the example, we have:

bool IsDry(bool isRaining, bool isWithUmbrella) {
    bool isWet = (isRaining && !isWithUmbrella);
    return !isWet;
}

Additional tips

Here are some examples of common boolean expression rewriting:

BEFORE                                  | AFTER
________________________________________|________________________________________
                                        |
if (condition == true) ...              | if (condition) ...
________________________________________|________________________________________
                                        |
if (condition == false) ...             | if (!condition) ...
________________________________________|________________________________________
                                        |
if (condition) {                        | return condition;
    return true;                        |
} else {                                |
    return false;                       |
}                                       |
________________________________________|________________________________________
                                        |
if (condition1) {                       | return (condition1 && condition2
   if (condition2) {                    |             && condition3);
      if (condition3) {                 |
         return true;                   |
      } else {                          |
         return false;                  |
      }                                 |
   } else {                             |
      return false;                     |
   }                                    |
} else {                                |
   return false;                        |
}                                       |
________________________________________|________________________________________
                                        |
return (condition1 && !condition2) ||   | return condition1 != condition2;
   (condition2 && !condition1);         | // or  condition1 ^ condition2;

Note that the predefined ^ in C# is the exclusive-or operator, even for integral types (i.e. it's not an exponentiation operator). The predefined && and || are conditional logical operators that performs "short-circuit" evaluation.

+1. Just to add to this, `!(isRaining && !isWithUmbrella)` is equivalent to `(!isRaining || isWithUmbrella)` by DeMorgan's. — NullUserException, Aug 14 '10 at 13:00
might help get your answer votes. if you explained that p implies q is descrete maths... oh 55 votes never mind :) — Johnno Nolan, Aug 15 '10 at 21:13
Oh boy, too bad the OP is unregistered. You deserve one more guru badge! — BalusC, Aug 17 '10 at 17:18

score 11 · Answer 2 · answered Aug 14 '10 at 12:52

11

bool IsDry(bool isRaining, bool isWithUmbrella) 
{
    return (!isRaining || isWithUmbrella);
}

answered Aug 14 '10 at 12:52

NullUserException

83,810
28
209
234

@Mitch Thanks to that vote I think I found a potential bug on the rep system. Posting on meta now. – NullUserException Aug 14 '10 at 13:28

score 8 · Answer 3 · answered Aug 14 '10 at 12:52

8

bool IsDry(bool isRaining, bool isWithUmbrella)
{
    return !isRaining || isWithUmbrella;
}

answered Aug 14 '10 at 12:52

Mitch Wheat

295,962
43
465
541

@Null, it's a popularity contest don't you know! +1 to you Null. – JohnB Aug 14 '10 at 13:31

score 5 · Answer 4 · answered Aug 14 '10 at 12:59

5

Here's a truth table:

isRaining    isWithUmbrella    isWet    isDry 
true         true              false    true
true         false             true     false
false        true              false    true
false        false             false    true

Some one answer could be:

var isWet = isRaining && !isWithUmbrella;
return !isWet;

answered Aug 14 '10 at 12:59

Douglas

36,802
9
76
89

Why not just: `return (!(isRaining && !isWithUmbrella));` ? – George Stocker Aug 16 '10 at 12:36
1

I'm trying to show intent by giving an explicit meaning to the inner boolean expression. The hope is that it makes it easier for someone who comes along later with an isAlreadyWetAnyway flag to put it in the right place. – Douglas Aug 16 '10 at 13:04

score 5 · Answer 5 · answered Aug 14 '10 at 13:03

The approach I usually take is successive refinement. e.g. first eliminate the inner if-else:

bool IsDry(bool isRaining, bool isWithUmbrella) {
    if (isRaining)
        return isWithUmbrella;
    else
        return true;
}

then collapse the if

bool IsDry(bool isRaining, bool isWithUmbrella) {
    return isRaining ? isWithUmbrella : true;
}

score 3 · Answer 6 · answered Aug 14 '10 at 17:24

How can I refactor this

With unit tests.

Seriously. There are two Boolean inputs, so you only need four unit tests to fully cover this method.

Then, with full branch coverage already in place, you can just play around with the implementation. Try something that seems right (I find it very helpful to write a truth table in this case), and the tests will tell you if you got a detail wrong.

And as a further benefit, you can keep the tests around forever, as documentation of what the method actually does. This is especially useful if you opt for a clever implementation, like fancy Boolean expressions -- if you're scratching your head trying to follow the flow, you can just look at the tests and see, "Oh, I see -- if I pass this and this, I get this."

+1! This will also show usage from the caller's perspective, which can help with design. if (IsDry(true, false)) can be difficult to interpret in client code - what do the Boolean values represent? Perhaps this could be an instance method of a class with IsRaining and HasUmbrella properties. — TrueWill, Aug 22 '10 at 17:20

Anthony Faull · Answer 7 · 2010-08-22T17:01:25.593

2

With just six clicks ReSharper will simplify your code into a single line.

The first three clicks will morph

 if (isWithUmbrella)
                    return true;
                else
                    return false;

into

return isWithUmbrella;

The next three clicks change

    if (isRaining)
    {
        return isWithUmbrella;
    }
    else
        return true;

into

    return !isRaining || isWithUmbrella;

and voila, you're done.

edited Aug 22 '10 at 17:01

answered Aug 14 '10 at 17:14

Anthony Faull

17,549
5
55
73

3

this answer may be useful if you explained why ReSharper would help. – Johnno Nolan Aug 15 '10 at 21:09
I've added an explanation. Thanks for your feedback. – Anthony Faull Aug 22 '10 at 17:03

score 1 · Answer 8 · answered Aug 14 '10 at 15:21

1

bool IsDry(bool isRaining, bool isWithUmbrella) {
 return isRaining ? isWithUmbrella : true;
}

answered Aug 14 '10 at 15:21

Ivey

499
4
13

score 1 · Answer 9 · answered Aug 14 '10 at 18:52

1st remove the "else" from "else return ...;" statements, so you get:

if (isRaining) {
    if (isWithUmbrella)
        return true;
    return false;
}
return true;

With a little logic work..

if (isRaining) {
//    return (isWithUmbrella) ?
//        true :
//        false;
    return isWithUmbrella;
}
return true;

Then, you can quickly put it into a simple return statement...

//return (isRaining) ? isWithUmbrella : true;
//return (!isRaining) ? true : isWithUmbrella;
return (!isRaining) || isWithUmbrella;

score 0 · Answer 10 · answered Aug 14 '10 at 14:35

0

It's perfectly obvious

bool IsNotRaining { return isWithUmbrella }

:-)

answered Aug 14 '10 at 14:35

Mark Mullin

1,340
1
9
21

Developing software and having no sense of humor is a certain recipe for misery – Mark Mullin Aug 15 '10 at 18:53

Help refactoring this C# function

10 Answers10

On (not) thinking negatively

Additional tips

See also