Angband Coding Style

[calris]

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.

The guidelines do state that variables should be declared as locally as possible, so variables declared inside for loops, do/while blocks, case blocks, etc. are encouraged.

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

The big flaw in not using braces has been pointed out, so I agree that case statements should have braces exactly for the purpose of declaring variables locally to assist in refactoring.

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.

Guidelines are, after all, guidelines... there comes a point where you need to ignore them so that you code is more readable (string constants going over 80 columns for example). I have no issue with parameter asserts being at the very start of the function, particularly when you want to do the this:

int my_function(struct object *obj)
{
    assert(obj);

    int x = obj->x;
    int y = obj->y;
}

which is cleaner and shorter than this:

int my_function(struct object *obj)
{
    int x, y;

    assert(obj);

    x = obj->x;
    y = obj->y;
}

I also don't mind them being immediately after variable declarations

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.

Typically we refactor blocks of code, like functionising the internals of a loop. The local variables will be declared at the start of that block of code and should come out nice and cleanly.

I, personally, find it very messy when there are variable declarations spread semi-randomly through the code. I find my train of though works better when I know upfront what types of data the following code will be dealing with - like

"OK, this block of code will be messing around with a foo - I haven't seen a foo for a while, I'll quickly check foo.h - yep, got it. Now let's look at the code"

Versus

"OK yep, yep, got it, um, bugger, what's a foo look like, better check foo.h. Right, got it, now, what was this code doing, bugger, better go back to the start"

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

That would be multi-assignments and double blanks

[calris]

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.

Why yes, I do believe it will

While I'm saying things most people will disagree with, I'd prefer far fewer comments in the code.

I think this is a far less contentious point. Good, clean, readable code doesn't need comment littered throughout it - it it actually decreases readability.

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.

I said it earlier - If the comments don't match the code, they are probably both wrong

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.

obj-desc.c, obj_desc_name_format() - there are couple of nice comments which help by pointing out what the next section of the function does (relating the complex nature of pluralisation in English) - I think this is a good example of commenting done right

[calris]

I've push the following changes to the coding guidlines.

• case statements now need braces
• assert() calls to check function parameters can be placed before variable declarations

[the Invisible Stalker]

I think we may be looking at this the wrong way. The Linux coding standards are simply a more detailed version of the statement "If Linus doesn't like it then it's wrong". I'd suggest replacing "Linus" with "Nick" in that sentence, since he's the current maintainer, and deducing all the details from that premise.

[calris]

I think we may be looking at this the wrong way. The Linux coding standards are simply a more detailed version of the statement "If Linus doesn't like it then it's wrong". I'd suggest replacing "Linus" with "Nick" in that sentence, since he's the current maintainer, and deducing all the details from that premise.

Nick has already distanced himself from the argument.

Nick is smart.

Be like Nick.

[the Invisible Stalker]

There are probably issues where Nick has no preference one way or the other, but needs things to be consistent. Indentation might well be an example of this. I don't know if Nick really cares whether we use four or eight spaces, but I doubt he we would want to read code with a mixture of the two. Most of the issues being discussed here don't seem to me to belong to that category. They mostly seem like issues where not caring one way or the other seems eminently sensible, and where I'd be quite happy to see that indifference reflected in the coding standards.

[calris]

Most of the guidelines don't really matter one way or the other, but the important thing is consistency. I've done a _little_ bit of Linux coding and fair amount of U-Boot coding (which follows the Linux coding standards) and coding for those two code bases is vastly superior to Angband - The code you start with is consistent, the code you end with is consistent, and the patches that lead from A to B are clean.

Some of the arguments I'm putting forward apply as much (if not more) to the patches as to the actual code. Once you get the code to a state of near-perfect consistency, you start to look at patches and think 'no, that's not right - there's a bug right there' without even applying the patch to the code.

[calris]

Does anyone have any more to say on the guidelines? I would really like to start getting stuck into cleaning up the code, but there's no real point in starting until we agree with the guidelines and Nick pulls them into mainline