Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This is a minor niggle, but every time I have to code something like this, the repetition bothers me, but I'm not sure that any of the solutions aren't worse.

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}
  • Is there a name for this kind of logic?
  • Am I a tad too OCD?

I'm open to evil code suggestions, if only for curiosity's sake...

share|improve this question
7  
@Emmad Kareem : two DefaultAction calls violate DRY principle –  Abyx Nov 30 '11 at 11:52
9  
I think the main issue here is that you are working at inconsistent levels of abstraction. The higher abstraction level is: make sure I have valid data for DoSomething(), and then DoSomething() with it. Otherwise, take DefaultAction(). The nitty gritty details of making sure you have the data for DoSomething() are at a lower abstraction level, and therefore should be in a different function. This function will have a name in the higher abstraction level, and its implementation will be low-level. The good answers below address this issue. –  Gilad Naor Nov 30 '11 at 15:37
2  
Please specify a language. Possible solutions, standard idioms, and longstanding cultural norms are different for different languages and will lead to different answers to your Q. –  Caleb Nov 30 '11 at 19:12

22 Answers 22

Extract it to separate function (method) and use return statement:

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }
}

DefaultAction();

Or, maybe better, separate getting contents and its processing:

contents_t get_contents(name_t file)
{
    if(!FileExists(file))
        return null;

    contents = OpenFile(file);
    if(!SomeTest(contents)) // like IsContentsValid
        return null;

    return contents;
}

...

contents = get_contents(file)
contents ? DoSomething(contents) : DefaultAction();

Upd:

Why not exceptions, why OpenFile doesn't throw IO exception:
I think that it's really generic question, rather than question about file IO. Names like FileExists, OpenFile can be confusing, but if to replace them with Foo, Bar, etc, - it would be clearer that DefaultAction may be called as often as DoSomething, so it may be non-exceptional case. Péter Török wrote about this at end of his answer

Why there is ternary conditional operator in 2nd variant:
If there would be [C++] tag, I'd wrote if statement with declaration of contents in its condition part:

if(contents_t contents = get_contents(file))
    DoSomething(contents);
else
    DefaultAction();

But for other (C-like) languages, if(contents) ...; else ...; is exactly the same as expression statement with ternary conditional operator, but longer. Because the main part of the code was get_contents function, I just used the shorter version (and also omitted contents type). Anyway, it's beyond this question.

share|improve this answer
77  
+1 for multiple returns - when methods are made sufficiently small, this approach worked best for me –  gnat Nov 30 '11 at 11:31
1  
Multiple return paths can have negative performance implications in C++ programs, defeating the optimizer's efforts to employ RVO (also NRVO, unless each path returns the same object). –  Functastic Dec 1 '11 at 5:37
1  
Hi Abyx, I noticed you incorporated some of the feedback from the comments here: thanks for doing that. I've cleaned up everything that was addressed in your answer and other answers. –  user8 Dec 1 '11 at 19:37

If the programing language you're using (0) short circuits binary comparisons (i.e. if doesn't call SomeTest if FileExists returns false) and (1) assignment returns a value (the result of OpenFile is assigned to contents and then that value is passed as an argument to SomeTest), you can use something like the following, but it would still be advised to you to comment the code noting that the single = is intentional.

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}

Depending on how convoluted the if is, it might be better to have a flag variable (which separates the testing of success/failure conditions with the code that handles the error DefaultAction in this case)

share|improve this answer
4  
Pretty gross to put so much code in an if statement, in my opinion. –  moteutsch Dec 1 '11 at 22:07
5  
I, on the contrary, like this kind of "if something exists and meets this condition" statement. +1 –  Gorpik Dec 2 '11 at 12:40

More seriously than the repetition of the call to DefaultAction is the style itself because the code is written non-orthogonal (see this answer for good reasons for writing orthogonally).

To show why non-orthogonal code is bad consider the original example, when a new requirement that we should not open the file if it is stored on a network disk is introduced. Well, then we could just update the code to the following:

