Angband.oook.cz
Angband.oook.cz
AboutVariantsLadderForumCompetitionComicScreenshotsFunniesLinks

Go Back   Angband Forums > Angband > Development

Reply
 
Thread Tools Display Modes
Old April 25, 2016, 11:13   #21
takkaria
Veteran
 
takkaria's Avatar
 
Join Date: Apr 2007
Posts: 1,935
Donated: $40
takkaria is on a distinguished road
Quote:
Originally Posted by calris View Post
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.
__________________
takkaria whispers something about options. -more-
takkaria is offline   Reply With Quote
Old April 25, 2016, 14:37   #22
Nick
Vanilla maintainer
 
Nick's Avatar
 
Join Date: Apr 2007
Location: Canberra, Australia
Age: 54
Posts: 7,832
Donated: $60
Nick is on a distinguished road
Quote:
Originally Posted by takkaria View Post
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
Code:
	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.
__________________
One for the Dark Lord on his dark throne
In the Land of Mordor where the Shadows lie.
Nick is offline   Reply With Quote
Old April 25, 2016, 14:54   #23
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
I must have missed the submit button when I replied..

Quote:
Originally Posted by takkaria View Post
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)
Quote:
  • 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.
Quote:
  • Do not use braces for 'case' blocks
This saves one line of code per unique case statement. Also, when you look at:
Code:
    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:
Code:
    case 1:
    case 2:
    case 3: {
        do_something();
    } break;
Quote:
  • 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.

Quote:
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
Quote:
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 is offline   Reply With Quote
Old April 25, 2016, 14:58   #24
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Quote:
Originally Posted by Nick View Post
Thanks indeed. I agree with takkaria on these, although I don't feel strongly about blank lines.
As maintainer, you have the final say

Quote:
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"
calris is offline   Reply With Quote
Old April 25, 2016, 16:49   #25
AnonymousHero
Veteran
 
AnonymousHero's Avatar
 
Join Date: Jun 2007
Posts: 1,367
AnonymousHero is on a distinguished road
Quote:
Originally Posted by calris View Post
when you look at:
Code:
    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.)
AnonymousHero is offline   Reply With Quote
Old April 25, 2016, 17:18   #26
the Invisible Stalker
Adept
 
Join Date: Jul 2009
Posts: 164
the Invisible Stalker is on a distinguished road
Quote:
Originally Posted by calris View Post
Here's a simple example of all the variants for the sake of comparisons...
Personally I would quite happily write
Code:
    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.
the Invisible Stalker is offline   Reply With Quote
Old April 25, 2016, 17:41   #27
Derakon
Prophet
 
Derakon's Avatar
 
Join Date: Dec 2009
Posts: 8,916
Derakon is on a distinguished road
Quote:
Originally Posted by the Invisible Stalker View Post
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).
Derakon is offline   Reply With Quote
Old April 25, 2016, 18:55   #28
the Invisible Stalker
Adept
 
Join Date: Jul 2009
Posts: 164
the Invisible Stalker is on a distinguished road
Quote:
Originally Posted by Derakon View Post
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
Code:
/*
 * 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
the Invisible Stalker is offline   Reply With Quote
Old April 26, 2016, 00:07   #29
takkaria
Veteran
 
takkaria's Avatar
 
Join Date: Apr 2007
Posts: 1,935
Donated: $40
takkaria is on a distinguished road
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:

Code:
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.
__________________
takkaria whispers something about options. -more-
takkaria is offline   Reply With Quote
Old April 26, 2016, 02:43   #30
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Quote:
Originally Posted by AnonymousHero View Post
... 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
calris is offline   Reply With Quote
Reply


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump

Similar Threads
Thread Thread Starter Forum Replies Last Post
Sil: 'forge' key in Angband-style keyset? Therem Harth Sil 3 October 24, 2012 18:29
Skyrim, angband style BlackKite Idle chatter 8 June 27, 2012 03:28
some more questions about coding Malak Darkhunter Vanilla 16 January 3, 2012 05:52
more variant coding woes will_asher Variants 1 February 10, 2011 06:18
Angband coding question will_asher Variants 10 September 3, 2008 23:03


All times are GMT +1. The time now is 07:09.


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2019, vBulletin Solutions Inc.