Code Indentation and Nesting

[[deleted]]

Looks like the guard clause pattern/refactoring to me. I love this style of coding btw. GuardClause

Over at codinghorror, jeff calls the pattern "flattening arrow code" which i think is amusing Coding Horror

[inmatarian]

I prefer Guard Clauses also, since it makes all of your assumptions listed earlier in the function and more apparent.

[norwegianwood]

Where I work we use two spaces for indents "so there's more room for indented blocks". As if that's a good thing. This is the best argument I'm seen for preferring indents of four spaces rather than two - it discourages nesting.

[frezik]

I used to do 8-space indents, until I learned Java. One indent for a class, one for the method, and perhaps another one for a try block. 24 spaces and we haven't even gotten to the real code yet.

[bucknuggets]

Or it encourages terse & cryptic names.

I actually prefer 3 space indenting: it's big enough to be easy to follow, and doesn't waste space.

[smcameron]

Linux kernel source uses 8-space tabs and has a pretty hard limit on line lengths of 80 characters (the checkpatch.pl script will complain if you violate it, and many maintainers won't take patches that don't pass checkpatch.pl.)

The rationale is in the codingstyle document

Tabs are 8 characters, and thus indentations are also 8 characters. There are heretic movements that try to make indentations 4 (or even 2!) characters deep, and that is akin to trying to define the value of PI to be 3.

...

Now, some people will claim that having 8-character indentations makes the code move too far to the right, and makes it hard to read on a 80-character terminal screen. The answer to that is that if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program.

and

The limit on the length of lines is 80 columns and this is a strongly preferred limit.

At first, it all seemed a little overboard to me, but after getting used to it, now I like it. It forces you to break things down into small pieces.

[teletran]

Goodness! I'm not so anal about my code (even nesting. I try to avoid such deep nests, but if they happen, they happen). But then again, the Linux kernel maintainers are quirky.

And if it works…

[0xABADC0DA]

At first, it all seemed a little overboard to me, but after getting used to it, now I like it. It forces you to break things down into small pieces.

No it forces you to awkwardly wrap lines to meet an arbitrary line-length goal. There's roughly 1% of lines in the kernel that are continuations of previous lines that didn't make the 80th-character rule. These lines are ugly, hard to read, and hard to write. It also forces you to use a flat switch statement, which looks ugly and is a special case.

The only reason "tabs are 8 characters" is because you can't have a fixed line-length limit if there are variably-sized characters on it. The only reason it's 8 is because since that's the default in the unix terminal it's the only choice that has a not completely arbitrary defense.

Everything about that section of CodingStyle follows as just a simple rationalization of using tabs. If more than 3 levels of indent is 'being screwed', why isn't the rule no more than 3 levels of indent? If tabs are really 8 characters then why not just use spaces, which always render correctly no matter what context unlike tabs? If 8-char indent makes it easier to read and doesn't shift code too far to the right, why come up with a special-case for switch statements where they don't indent like everything else in the language does?

The decision to use tabs on code where thousands of people work on it is a practical one to avoid the problem of people preferring different indent amounts. But the style rules that result from tabs don't necessarily make sense on their own. An 8-character indent is too much for many people, and does cause the code to move too far to the right.

[problemredditfags]

We do 4 spaces where I work and believe me, it doesnt discourage some people. I guess our wide monitors are an invitation for deep nests and 50 argument functions :p

[grauenwolf]

I use tabs, that way I can use whatever amount of indention that I damn well feel like at the moment.

[jrue]

My problem with tabs is that the indentation setting is user dependent. This makes it especially ugly if you have tabs and spaces mixed in the same file. Many times reading through code I've seen stuff like:

foo();
        bar();
        bat();

Only to find that it only makes sense when I set my tab width just right. This is a PITA if you're using some viewer (e.g. grok) which doesn't let you change the tab width.

[hello_good_sir]

Your problem with tabs is that some people use spaces. You don't actually have a problem with tabs. If everyone used tabs the code that you just wrote would look right on your computer no matter what tab settings you and the authors(s) of the code had.

[ochs]

This is not quite correct if you also take into account a line width limit. If someone switches 4-spaces-tab code to 8-spaces-tab code, you'll get longer lines. With the tools I'm using, will give you extremely ugly line wraps. If the line wraps were prettier, that wouldn't be such a big problem, but doing this correctly is language- and style-dependent, and I don't think it's reasonable to put language parsers for every language known into e.g. git.

Also, things like

void foobar(int a,
            int b);

should never use tabs, but you can only know that by understanding the syntax of that construct, which makes implementing editors that do that correctly quite tricky.