if(FileExists(file))
{
    if(! OnNetworkDisk(file))
    {
        contents = OpenFile(file); // <-- prevents inclusion in if
        if(SomeTest(contents))
        {
            DoSomething(contents);
        }
        else
        {
            DefaultAction();
        }
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

But then there also comes a requirement that we should not open large files over 2Gb either. Well, we just update again:

if(FileExists(file))
{
    if(LessThan2Gb(file))
    {
        if(! OnNetworkDisk(file))
        {
            contents = OpenFile(file); // <-- prevents inclusion in if
            if(SomeTest(contents))
            {
                DoSomething(contents);
            }
            else
            {
                DefaultAction();
            }
        }
        else
        {
            DefaultAction();
        }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

It should be very clear that such code style will be a huge maintenance pain.

Among the answers here which is written properly orthogonally are Abyx' second example and Jan Hudec's answer, so I will not repeat that, just point out that adding the two requirements in those answers would just be

if(! LessThan2Gb(file))
    return null;

if(OnNetworkDisk(file))
    return null;

(or goto notexists; instead of return null;), not affecting any other code than those lines added. E.g. orthogonal.

When testing, the general rule should be to test exceptions, not the normal case.

share|improve this answer
3  
+1 for me. Early returns help to avoid the arrowhead anti pattern. See codinghorror.com/blog/2006/01/flattening-arrow-code.html and lostechies.com/chrismissal/2009/05/27/… Prior to reading about this pattern, I always subscribed to the 1 entry/exit per function theory due to what I was taught 15 years or so ago. I feel this just makes code so much easier to read and as you mention more maintainable. –  Mr Moose Dec 1 '11 at 2:10
1  
@MrMoose: your mention of the arrowhead anti-pattern answers Benjol's explicit question: "Is there a name for this kind of logic?" Post it as an answer and you've got my vote. –  outis Dec 1 '11 at 5:59
1  
+1 for "test exceptions, not the normal case." –  Roy Tinker Dec 1 '11 at 17:54

Obviously:

Whatever(Arguments)
{
    if(!FileExists(file))
        goto notexists;
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(!SomeTest(contents))
        goto notexists;
    DoSomething(contents);
    return;
notexists:
    DefaultAction();
}

You said you are open even to evil solutions, so using evil goto counts, no?

In fact depending on context this solution might well be less evil than either evil doing the action twice or evil extra variable. I wrapped it in a function, because it would definitely not be OK in the middle of long function (not least due to the return in the middle). But than long function is not OK, period.

When you have exceptions, they will be easier to read, especially if you can have the OpenFile and DoSomething simply throw exception if the conditions are not satisfied, so you don't need explicit checks at all. On the other hand in C++, Java and C# throwing an exception is a slow operation, so from performance point, the goto is still preferable.


Note on "evil": C++ FAQ 6.15 defines "evil" as:

It means such and such is something you should avoid most of the time, but not something you should avoid all the time. For example, you will end up using these "evil" things whenever they are "the least evil of the evil alternatives."

And that applies to goto in this context. Structured flow control constructs are better most of the time, but when you get in the situation where they accumulate too many of their own evils, like assignment in condition, nesting more than about 3 levels deep, duplicating code or long conditions, goto may simply end up being less evil.

share|improve this answer
9  
My cursor is hovering over the accept button... just to spite all the purists. Oooohh the temptation :D –  Benjol Nov 30 '11 at 12:33
2  
Yes, Yes! This is the absolutely "right" way to write code. The structure of the code now is "If error, handle error. Normal action. If error, handle error. Normal action" which is exactly like it should be. All the "normal" code is written with just a single level indentation while all the error related code has two levels of indentation. So the normal AND MOST IMPORTANT code gets the most prominent visual place and it is possible to very quicly and easily read the flow sequencially downwards. By all means accept this answer. –  hlovdal Nov 30 '11 at 19:25
2  
And another aspect is that the code written this way is orthogonal. For instance the two lines "if(!FileExists(file))\n\tgoto notexists;" are now ONLY related to handling this single error aspect (KISS) and most importantly it does not affect any of the other lines. This answer stackoverflow.com/a/3272062/23118 lists several good reasons to keep code orthogonal. –  hlovdal Nov 30 '11 at 19:48
5  
Speaking of the evil solutions: I can have your solution without goto: for(;;) { if(!FileExists(file)) break; contents = OpenFile(file); if(!SomeTest(contents)) break; DoSomething(contents); return; } /* broken out */ DefaultAction(); –  herby Nov 30 '11 at 21:29
2  
@herby: Your solution is more evil than goto, because you are abusing break in a way nobody expects it to be abused, so people reading the code will have more problems seeing where the break leads them than with goto which says it explicitly. Besides you are using an infinite loop that will only run once, which will be rather confusing. Unfortunately do { ... } while(0) is not exactly readable either, because you only see it's just a funny block when you get to the end and C does not support breaking from other blocks (unlike perl). –  Jan Hudec Dec 2 '11 at 7:11

One possibility:

boolean handled = false;

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        handled = true;
    }
}
if (!handled)
{
    DefaultAction();
}

Of course, this makes the code slightly more complex in a different way. So it is largely a style question.

A different approach would be using exceptions, e.g.:

try
{
    contents = OpenFile(file); // throws IO exception if file not found
    DoSomething(contents); // calls SomeTest() and throws exception on failure
}
catch(Exception e)
{
    DefaultAction();
    // and the exception should be at least logged...
}

This looks simpler, however it is only applicable if

  • we know precisely what kind of exceptions to expect, and DefaultAction() fits to each
  • we expect the file processing to succeed, and a missing file or a failing SomeTest() is clearly an erroneous condition, thus it is suitable to throw an exception on it.
share|improve this answer
14  
Noooo~! Not a flag variable, it's definitely wrong way, because it leads to complex, hard to understand (where-that-flag-becomes-true) and hard to refactor code. –  Abyx Nov 30 '11 at 10:36
4  
+1 This mutual 'Nooooo!' is funny. I think the status variable and early return are both reasonable in certain cases. In more complex routines, I'd go for the status variable because, rather than adding complexity, what it really does is make the logic explicit. Nothing wrong with that. –  grossvogel Nov 30 '11 at 18:58
1  
This is our preferred format where I work. The 2 main usable options seem to be "multiple returns" and "flag variables". Neither seems to have any kind of true advantage on average, but both fit certain circumstances better than others. Have to go with your typical case. Just another "Emacs" vs. "Vi" religous war. :-) –  Brian Knoblauch Nov 30 '11 at 19:34
function FileContentsExists(file) {
    return FileExists(file) ? OpenFile(file) : null;
}

...

contents = FileContentExists(file);
if(contents && SomeTest(contents))
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
share|improve this answer
1  
Yes. Depends. @Benjol knows better. –  herby Nov 30 '11 at 13:18
3  
... of course I meant "go the extra mIle"... :) –  UncleZeiv Nov 30 '11 at 15:36

