How can I reformat my condition to make it better?

Question

I have a condition

if(exists && !isDirectory || !exists)
{}

how can I modify it, so that it may be more understandable.

if(!isDirectory) ... (exists || !exists) will always be true. — Shark, Oct 05 '12 at 15:29
I read the title as "I have a personality problem, can I erase my mind to fix it". Yeah, I'm tired. — AnnanFay, Oct 12 '12 at 01:55
This should be moved to http://codereview.stackexchange.com/ — Thomas Eding, Oct 17 '12 at 20:22
@ThomasEding - given that the question has 8 answers, one of which is accepted, it's probably a waste of everyone's time to migrate this now. — ChrisF, Oct 17 '12 at 20:54

score 111 · Accepted Answer · edited Oct 06 '12 at 14:28

111

|| is commutative so

if(!exists || (exists && !isDirectory))

is equivalent.

Now because exists is always true in the second part of the || you can drop the &&:

if(!exists || !isDirectory)

Or you can go a step further and do:

if(!(exists && isDirectory))

edited Oct 06 '12 at 14:28

Ikke

290
1
8

answered Oct 05 '12 at 12:04

ratchet freak

25,706
2
62
97

5

What was implied but not explicitly mentioned here is that `&&` has higher precedence (at least in most well known languages - there may be exceptions) than `||`. Thus `a && b || c` is equivalent to `(a && b) || c` but not to `a && (b || c)`. – Péter Török Oct 05 '12 at 12:21
27

I think, that `!exists || !isDirectory` is more "understandable", because, `isDirectory` cannot be true if `!exists`. So as a human we will say "if it does not exist or it [exists and it] is not a directory". – duros Oct 05 '12 at 12:39
6

I prefer !exists || !isDirectory over the last one. – Apoorv Oct 05 '12 at 12:40
3

`||` is only commutative if used on values without side effects - if for example used with functions some functions might not get called (short-circuiting) or return a different value in a different order. – orlp Oct 05 '12 at 13:36
26

Anyone who relies on the relative precedence of '&&', '||', '==', '!=', etc and doesn't make their intention clear by using brackets deserves to be shot. In all languages, something like 'a && b || c' is equivalent to a comment saying that the author probably screwed the entire thing up in their rush to avoid typing a few extra characters. – Brendan Oct 05 '12 at 16:26
1

Or, if your language supports it, `unless(exists && isDirectory)`. – Nathan Long Oct 05 '12 at 17:47
Accurate, succinct and a correct answer to the OP. It's still horrible code to read and understand - in fact, it's harder to understand now than the code in the OP. The expression may be correct, but what is the writer trying to achieve? – mattnz Oct 05 '12 at 22:13
3

It should be just `if (!isDirectory)`, since `isDirectory` depends on `exists`. It can't be a directory and not exist. – Christoffer Hammarström Oct 06 '12 at 00:09
1

@ChristofferHammarström you don't know that, it could be that calling isDirectory on a non-existent file is UB (or a thrown exception) – ratchet freak Oct 06 '12 at 00:19
1

Well, if the variables refer to two different contexts, then they are too terse and need to be renamed. – Christoffer Hammarström Oct 06 '12 at 13:21
1

@Brendan: Or perhaps the programmer actually knows the language's rules. Hmmmmm... – Thomas Eding Oct 17 '12 at 20:21

zzzzBov · Answer 2 · 2012-10-17T19:16:24.403

52

As a process, I suggest building a truth table:

e = exists
d = isDirectory

e | d | (e && !d) || !e
--+---+----------------
0 | 0 | 1
0 | 1 | 1
1 | 0 | 1
1 | 1 | 0

This matches the NAND operation, which is simply:

!(exists && isDirectory)

If you don't remember all your logic gates, wikipedia has a nice reference with the truth tables to boot.

@Christoffer Hammarström brought up an important point about the state of isDirectory being tied to the state of exists. Assuming that they refer to the same reference, and that it's not possible to have a state where the reference doesn't exist and is a directory, the truth table can be written as follows:

e | d | (e && !d) || !e
--+---+----------------
0 | 0 | 1
0 | 1 | n/a
1 | 0 | 1
1 | 1 | 0

The n/a is used to represent a state that doesn't matter. Acceptable reductions could result in either 1 or 0 for states resulting in n/a.

With this in mind, !(exists && isDirectory) is still a valid reduction, resulting in a 1 for !e && d.

However, !isDirectory would be a much simpler reduction, resulting in 0 for !e && d.

edited Oct 17 '12 at 19:16

answered Oct 05 '12 at 17:57

zzzzBov

5,794
1
27
28

4