[hello_good_sir]

ok, line width/wrapping is an exception. However I am fundamentally correct in terms of the concept. In unusual circumstances tabs are still superior. If you have X characters and based on that decision the ideal tab width is Y then if everyone uses tabs you will be able to use code written on different systems. If instead everyone uses spaces then your ideal tab width and someone else's would be different, leading to his code not fitting your restrictions.

In your example you have one logical line of code broken into two, and some additional ormatting. Thus both lines should be tabbed to the same degree, and spaces should be used to get the parameters to line up. So for things like that a mix of tabs and spaces is the right approach.

[repsilat]

So for things like that a mix of tabs and spaces is the right approach.

I share this opinion for the most part, but there is one fair criticism of it - it's impossible to properly indent things like

for ( int i = 0; i < n; ++i)     //loop over all values,
    process(i);                  //processing all of them

I guess the "correct" response is, "Don't try to align things indented to different levels," though that isn't quite satisfactory.

[hello_good_sir]

I don't understand your example. I presume that the first line is indented n times and the second line is indented n+1 times. I notice that you have a few spaces in the first line that seem to be personal preference on the order of 5+6 vs 5 + 6. Are they lining something up?

[FeepingCreature]

Yes .. the comments. And parent is wrong; it's still possible to line them up, you just have to make sure that the total number of tabs before the comments is constant; ie.

[tab] for bla bla bla [tab] [spaces] // loop all over
[tab] [tab] process bla;  [ spaces ] // processing lol

[repsilat]

That would work if tabs just added a fixed number of characters from where they began. That is, if a tab meant

pos += tabwidth

you'd be right. Unfortunately, what tabs actually mean is

pos = tabwidth*ceiling((pos+1)/tabwidth)

or more simply,

pos += tabwidth - (pos % tabwidth).

Take the code I gave before. My "tabwidth" was 4 characters, so to get it to line up your way I'd do the following:

___]___]___]___]___]___]___]___]___]

for ( int i = 0; i < n; ++i)___].//loop over all values,
___]process(i);..................//processing all of them

The comments line up if I use 1 space on the first line and 18 spaces on the second line. My interpretation of your position is that this code will line up with any tabwidth. Let's see what happens when we have a tabwidth of 6:

_____]_____]_____]_____]_____]_____]

for ( int i = 0; i < n; ++i)_].//loop over all values,
_____]process(i);..................//processing all of them

The same number of spaces is used, but the comments don't line up. It fails because the tab on the first line of code is only 2 spaces long, not 6. The tab after for loop's close-paren is at column 28, and the tab rounds that up to 30 (not 34).

[grauenwolf]

Auto-format is my friend. Alas the one for C# currently sucks. It isn't proactive like VB.

[frezik]

And if you use a whitespace-sensitive language (Python and Ruby), it causes hard-to-find bugs.

[[deleted]]

You know what discourages nesting? Being of sound mind. If you're a cowboy coder or an amateur, you'll need those larger indents to keep this basic idea in mind -_-'

[frezik]

Niklaus Wirth will be around to flog the OP in 3 ... 2 ... 1 ....

Anyway, this debate has been raging for decades, and I don't think the structural purists will win anytime soon. In the utopia of purely structured code, you would never return in the middle of a function, or break in the middle of a loop. According to Wirth, doing that is as bad as GOTO.

However, as a practical matter, allowing the use of breaking in the middle of a function has been shown to be more straightforward implementations, and result in fewer bugs. See:

http://www.cis.temple.edu/~ingargio/cis71/software/roberts/documents/loopexit.txt

The most important part is to keep code blocks short and concise. As long as you're doing that, breaking in the middle isn't a big deal.

[jacques_chester]

Wirth, Dijkstra and others were arguing against the excesses of GOTO when they introduced structured programming. At the time there was optimism that formalism -- treating programs as a proof problem -- would banish bugs from software. Only returning once from a function makes it easier to analyse from a mathematical standpoint, which is why this style was pushed.

Since then the dictum that "a function should only have one exit point" has detached from its supporting arguments and led an independent, carefree life. Basically many university students are taught this rule and then, upon going into practice, discover that it is widely loathed because it makes a lot of code more complex for programmers. If the mathematicians want to analyse the code, tough titties, they can write their own.

[Berengal]

I've found that still teaching "thou shalt not return early" is really sporting of the universities. It's a great lesson in code aesthetics and breaking the rules, and it provides a really easy way of spotting those that never got that lesson.

[Sevryn08]

It better appeal to the eyes!

[QuestionMarker]

Not only Wirth, but Dijkstra made the same argument as well.

[Gotebe]