Functions should do one thing. They should do it well. They should do it only.
— Robert Martin in Clean Code

Some people find that approach a little extreme, but it's also very clean. Allow me to illustrate in Python:

def processFile(self):
    if self.fileMeetsTest():
        self.doSomething()
    else:
        self.defaultAction()

def fileMeetsTest(self):
    return os.path.exists(self.path) and self.contentsTest()

def contentsTest(self):
    with open(self.path) as file:
        line = file.readline()
        return self.firstLineTest(line)

When he says functions should do one thing, he means one thing. processFile() chooses an action based on the result of a test, and that's all it does. fileMeetsTest() combines all the conditions of the test, and that's all it does. contentsTest() transfers the first line to firstLineTest(), and that's all it does.

It seems like a lot of functions, but it reads practically like straight English:

To process the file, check if it meets the test. If it does, then do something. Otherwise, take the default action. The file meets the test if it exists and passes the contents test. To test the contents, open the file and test the first line. The test for the first line ...

Granted, that's a little wordy, but note that if a maintainer doesn't care about the details, he can stop reading after just the 4 lines of code in processFile(), and he will still have good high level knowledge of what the function does.

share|improve this answer
3  
+1 It's good advice, but what constitutes "one thing" depends on the current layer of abstraction. processFile() is "one thing," but two things: fileMeetsTest() and either doSomething() or defaultAction(). I fear that the "one thing" aspect may confuse beginners who don't a priori understand the concept. –  Caleb Nov 30 '11 at 19:25

This is at a higher level of abstraction:

if (WeCanDoSomething(file))
{
   DoSomething(contents);
}
else
{
   DefaultAction();
} 

And this fills in the details.

boolean WeCanDoSomething(file)
{
    if FileExists(file)
    {
        contents = OpenFile(file);
        return (SomeTest(contents));
    }
    else
    {
        return FALSE;
    }

}
share|improve this answer

With regards to what this is called, it can easily develop into the arrowhead anti pattern as your code grows to handle more requirements (as shown by the answer provided at http://programmers.stackexchange.com/a/122625/33922) and then falls into the trap of having huge sections of codes with nested conditional statements that resemble a arrow.

See Links such as;

http://codinghorror.com/blog/2006/01/flattening-arrow-code.html

http://lostechies.com/chrismissal/2009/05/27/anti-patterns-and-worst-practices-the-arrowhead-anti-pattern/

There's plenty more on this and other anti patterns to be found on Google.

Some great tips Jeff provides on his blog regarding this are;

1) Replace conditions with guard clauses.