The next step is to realise that `isDirectory` depends on `exists`. It can't both be a directory and not exist. – Christoffer Hammarström Oct 06 '12 at 00:12
@ChristofferHammarstrom, Out of context I can't assume that the variables refer to the same thing, but that is a valid point. The result column should be filled with `n/a` in places where the state is impossible to achieve, and the equation reduced accordingly. – zzzzBov Oct 06 '12 at 04:24
Well, if the variables refer to two different contexts, then they are too terse and need to be renamed. – Christoffer Hammarström Oct 06 '12 at 13:22
But building a truth table and evaluating it is NP-complete! – Thomas Eding Oct 17 '12 at 20:18
@ThomasEding, I have two quotations for you then, "In theory, theory and practice are the same; in practice, they are not." and "Premature optimization is the root of all evil." – zzzzBov Oct 17 '12 at 20:23
"Premature optimization is the root of all evil." you are using the quote out of context. – Thomas Eding Oct 17 '12 at 20:24
@ThomasEding, my point was to keep track of your reality and to solve for the problems you have, instead of trying to solve for the problems you hope to have. In most cases, building a truth table to simplify a bit of logic is a good method. I never said this was a panacea. – zzzzBov Oct 17 '12 at 20:30

Puckl · Answer 3 · 2012-10-05T19:36:45.053

22

For better readability, I like to extract boolean conditions to methods:

if(fileNameUnused())
{...}

public boolean fileNameUnused() {
   return exists && !isDirectory || !exists;
}

Or with a better method name. If you can name this method properly, the reader of your code doesn´t need to figure out what the boolean condition means.

edited Oct 05 '12 at 19:36

answered Oct 05 '12 at 12:21

Puckl

1,525
2
13
17

+1 for saying something about useful names. But somewhere you will have to reformat the conditional. – Apoorv Oct 05 '12 at 12:41
4

A less extreme alternative, that still conveys intent, is just to name the condition used: `boolean fileNameUnused = !exists || !isDirectory; if (fileNameUnused) { doSomething(); }` – Steven Oct 06 '12 at 20:00

ZJR · Answer 4 · 2012-10-05T15:11:55.837

8

You could just try to nail the no-go case and bail out if that shows up.

while(someCondition) {

    if(exists && isDirectory)
        continue;
        // maybe "break", depends on what you're after.

        // the rest of the code
}

or even

function processFile(someFile)
{ 
    // ...
    if(exists && isDirectory)
       return false;
    // the rest of the code
    // ...
}

edited Oct 05 '12 at 15:11

answered Oct 05 '12 at 12:44

ZJR

6,301
28
36

Aren't break, continue, and more-than-one return statement considered code smells? – Freiheit Oct 05 '12 at 14:37
8

@Freiheit It depends on the context. Sometimes an early return statement is used to reduce indentation, thus enhancing readability. – marco-fiset Oct 05 '12 at 14:57
Best answer - complex conditionals waste enormous amounts of time reading and accurately understanding them. As a result often are "taken as read" leading to insidious bugs. – mattnz Oct 05 '12 at 22:08

Eddie G. · Answer 5 · 2012-10-06T14:22:58.233

You can use a truth table as pointed out. Second step could be a KV-map for minimizing the number of terms.

Using the laws of Boolean algebra is another approach:

A = exists
B = !isDirectory
!A = !exists

&& = *
|| = +

[Edit]
A simpler transform, because the operations AND and OR are mutually distributive :

exists && !isDirectory || !exists
= A*B + !A
= (A + !A) * (B + !A)
= 1*(B + !A)
= B + !A
[/Edit]

exists && !isDirectory || !exists
= A*B + !A
= A*B + !A*1 //Identity
= A*B + !A*(B+1) //Annihilator
= A*B + !A*B + !A //Distributivity and Identity
= B*(A + !A) + !A //Distributivity
= B*1 + !A //Complementation 2
= B + !A //Identity
= !isDirectory || !exists

Or with double complement (!!x = x):

A*B + !A
= !!(A*B + !A)
= !(!(A*B)*A)
= !((!A + !B)*A)
= !(!A*A + !B*A)
= !(0 + !B*A)
= !(!B*A)
= B + !A
= !isDirectory || !exists

+1 for using formal rules (I never though that I would see one of these after my first year of college). — Nemanja Boric, Oct 05 '12 at 23:48

score 5 · Answer 6 · answered Oct 05 '12 at 22:06

5

I don't like to use "!" when there is more than one condition in the expression. I'll add lines of code to make it more readable.

doesNotExist = !exists;
isFile = exists && !isDirecotry;
if (isFile || doesNotExist) 
   {}

answered Oct 05 '12 at 22:06

David W

151
2

+1 This makes it easier to read out as "if is file or does not exist" which is a lot closer to English. – Phil Oct 05 '12 at 22:09
This is a refactoring called [Introduce Explaining Variable](http://www.refactoring.com/catalog/introduceExplainingVariable.html). – Eddie G. Oct 28 '12 at 15:52

score 1 · Answer 7 · answered Oct 06 '12 at 16:23

1

As previously indicated, the condition can be reduced to:

if (!(exists && isDirectory))

However, I'll bet that being a directory implies existence. If so, we can reduce the condition to:

if (!isDirectory)

answered Oct 06 '12 at 16:23

Jack Applin

19
1

How can I reformat my condition to make it better?

7 Answers7

Linked