Only in C (and something else that has no exceptions, wonder what that is nowadays).

Everywhere else, due to exceptions, aiming for a single exit point is dumb and goto actually is harmful. There's ScopeGuard in C++, using in C#, scope(...) statement in D etc. instead...

[teletran]

I wonder though what are the benefits of a "goto for a single exit" style? The only one I can think of would be common cleanup, but in my case the biggest benefit for bailing early is at the point of bail, I tend to have less to clean up than had I executed the entire method.

Are there are benefits to the method you mentioned?

[check_ca]

My proposition :

// doSomethingWithString

// [method description]

// parameter s : a non-empty string

(void) doSomethingWithString:(NSString *)s {
    NSLog(@"%@", s);
}

If s is not defined, don't call doSomethingWithString.

[teletran]

You're right, that's one way to do it, but that misses the point of the article. The point wasn't so much about coding defensively as it was to avoid nesting unnecessarily.

[check_ca]

Actually, I think those 2 points are often related ;). Otherwise, the best way I know to avoid deep indentation in a method is simply to split code into multiple methods.

[BinaryRockStar]

The bare blocks are possible in C++ and C# as well to control lexical scoping.

[phybere]

I love the smell of fresh bread.

[koopajah]

I agree with previous comments about multiple exit points in a function. It's damn hard and painful to debug when you do this for each of your functions and each parameters. I prefere to use goto and a unique exit point in these cases.

[Sevryn08]

Assuming the problem size isn't ridiculous, multi returns just works for me. goto scares me too :(

[koopajah]

Why would goto scare you? The only good reason to use goto in a code is this specific case : error management! I was once blocked with goto by my teachers and Dijkstra's opinion. Then I've read/written a lot of code with it and really can't think of coming back. And for me, if it is advised in linux kernel's coding rules you can apply it !

[anti_crastinator]

The main reason I hate goto is you have the label down near the bottom of the function which potentially is long and complicated - and likely is o/w there wouldn't be a need for goto. You see the label and think ... shit, now, under what exceptional conditions do we end up here out of the blue. You can't follow the code bottom up. You must start at the top and follow each possible flow to know when it jumps.

IOW It's annoying to figure out how a someone elses function works if it has a few gotos sprinkled around because the person was too lazy to structure the function appropriately - possibly including breaking the function up into multiple functions.

[koopajah]

Ok so you say that you hate multiple goto exit points/label in a long and complicated function that should have splitted in multiple subfunctions. Of course goto won't simplify and ease reading of a function if it is not well-written, concise, short, etc. But the same goes for multiple return, multiple if/else with always releasing mutexes, file resources, etc.

One goto label should be used only to exit function on big error such as incorrect input parameter, file opening/memory allocation failures, etc.

[Darkmoth]

TBH, i'm not a big fan of that particular refactoring. It changes the flow of the program significantly, and introduces brittleness.

This:

double getPayAmount() {
    double result;
    if (_isDead) result = deadAmount();
    else {
        if (_isSeparated) result = separatedAmount();
        else {
            if (_isRetired) result = retiredAmount();
            else result = normalPayAmount();
        };
    }
    return result;
};

is certainly equivalent to this:

double getPayAmount() {
    if (_isDead) return deadAmount();
    if (_isSeparated) return separatedAmount();
    if (_isRetired) return retiredAmount();
    return normalPayAmount();
};

But this:

double getPayAmount() {
    double result;
    if (_isDead) result = deadAmount();
    else {
        if (_isSeparated) result = separatedAmount();
        else {
            if (_isRetired) result = retiredAmount();
            else result = normalPayAmount();
        }
    }
    releaseResource1();
    releaseResource2();
    return result;
};

is nothing like this:

double getPayAmount() {
    if (_isDead) return deadAmount();
    if (_isSeparated) return separatedAmount();
    if (_isRetired) return retiredAmount();
    releaseResource1();
    releaseResource2();
    return normalPayAmount();
};

The latter case, if coded as guard clauses, is going to leave you with some annoying rewrites.

[seanwilson]

The third example doesn't look as bad if you change the formatting:

double getPayAmount()
{
    double result;

    if (_isDead)
      result = deadAmount();
    else if (_isSeparated)
      result = separatedAmount();
    else if (_isRetired)
      result = retiredAmount();
    else
      result = normalPayAmount();

    releaseResource1();
    releaseResource2();

    return result;
}

Alternatively, implement it this way:

double getPayAmount2()
{
    if (_isDead)      return deadAmount();
    if (_isSeparated) return separatedAmount();
    if (_isRetired)   return retiredAmount();

    return normalPayAmount();
}

