Angband Coding Style

[calris]

Sorry, I was referring to code folding; I use it all the time. I was trying to give an example of why I prefer to have a line break between the } and the else. Otherwise, I totally agree with what you are saying.

I you need to fold code, there is deeper problem. I'm a personal fan of Linux's 'the length of a function should be inversely proportional to it's complexity' rule:

The maximum length of a function is inversely proportional to the
complexity and indentation level of that function. So, if you have a
conceptually simple function that is just one long (but simple)
case-statement, where you have to do lots of small things for a lot of
different cases, it's OK to have a longer function.

However, if you have a complex function, and you suspect that a
less-than-gifted first-year high-school student might not even
understand what the function is all about, you should adhere to the
maximum limits all the more closely. Use helper functions with
descriptive names (you can ask the compiler to in-line them if you think
it's performance-critical, and it will probably do a better job of it
than you would have done).

There a a lot of overly complex functions in Angband with duplicated snippets of code all over the place.

• Complex functions being longer that a screen long are difficult for mere mortals to process - Anything longer than 40 lines (be it a paragraph of text, a C function, a part of mathematical proof) pushes the limits of human memory and comprehension
• Long, complex functions are daunting for new programs and create a barrier for entry for new coders that want to get involved. Keeping the barrier to entry for new coders low is 'A Good Thing'(tm)
• Duplicated code snippets are prone to introduce bugs if changes are made that are not captured in ALL versions of the snippet
• The code ends up having insane levels of indent, which pushes the 80 column limit to the max

The entire Linux kernel, nearly 17 million lines of code, is (mostly) less than 80 columns using 8 space tabs. Angband has a hard time using 4 space tabs.

[molybdenum]

I you need to fold code, there is deeper problem.

I'd say folding is more of a visual preference, hiding stuff I don't want to look at or constantly scroll past.

I'm a personal fan of Linux's 'the length of a function should be inversely proportional to it's complexity' rule:

...

There a a lot of overly complex functions in Angband with duplicated snippets of code all over the place.

...

The entire Linux kernel, nearly 17 million lines of code, is (mostly) less than 80 columns using 8 space tabs. Angband has a hard time using 4 space tabs.

