175

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...

12
  • 8
    @Emmad Kareem : two DefaultAction calls violate DRY principle
    – Abyx
    Nov 30, 2011 at 11:52
  • Thanks for your reply, but I think that it is OK, except for not using try/catch since there are may be errors that don't return results and would cause abends (depending on your programming language).
    – NoChance
    Nov 30, 2011 at 12:02
  • 22
    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, 2011 at 15:37
  • 6
    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, 2011 at 19:12
  • 1
    You could reference this book "Refactoring: Improving the Design of Existing Code".There are several sections about the if-else structure,truly useful practice.
    – Vacker
    Dec 1, 2011 at 1:48

24 Answers 24

107

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.

6
  • 98
    +1 for multiple returns - when methods are made sufficiently small, this approach worked best for me
    – gnat
    Nov 30, 2011 at 11:31
  • Not a big fan of the multiple returns, although I do use it occasionally. It's quite reasonable on something simple, but just doesn't scale well. Our standard is to avoid it for all but crazy simple methods because methods tend to grow in size more than they shrink. Nov 30, 2011 at 19:37
  • 4
    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, 2011 at 5:37
  • I'd recommend reversing the logic on the 2nd solution: { if(file exists) { set contents; if(sometest) { return contents; } } return null; } It simplifies flow and reduces the number of lines.
    – Wedge
    Dec 1, 2011 at 7:09
  • 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, 2011 at 19:37
58

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)

6
  • This is how I would do it.
    – Anthony
    Dec 1, 2011 at 7:08
  • 14
    Pretty gross to put so much code in an if statement, in my opinion.
    – moteutsch
    Dec 1, 2011 at 22:07
  • 15
    I, on the contrary, like this kind of "if something exists and meets this condition" statement. +1
    – Gorpik
    Dec 2, 2011 at 12:40
  • I do too! I personally dislike the way people use multiple returns some premises are not met. Why don't you invert those ifs and execute your code if they are met?
    – klaar
    Oct 28, 2015 at 7:43
  • 1
    @klaar Early exiting is easier to understand, particularly for 3 or more condition branches. Mar 13, 2018 at 9:27
27

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.

5
  • 8
    +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, 2011 at 2:10
  • 3
    @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, 2011 at 5:59
  • 1
    This is a great answer, thank you. And @MrMoose: "arrowhead anti pattern" possibly answers my first bullet, so yes, do post it. I can't promise I'll accept it, but it deserves votes!
    – Benjol
    Dec 1, 2011 at 6:03
  • @outis. Thanks. I've added answer. The arrowhead anti pattern certainly is relevant in hlovdal's post and his guard clauses work well at getting around them. I don't know how you could answer the second bullet point to this. I'm not qualified to diagnose that :)
    – Mr Moose
    Dec 1, 2011 at 7:53
  • 4
    +1 for "test exceptions, not the normal case."
    – Roy Tinker
    Dec 1, 2011 at 17:54
25

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.

11
  • 12
    My cursor is hovering over the accept button... just to spite all the purists. Oooohh the temptation :D
    – Benjol
    Nov 30, 2011 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, 2011 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, 2011 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, 2011 at 21:29
  • 4
    @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, 2011 at 7:11
12
function FileContentsExists(file) {
    return FileExists(file) ? OpenFile(file) : null;
}

...

contents = FileContentExists(file);
if(contents && SomeTest(contents))
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
7
  • or go the extra male and create an additional FileExistsAndConditionMet(file) method...
    – UncleZeiv
    Nov 30, 2011 at 12:08
  • @herby SomeTest can have the same semantics as file existence if SomeTest checks file type, for example checks that .gif is really GIF file.
    – Abyx
    Nov 30, 2011 at 13:11
  • 1
    Yes. Depends. @Benjol knows better.
    – herby
    Nov 30, 2011 at 13:18
  • 3
    ... of course I meant "go the extra mIle"... :)
    – UncleZeiv
    Nov 30, 2011 at 15:36
  • 2
    That is taking ravioli to the extremities even I don't go to (and I am extreme in this)... I think now it is nicely readable considering the contents && f(contents). Two functions to save one else?!
    – herby
    Nov 30, 2011 at 15:42
12

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.
5
  • 19
    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, 2011 at 10:36
  • Not if you restrict it to as local as possible scope. (function () { ... })() in Javascript, { flag = false; ... } in C-like etc.
    – herby
    Nov 30, 2011 at 10:43
  • +1 for the exception logic, which could very well be the most suitable solution depending on the scenario. Nov 30, 2011 at 13:48
  • 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, 2011 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. :-) Nov 30, 2011 at 19:34