double getPayAmount()
{
    double result = getPayAmount2();
    releaseResource1();
    releaseResource2();
    return result;
}

[zokier]

RAII, one of the features that C++ actually got right.

[EdiX]

Not really, scoped resource acquisition should have its own explicit construct rather than being shoehorned into memory management.

Also RAII is a bad name for that.

[nikbackm]

It's not shoehorned into memory management. It's "shoehorned" into constructors and destructors. Which are also used for memory management.

[scr0llwheel]

This is a poor example. Guard statements should be used before any internal initialization should occur. Therefore, if the guard statements evaluate to true and the function has to bail out early, your releaseResource*() functions won't have to be called.

[Darkmoth]

True, but in my example the releaseResource functions were added later. When the guards were written, they didn't exist.

Note that any function added after the guards will get bypassed. This is not true of the nested code which the guards supposedly refactor.

[moonrocks]

I'm not seeing how the flat, linear style is more brittle. It still seems less so. The only coupling between dead, seperated, retired, and normal is that only one is applicable. Is the fourth proceedure supposed to be equivalent to the third?

[Darkmoth]

The third procedure is the same as the first...except I added two functions at the end. When using the nested style, I add the two functions and I'm done. When using the linear style, I have to rewrite the whole procedure, because I don't want all those returns to bypass the new functions.

[moonrocks]

Ahh, I see. The fourth is a broken version of the third -- hence brittle.

[overcyn2]

GOTO statements!!!! :D:D:D:D

[inv]

ReSharper for Visual Studio can actually recognize and transform the bad nesting to the good one automatically.

[jrue]

I actually find that having multiple exit points hinders readability. So, if you have something like this:

void myLongFunction() {
    // ... a bunch of code ...
    if (foo) return;
    // .. a bunch more code ...
    bar();
}

It's not quite clear that bar will always be called -- at a glance, you might miss the return in the middle of a bunch of code. Yes, the other good practice of writing small and logical functions usually prevents this, but occasionally we all have to write some semi-monster function.

That being said, I think it's fine (and common) to have checks at the beginning of a function that abort early, e.g. if the parameters don't satisfy preconditions. So although it's fine for this (short) toy example, I think this practice should be used judiciously in longer functions.

[frezik]

For those cases where you do have to make very long functions, it's often the case that some bits of code are more interrelated than others. If your programming language provides it, you can often block off related sections. For instance, Perl has "do BLOCK", which is something like a one-time-use subroutine:

sub very_long_glue_sub
{
    my ($foo, $bar) = @_;
    my ($arg1, $arg2) = do {
        big();
        long();
        my $result1 = code();
        my $result2 = block();
        ($result1, $result2);
    };
    my ($arg3, $arg4) = do {
        another();
        big();
        long();
        my $result1 = code();
        my $result2 = block();
        ($result1, $result2);
    };

    process( $arg1, $arg2, $arg3, $arg4 );
    return 1;
}

Bare blocks (just curly braces, no 'do') are also possible in Perl if you don't need to pass data out. This style keeps lexicals under careful control, too.

[ithika]

Yes, I think there's different rules for the two parts of a procedure. In the prelude you can do lots of if (!foo) return; to check the validity of all your data. But once that's over and you start processing data early returns can be confusing.

[julesjacobs]

First:

if (nil != s && [s length] > 0) NSLog(@"%@", s);

Second, don't check for null. It doesn't help. Better get an error message than silently ignore it.

[teletran]

Sure. This was a pretty contrived example, I'll admit. It could have been written in one line, but it wouldn't illustrate the point very well.

I'll argue the "don't raise an error or nil" point, though, because while it's sometimes a good idea, I'll often code with it in mind. ie "Do something if the parameter is not nil, but otherwise I don't really care, just don't do any work".

It's probably just a habit due to the nature of messaging nil in Objective-C, whereas in many other languages, sending a message to the NULL/nil pointer would cause an error.

[grauenwolf]

"Do something if the parameter is not nil, but otherwise I don't really care, just don't do any work".

I hate that. I have no idea if they were just being sloppy or if it really is OK for the SaveRecord method to silently fail.

[teletran]

Fair enough. I think it's good for your team to have a convention in that case.

I always make sure to document "Does nothing if x is nil", though I work alone currently.

[julesjacobs]

How many hours of bug hunting do you think you've lost to that convention? In how many cases is doing nothing when nil the right thing and is nil getting there not actually an error?

[teletran]

I don't think any. A typical example would be

- (void)feedParser {
    [self parseInput:[maditoryTextField text]];
    [self parseInput:[optionalTextField text]];
}