This is new concept for me, using certain aspects of code style to enforce simpler implementation. For example, restricting line length to 80 drives me nuts, but that's probably because I work with code that's particularly verbose. Just today I was having to throw around this constant: NSURLSessionAuthChallengeCancelAuthenticationChall enge (54 characters, and even the forum doesn't like that length). Just what I'm used to, I guess.

[calris]

I'd say folding is more of a visual preference, hiding stuff I don't want to look at or constantly scroll past.

I'll happily fold functions. But even this can suggest that the source file is becoming too long and the function deserves to be somewhere else. main-win.c is a classic example - over 5000 lines - sound is less that 200 lines buried right in the middle

This is new concept for me, using certain aspects of code style to enforce simpler implementation. For example, restricting line length to 80 drives me nuts

It used to drive me nuts too - But once you get into a mindset of 'my function needs to fit inside 80 columns and 30 lines' you develop a knack for taking what would otherwise been an insanely complicated concept to code and break it down into logical chunks.

but that's probably because I work with code that's particularly verbose. Just today I was having to throw around this constant: NSURLSessionAuthChallengeCancelAuthenticationChall enge (54 characters, and even the forum doesn't like that length). Just what I'm used to, I guess.

That is just EVIL - compare these

• NSURLSessionAuthChallengeCancelAuthenticationChall enge
• NSURLSessionAuthChallengeAcceptAuthenticationChall enge

Having to focus on a tiny spec in the middle of two otherwise identical names is just asking for trouble - no wonder so much security related code is buggy

[Pete Mack]

While I agree deeply nested code is bad, it's a long way from being the worst problem in angband. The worst problems are the ones that make modification a nightmare
1. Duplicated code.
2. Code related to a concept scattered across creation.
The latter lead to untold numbers of failed assertions in the known-objects change. I saw yet another one posted a couple days ago.

[calris]

If pushed a draft version of the coding guidelines into my repo here

Comments are appreciated

[takkaria]

If pushed a draft version of the coding guidelines into my repo here

Comments are appreciated

Thanks for these. It's nice to have a slightly longer document to wave at new coders.

I disagree with:

• Do not put multiple assignments on a single line.
• Only ever add one blank line - do not insert two blank lines between functions for example.
• Do not use braces for 'case' blocks
• Don't mix code and declarations

Given that these weren't discussed here I'd like to hear the rationale for them.

I'd also prefer sizeof to not use brackets (sizeof buf instead of sizeof(buf)) when it doesn't need to. I didn't realise the coding guidelines mandated the other way round until I re-read them.

[Nick]

Thanks for these. It's nice to have a slightly longer document to wave at new coders.

I disagree with:

• Do not put multiple assignments on a single line.
• Only ever add one blank line - do not insert two blank lines between functions for example.
• Do not use braces for 'case' blocks
• Don't mix code and declarations

Thanks indeed. I agree with takkaria on these, although I don't feel strongly about blank lines.

On the if .. else if .. else question, I am in favor of

	if (x == y) {
		..
	} else if (x > y) {
		...
	} else {
		....
	}

as I think it seems clearest - although it may just be habit.

I am also a bit less hardcore on not commenting inside a function body. When I first started doing stuff with it, the Angband code had nearly every line commented - I think that is excessive, but it certainly helped me as a n00b. I think descriptive variable names and code which reads like what it's supposed to do can often make comments unnecessary, but on the other hand a story sometimes needs narration as well as action and dialogue.

[calris]

I must have missed the submit button when I replied..

Thanks for these. It's nice to have a slightly longer document to wave at new coders.

I disagree with:

• Do not put multiple assignments on a single line.

A couple of reasons...

Firstly, it discourages the bad practice of assigning completely unrelated variables that just _happen_ to need the same initial value (often something like '1').
Secondly, when you refactor code, you really want patches to be as clean as possible - Ugly patches hide bugs. Having to pull out an unrelated variable means you are patching a line that does not need patching (i.e. a not very clean patch)

• Only ever add one blank line - do not insert two blank lines between functions for example.

I'm using checkpatch.pl to check for style - it has some flexibility, and I've made a change for non-Linux style already (always use braces after conditions). The single blank rule doesn't strike me as something that I needed to go out of my way to make an exception for - I see no compelling reason to ever have more than one blank line. The only time it is ever done is between functions, and we should have a comment block to provide a crystal clear delimitation anyway.

• Do not use braces for 'case' blocks

This saves one line of code per unique case statement. Also, when you look at:

    case 1:
    case 2:
    case 3: {
        do_something();
        break;
    }

It looks like the code block is tied to case 3 because that line looks different to the previous ones.

And then we get the argument of is the above preferred, or this:

    case 1:
    case 2:
    case 3: {
        do_something();
    } break;

• Don't mix code and declarations

Again, this is for refactoring and 'keeping patches clean' - If the variables are all defined together at the start of the function of code block, they are easy to find, easy to factor out, and the patch won't have some weird change in the middle of nowhere.

Given that these weren't discussed here I'd like to hear the rationale for them.

And to be honest, I hadn't thought of them until I wrote up the document

I'd also prefer sizeof to not use brackets (sizeof buf instead of sizeof(buf)) when it doesn't need to. I didn't realise the coding guidelines mandated the other way round until I re-read them.

sizeof (and typeof) are, again, enforced by checkpatch. I don't want it to look like I think checkpatch is the 'golden rule', but it's a really good tool that makes writing consistent code much easier. I'm happy to tweak it, but I would rather not (perl is not my strong suit)

Besides checkpatch, I honestly feel the brackets give real clarity about what you are actually getting the size of.

I will put something out there (and this really does relate to the case braces argument) - Some people like a coding style that their favourite IDE plays nicely with (code folding, jumping from the start and end of code blocks, etc.). Tools shouldn't dictate standards - yes, yes, I know I have defined a couple based on checkpatch but:

a) checkpatch was written to check against the Linux coding standard - the tool was written for a standard, not the standard for the tool originally, and;
b) I am happy to tweak checkpatch if there is a compelling reason to

I've seen a lot of code in my time - I've seen Visual C and C# code which is practically illegible outside the Microsoft IDE. If you need to use a particular IDE to work on a given chunk of code, there is a bigger problem that a coding style supporting the IDE will ever fix.

[calris]

Thanks indeed. I agree with takkaria on these, although I don't feel strongly about blank lines.

As maintainer, you have the final say

I am also a bit less hardcore on not commenting inside a function body. When I first started doing stuff with it, the Angband code had nearly every line commented - I think that is excessive, but it certainly helped me as a n00b. I think descriptive variable names and code which reads like what it's supposed to do can often make comments unnecessary, but on the other hand a story sometimes needs narration as well as action and dialogue.

Totally agree - comments in the code can be very helpful - particularly when you write something you feel might need some TLC later, or there is a gotcha. But always remember:

"If the comment and code don't match - they are probably both wrong"

[AnonymousHero]

when you look at:

    case 1:
    case 2:
    case 3: {
        // <--------- HERE
        do_something();
        break;
    }

It looks like the code block is tied to case 3 because that line looks different to the previous ones.