11

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;
    }
}
11

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.

4
  • 5
    +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, 2011 at 19:25
  • 1
    It's a good goal... That's all I have to say about that... ;-) Nov 30, 2011 at 19:40
  • 1
    I don't like implicitely passing arguments as instance variables like that. You get full of "useless" instance variables and there are many ways to botch up your state and break the invariants.
    – hugomg
    Dec 1, 2011 at 16:40
  • @Caleb, ProcessFile() is indeed doing one thing. As Karl says in his post, it is using a test to decide which action to take, and deferring the actual implementation of the action possibilities to other methods. If you were to add many more alternate actions, the single purpose criteria for the method would still be met so long as no nesting of logic occurs in the immediate method.
    – S.Robins
    Dec 2, 2011 at 11:34
7

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();
}
6
  • 1
    Conforming to standards does not necessarily equal good code though. I'm currently undecided on this snippet of code. Nov 30, 2011 at 19:41
  • this just replaces 2 defaultAction(); with 2 identical if conditions and adds a flag variable which imo is much worse.
    – Ryathal
    Nov 30, 2011 at 19:57
  • 3
    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, 2011 at 1:17
  • 2
    Yeeaah, I'm kind of liking it, but I think I'd rename success to ok_so_far :)
    – Benjol
    Dec 1, 2011 at 6:05
  • This is very similar to what I do when (1) the process is very linear when everything goes right and (2) you would otherwise have the arrow anti-pattern. I, however, try to avoid adding an extra variable, which is usually easy if you think in terms of the prerequisites for the next step (which is subtly different than asking if an earlier step failed). If the file exists, open the file. If the file is open, read the contents. If I have contents, process them, else do the default action. Jan 12, 2018 at 19:14
6

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 https://softwareengineering.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

3

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.

3

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.

2

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.

2

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();
}
2

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;
}
1

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;
1

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.

1

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.

0

I realse that this is an old question, but I noticed a pattern that has not been mentioned; mainly, setting a variable to later determine the method/s you would like to call (outside of the if...else...).

This is just as another angle to look at to make the code easier to work with. It also allows for when you may want to add another method to be called or change the appropriate method that needs to be called in certain situations.

Rather than having to replace all mentions of the method (and maybe missing some scenarios), they are all listed at the end of the if...else... block and are simpler to read and alter. I tend to use this when for example, several methods may be called, but within the nested if...else... a method may be called in several matches.

If you set a variable that defines the state, you could have many deeply nested options, and update state when something is to be (or not to be) performed.

This could be used as in the example asked in the question where we are checking if 'DoSomething' has occurred, and if not, perform the default action. Or you could have state for each method you may want to call, set when applicable, then call the applicable method outside of the if...else...

At the end of the nested if...else... statements, you check state and act accordingly. This means you only need a single mention of a method instead of all the locations that it should be applied.

bool ActionDone = false;

if (Method_1(object_A)) // Test 1
{
    result_A = Method_2(object_A); // Result 1

    if (Method_3(result_A)) // Test 2
    {
        Method_4(result_A); // Action 1
        ActionDone = true;
    }
}

if (!ActionDone)
{
    Method_5(); // Default Action
}
0

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();
0

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".

0

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.

0

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.

5
  • For some people, and in some languages, preprocessor macros are evil code :)
    – Benjol
    Dec 1, 2011 at 6:06
  • @Benjol You said you were open to evil suggestions, no? ;) Dec 1, 2011 at 6:11
  • yes, absolutely, it was just wrt your "avoid the evils" :)
    – Benjol
    Dec 1, 2011 at 6:23
  • 4
    This is so horrible, I just had to upvote it :D
    – back2dos
    Dec 1, 2011 at 18:20
  • Shirley, you're not serious!!!!!! Dec 1, 2011 at 18:40
-1

Since you asked out of curiosity, and your question is not tagged with a specific language (even though it's clear you had imperative languages in mind), it may be worth adding that languages supporting lazy evaluation allow for a complete different approach. In those languages, expressions are only evaluated when needed, so you can define "variables" and use them only when it makes sense to do so. For example, in a fictional language with lazy let/in structures you forget about flow control and write:

let
  contents = ReadFile(file)
in
  if FileExists(file) && SomeTest(contents) 
    DoSomething(contents)
  else 
    DefaultAction()

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