2) Decompose conditional blocks into seperate functions.

3) Convert negative checks into positive checks

4) Always opportunistically return as soon as possible from the function.

See some of the comments on Jeff's blog regarding Steve McConnells suggestions on early returns also;

"Use a return when it enhances readability: In certain routines, once you know the answer, you want to return it to the calling routine immediately. If the routine is defined in such a way that it doesn't require any further cleanup once it detects an error, not returning immediately means that you have to write more code."

...

"Minimize the number of returns in each routine: It's harder to understand a routine when, reading it at the bottom, you're unaware of the possibility that it returned somewhere abouve. For that reason, use returns judiciously--only when they improve readability. "

I always subscribed to the 1 entry/exit per function theory due to what I was taught 15 years or so ago. I feel this just makes code so much easier to read and as you mention more maintainable

share|improve this answer

This conforms to DRY, no-goto and no-multiple-returns rules, is scalable and readable in my opinion:

success = FileExists(file);
if (success)
{
    contents = OpenFile(file);
    success = SomeTest(contents);
}
if (success)
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
share|improve this answer
1  
Conforming to standards does not necessarily equal good code though. I'm currently undecided on this snippet of code. –  Brian Knoblauch Nov 30 '11 at 19:41
2  
The benefit of using a construct like this is that as the number of test increases the code doesn't start to nest more ifs inside other ifs. Also, the code to handle the unsuccessful case (DefaultAction()) is only in one place and for debugging purposes the code doesn't jump around helper functions and adding breakpoints to the lines where the success variable is changed can quickly show which tests passed (above the triggered breakpoint) and which ones haven't been tested (below). –  frozenkoi Dec 1 '11 at 1:17

I'd extract it to a separate method and then:

    if(!FileExists(file))
    {
        DefaultAction();
        return;
    }

    contents = OpenFile(file);
    if(!SomeTest(contents))
    {
        DefaultAction();
        return;
    }

    DoSomething(contents);

which also allows for

    if(!FileExists(file))
    {
        DefaultAction();
        return Result.FileNotFound;
    }

    contents = OpenFile(file);
    if(!SomeTest(contents))
    {
        DefaultAction();
        return Result.TestFailed;
    }

    DoSomething(contents);
    return Result.Success;            

then possibly you could remove the the DefaultAction calls and leave executing the DefaultAction for the caller:

Result OurMethod(file)
{
    if(!FileExists(file))
    {
        return Result.FileNotFound;
    }

    contents = OpenFile(file);
    if(!SomeTest(contents))
    {
        return Result.TestFailed;
    }

    DoSomething(contents);
    return Result.Success;            
}

void Caller()
{
    // something, something...

    var result = OurMethod(file);
    // if (result == Result.FileNotFound || result == Result.TestFailed), or just
    if (result != Result.Success)        
    {
        DefaultAction();
    }
}

I like Jeanne Pindar's approach as well.

share|improve this answer

The case shown in the sample code can usually be reduced to a single if statement. On many systems, the file-open function will return an invalid value if the file doesn't already exist. Sometimes this is the default behavior; other times, it must be specified via an argument. This means the FileExists test can be dropped, which can also help with race conditions resulting from file deletion between the existence test and file opening.

file = OpenFile(path);
if(isValidFileHandle(file) && SomeTest(file)) {
    DoSomething(file);
} else {
    DefaultAction();
}

This doesn't directly address the abstraction-level-mixing issue as it sidesteps the issue of multiple, unchainable tests entirely, though doing away with the file existence test is not incompatible with separating abstraction levels. Assuming that invalid file handles are equivalent to "false" and file handles close when they go out of scope:

OpenFileIfSomething(path:String) : FileHandle {
    file = OpenFile(path);
    if (file && SomeTest(file)) {
        return file;
    }
    return null;
}

...

if ((file = OpenFileIfSomething(path))) {
    DoSomething(file);
} else {
    DefaultAction();
}
share|improve this answer

I'm in agreement with frozenkoi, however, for C# anyways, I thought it would help to follow the syntax of the TryParse methods.

if(FileExists(file) && TryOpenFile(file, out contents))
    DoSomething(contents);
else
    DefaultAction();

//////////////////////////////////////////

bool TryOpenFile(object file, out object contents)
{
    try{
        contents = OpenFile(file);
    }
    catch{
        //something bad happened, computer probably exploded
        return false;
    }
    return true;
}
share|improve this answer

Another possibility if you don't like to see too many the else's is to drop the use of else altogether and throw in an extra return statement. Else is kind of superfluous unless you require more complex logic to determine if there are more than just two action possibilities.