... but if you omit the braces then you get the problem that if you want to introduce scope-local declarations at the point I've marked HERE, you'll have to add braces... so you still have to decide (style-guide-wise) where to put them anyway.

Mandating braces would also seem to encourage using scope-local declarations when possible. (Generally considered good practice, by *most*, I believe.)

[the Invisible Stalker]

Here's a simple example of all the variants for the sake of comparisons...

Personally I would quite happily write

    if(test_1) i++; else if(test_2) j++; else k++;

but I'm aware that would probably result in a mob outside my door with pitchforks and torches.

While I'm saying things most people will disagree with, I'd prefer far fewer comments in the code. Angband code, like all code that has been around for ages, is full of examples where the comments no long correspond to the code. So I need to read the code anyway to determine what's really happening. But in that case what are the comments meant to be doing for me? Letting me know what some former maintainer intended the code, as it was then, to do? I honestly can't remember a case where I found that helpful.

[Derakon]

While I'm saying things most people will disagree with, I'd prefer far fewer comments in the code. Angband code, like all code that has been around for ages, is full of examples where the comments no long correspond to the code. So I need to read the code anyway to determine what's really happening. But in that case what are the comments meant to be doing for me? Letting me know what some former maintainer intended the code, as it was then, to do? I honestly can't remember a case where I found that helpful.

Of course the correct solution here is for comments to be updated when the code they refer to is updated and makes the comments inaccurate. This requires maintainers to have the discipline to a) do that, and b) reject pull requests that don't do that.

The old comment-for-every-line approach was probably overkill, but it's entirely too easy for there to be too few comments. Comments should usually concern themselves with high-level details, e.g. summarizing what a block or function does in an easy-to-digest fashion. For example, "This function finds the nearest valid target for a spell" is a good comment that will save people a ton of time when they're trying to figure out what bit of code they're looking for.

Lower-level, implementation-specific comments are only really needed when the implementation is doing something unusual or hard to understand. "This function generates a series of points using a Bresenham line-drawing algorithm" is a useful implementation-specific comment: it tells the user what background knowledge they need to have to understand the math in the function (which would otherwise be nearly totally opaque).

[the Invisible Stalker]

Of course the correct solution here is for comments to be updated when the code they refer to is updated and makes the comments inaccurate. This requires maintainers to have the discipline to a) do that, and b) reject pull requests that don't do that.

The old comment-for-every-line approach was probably overkill, but it's entirely too easy for there to be too few comments. Comments should usually concern themselves with high-level details, e.g. summarizing what a block or function does in an easy-to-digest fashion. For example, "This function finds the nearest valid target for a spell" is a good comment that will save people a ton of time when they're trying to figure out what bit of code they're looking for.

Lower-level, implementation-specific comments are only really needed when the implementation is doing something unusual or hard to understand. "This function generates a series of points using a Bresenham line-drawing algorithm" is a useful implementation-specific comment: it tells the user what background knowledge they need to have to understand the math in the function (which would otherwise be nearly totally opaque).

Those are both reasonable uses of comments. But most of the time it's more like

/*
 * Set the stack size (for the Amiga)
 */
#ifdef AMIGA
# include <dos.h>
__near long __stack = 32768L;
#endif


/*
 * SDL needs a look-in
 */
#ifdef USE_SDL
# include "SDL.h"
#endif

[takkaria]

Re braces after case & mixed declarations and code, the reason I disagreed was because (and this seems uncontroversial) it's a principle that variable declarations should be as close to the code that uses them as possible. I guess we're disagreeing on the extent to which that takes precedence over other principles.

For case statements I don't see a problem with them sometimes having braces and sometimes not, depending on whether variables are declared there, and if there are variables, it's easier to refactor later (e.g. by splitting the contents of the case statement to another function).

For mixed declarations and code, I guess I've never had any problem finding variables in a function, but I've repeatedly had situations where I've done something like:

int x;
assert(y != NULL);
x = y->property;

when it would have been much neater to start the function with asserts and then make the declaration of x and its value assignment on the same line. And If anything, I think putting variable declarations at the latest possible point for them makes the code less error-prone and easier to refactor, since the stuff you want to refactor is all grouped together.

The rest, I'm easy about, and I can see your points.

[calris]

... but if you omit the braces then you get the problem that if you want to introduce scope-local declarations at the point I've marked HERE, you'll have to add braces... so you still have to decide (style-guide-wise) where to put them anyway.

Mandating braces would also seem to encourage using scope-local declarations when possible. (Generally considered good practice, by *most*, I believe.)

I totally forgot that, although C now allows mixing declarations and code, you cannot have a declaration immediately after a case statement. So, in keeping with the ideal of declaring variables as locally as possible, case statements should, indeed, have braces