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 NaorNov 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.
–
CalebNov 30 '11 at 19:12

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:

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.

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

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)

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:

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

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.

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.

My cursor is hovering over the accept button... just to spite all the purists. Oooohh the temptation :D
–
BenjolNov 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.
–
hlovdalNov 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.
–
hlovdalNov 30 '11 at 19:48

@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 HudecDec 2 '11 at 7:11

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.
–
AbyxNov 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.
–
grossvogelNov 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 KnoblauchNov 30 '11 at 19:34

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.

+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.
–
CalebNov 30 '11 at 19:25

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.

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

Conforming to standards does not necessarily equal good code though. I'm currently undecided on this snippet of code.
–
Brian KnoblauchNov 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).
–
frozenkoiDec 1 '11 at 1:17

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.

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:

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.

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.

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.

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.

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.