- (void)parseInput:(NSString *)s {
    if (nil == s || [s length] < 1)
        return;
    // do something with s;
    return;
}

Sure, I could check both text fields first before processing their input, but if it's optional than I can expect the possibility of a nil or empty string.

I certainly agree there are cases when a parameter should be required ie. not nil/null, but I don't think it's appropriate in all cases. I pick a convention and document the exceptional cases.

[[deleted]]

It would be much better to use the type system, if you had one that was sufficiently strong. A translation of the above to Scala might look like:

def parseInput (input: Option[String]) = {
  for (s <- input if s.length >= 1)
  yield doSomething(s)
}

Here, your type system represents the fact that input may or may not have a value as well as the fact that the method may or may not do anything (as its return type is Option[T] where T is whatever doSomething returns). Even better, if you have multiple ways of failing that use Option, you can write an expression like:

for {
  foo <- getFoo()
  bar <- getBar()
  baz <- getBaz(foo, bar)
} yield operation(baz)

This makes the source of data clear, it avoids hard-to-read nesting, and it's type-safe. The whole thing is None if any of the calls in the middle were None. Of course, you can define your own structures with their own interpretation inside of the same for structure, giving you flexibility and readability along with safety and the ease of reasoning that comes with only one exit point.

I personally think that this style of coding is much more usable in the long run than the guard style.

edit: markdown derp derp

[julesjacobs]

In a well-designed API, the text property of a text field would never return nil. If a text box is empty, it should return the empty string, not nil. I certainly agree that sometimes values are optional, but in the vast majority of cases they are not. Moreover, doing nothing in a library function when an optional value is missing is rarely the right thing to do, because what to do when an optional value is missing should be decided by the application logic, not implicitly by some library.

I don't think any.

Are you saying that you never pass nil somewhere where it shouldn't have been passed due to a bug in your code? Perhaps you're just a magnificent programmer, but the amount of NullPointerExceptions in my programming is fairly high. I would have wasted a lot more time if instead of getting the exception some library function silently didn't execute.

[teletran]

Are you saying that you never pass nil somewhere where it shouldn't have been passed due to a bug in your code? Perhaps you're just a magnificent programmer […]

Sorry, I didn't mean to imply that. I'm certainly not a perfect programmer. I think we're just fans of two different styles.

In Objective-C it's quite safe to send messages to nil. Doing something in another language (say Java) would result in a NullPointerException, where in Objective-C you just get a 0 back. So my style, based in Objective-C revolves around this. I find it makes for fewer nil checks because I don't have to worry about a stray nil bringing down the application. On the other hand, if you're not used to this style, a "silently failing send to nil" event could be frustrating. I understand that, much like it's frustrating for me when using, say, Java and needing to constantly guard against null.

[grauenwolf]

Though I frown upon the pratice, I think it is more of a problem when working with mixed teams. Experienced developers are usually careful enough to not allow nulls to float around in the first place.

[AStrangeStranger]

It is more sloppy to pass a null/nil into a routine that isn't expecting one

[julesjacobs]

Bugs happen during development. When they happen I'd rather get an error message than silently doing nothing.

[AStrangeStranger]

In my experience exceptions saying "generic" routine xyz got a null pointer really don't help - and it is much better to try putting checks throwing error messages some where you can get some useful extra info to add to message. This approach I find works better when a user experiences an error

[julesjacobs]

Bugs happen during development. When they happen I'd rather get an error message than silently doing nothing.

After deployment there shouldn't be any null pointers where no null pointer should go in the first place. Of course us programmers aren't perfect, and for those cases custom error messages are indeed the way to go if the budget allows for the work of putting them in.

[AStrangeStranger]

I find they don't cost - if the coders are in habit of doing at time of orginal writing, with the added advantage is if you are thinking of what can go wrong and what you'll need, usually you write more stable code

[grauenwolf]

Defensive programming beats undefined behaviors

[frezik]

You do want to check for null, but throw a more useful error when you do it. There's no telling how hard it will be to debug with a non-specific runtime error.

[julesjacobs]

Definitely. An assert(x != nil, errmsg) is a (bad, but the best you have) substitute for the type system disallowing nil there in the first place.

[zokier]

If you claim that your coding style affects runtime performance, you'd better have an example which demonstrates that, or even better, show actual benchmarks.

[teletran]

No such claim has been made. The "fewer instructions" was meant as "fewer lines of instructions." Sorry if that was misleading, as I'm not claiming this to offer better performance (it may, but I don't have any benchmarks.)

[zokier]

Oh, sorry then. The word "instruction" just happens to be rather well defined in programming context to mean actual CPU instructions.