Thus your example might become:

void DoABunchOfStuff()
{
    if(FileExists(file))
    {
        DoSomethingWithFileContent(file);
        return;
    }

    DefaultAction();
}

void DoSomethingWithFileContent(file)
{        
    var contents = GetFileContents(file)

    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }

    DefaultAction();
}

AReturnType GetFileContents(file)
{
    return OpenFile(file);
}

Personally I don't mind using the else clause as it states explicitly how the logic is supposed to work, and so improves readability of your code. Some code beautification tools however prefer to simplify to a single if statement to discourage nesting logic.

share|improve this answer

For this particular case, the answer is easy enough...

There is a race condition between FileExists and OpenFile: what happens if the file is removed?

The only sane way to deal with this particular case is to skip FileExists:

contents = OpenFile(file);
if (!contents) // open failed
    DefaultAction();
else (SomeTest(contents))
    DoSomething(contents);

This neatly solves this problem and makes the code cleaner.

In general: Try to rethink the problem and devise another solution which avoids the issue entirely.

share|improve this answer

To reduce nested IF:

1/ early return;

2/ compound expression (short-circuit aware)

So, your example may be refactored like this:

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
    return;
}
DefaultAction();
share|improve this answer

Of course you can only go so far in scenarios like these, but here's a way to go:

interface File<T> {
    function isOK():Bool;
    function getData():T;
}

var appleFile:File<Apple> = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

You may want additional filters. Then do this:

var appleFile = appleStorage.get(fileURI, isEdible);//isEdible is of type Apple->Bool and will be used internally to answer to the isOK call
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Although this might make sense just as well:

function eat(apple:Apple) {
     if (isEdible(apple)) 
         digest(apple);
     else
         die();
}
var appleFile = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(appleFile.getData());
else
    cry();

Which is the best? That depends on the real world problem you're facing.
But the thing to take away is: you can do a lot with composition and polymorphism.

share|improve this answer

How about this solution:

content = NULL; //I presume OpenFile returns a pointer 
if(FileExists(file))
    contents = OpenFile(file);
if(content != NULL && SomeTest(contents))
    DoSomething(contents);
else
    DefaultAction();

I made the assumption that OpenFile returns a pointer, but this could work also with value type return by specifying some default value not returnable (error codes or something like that).

Of course I don't expect some possible action via the SomeTest method on NULL pointer (but you never know), so this could be also seen as an extra check for NULL pointer for the SomeTest(contents) call.

share|improve this answer

Clearly, the most elegant and concise solution is to use a preprocessor macro.

#define DOUBLE_ELSE(CODE) else { CODE } } else { CODE }

Which allows you to write beautiful code like this:

if(FileExists(file))
{
    contents = OpenFile(file);
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    DOUBLE_ELSE(DefaultAction();)

It might be difficult to rely on automatic formatting if you use this technique often, and some IDEs might yell at you a little bit about what it erroneously supposes is malformed. And as the saying goes, everything is a tradeoff, but I suppose it's not a bad price to pay to avoid the evils of repeated code.

share|improve this answer
3  
This is so horrible, I just had to upvote it :D –  back2dos Dec 1 '11 at 18:20

I saw a lot of examples with "return" which I use too but sometimes I want to avoid creating new functions and use a loop instead:

while (1) {
    if (FileExists(file)) {
        contents = OpenFile(file);
        if (SomeTest(contents)) {
           DoSomething(contents);
           break;
        } 
    }
    DefaultAction();
    break;
}

If you want to write less lines or you hate infinite loops as me, you can change the loop type to "do ... while(0)" and avoid the last "break".

share|improve this answer

What's wrong with the obvious

if(!FileExists(file)) {
    DefaultAction();
    return;
}
contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}        
DoSomething(contents);

It seems pretty standard to me? For that kind of big procedure where lots of little things have to happen, failure of any of which would prevent the latter. Exceptions make it a bit cleaner if that's an option.

share|improve this answer

Your code is ugly because you are doing too much in a single function. You want to either process the file or take the default action, so start by saying that:

if (!ProcessFile(file)) { 
  DefaultAction(); 
}

Perl and Ruby programmers write processFile(file) || defaultAction()

Now go write ProcessFile:

if (FileExists(file)) { 
  contents = OpenFile(file);
  if (SomeTest(contents)) {
    processContents(contents);
    return true;
  }
}
return false;
share|improve this answer

protected by gnat Jul 2 at 13:27

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.