[phobos] Initial Phobos style guide proposal

[Jonathan M Davis]

On 2011-03-31 08:21, Jacob Carlborg wrote:
> > It's become clear that we need at least a basic style guide for Phobos.
> > While some of our coding conventions are clear and consistent, others
> > vary depending on who's writing the code, and more importantly, new
> > folks writing code for Phobos (be they new Phobos devs or simply writing
> > code to be reviewed for inclusion in Phobos) need to be aware of the
> > coding conventions that we follow. So, I've put one together based on
> > what has previously been discussed, what we generally do in code, and
> > what the online style guide says (though all I did with that for the
> > most part was take some of its nicer points that we pretty much follow
> > anyway).
> > 
> > This is obviously not set in stone. Rather it's the starting point for a
> > discussion. In the first two sections (naming conventions and formatting
> > conventions), _most_ of it has been agreed upon by the Phobos devs in
> > general, as I understand it (though there are some areas - such as line
> > length - which have _not_ been agreed upon). The last section, general
> > coding guidelines, is mixture of what we already do and what Andrei has
> > said that he wants to be the case (though I did tweak some of what he
> > said - e.g. one of his posts implied that there shouldn't be _any_ empty
> > lines in a function which leads to highly unreadable functions IMHO), so
> > that is _definitely_ an area which is up for discussion. It might also
> > be a bit long with items that are obvious enough that we can remove
> > them, though the idea is to make what we expect in Phobos code clear.
> > 
> > These are intended to be general guidelines which are followed most of
> > the time but can be broken within reason (though hopefully that's
> > relatively rare).
> > 
> > ============
> > Naming conventions
> > ------------------
> > - Type names are camelcased and begin with an uppercase letter.
> > 
> > - Function and variable names and named values (including enum values)
> > are
> > 
> >  camelcased and begin with a lowercase letter.
> > 
> > - Module names are all lowercase without camelcasing.
> > 
> > - Private member variables begin with an underscore.
> 
> I don't see any reason what so ever to put an underscore in front of member
> variable. The only reason I would see this as useful is when having
> getter/setter method with the same name, ie:
> 
> class Foo
> {
>     int _foo;
> 
>     int foo () { return _foo; }
> }
> 
> But in that case I would still prefer to put the underscore after the name.

The current style used in Phobos is an underscore before the name. It's also 
likely to be frequent that a member variable has a property which goes with 
it, meaning that it _has_ to be named slightly differently. How much the exact 
naming scheme matters though depends on how much consistency we want within 
Phobos. It's _not_ part of the public API, so it's purely a matter of 
consistency in coding style. If we don't care all that much in keeping all of 
Phobos' internal stuff consistent in style, then this sort of rule isn't 
necessary. If we want obe consistent though, a choice needs to be made, and 
the underscore before is currently what's being used (it's also what I, 
personally, prefer - as does Andrei, I believ). But really, the main question 
is whether we _want_ to be this exacting about private names.

> > - With templates, if they're for a type or result in a type, use the
> > naming
> > 
> >  conventions for type names. If they generate a value, then use the
> >  naming conventions for variables. For template mixins, use the naming
> >  conventions for type names.
> > 
> > - Try to make names clear and descriptive, but avoid overly long names.
> > 
> >  Shorter names which are still appropriately descriptive are preferred.
> > 
> > Formatting Conventions
> > ----------------------
> > - Don't use tabs. Use spaces.
> > 
> > - Indenting is 4 spaces.
> > 
> > - Braces go on their own line and line up.
> > 
> > - Commit code with unix line endings (though what you use in your editor
> > 
> >  is irrelevant).
> > 
> > - Try to make lines not exceed 80 characters, but it's not a hard limit.
> > 
> >  If it harms code readability to restrict it to 80 characters, then
> >  exceed 80 characters, but if you go much beyond 80 characters, you
> >  really should break the line up.
> 
> I would increase this to something like 100 or 120.

I would love that and think that it would be a _big_ improvement, but there 
are some folks (Andrei in particular) who seem to really not like code going 
beyond 80 characters. I do know that there are other Phobos devs who _do_ want 
longer line lengths though, so maybe we can change this. Where this really 
gets to be a problem is with lines with several levels of indentation, and 
trying to restrict the line length (particularly to something short like 80 
characters) tends to encourage overly short and non-descriptive variable and 
function names, which is _not_ good.

> > General Coding Guidelines
> > -------------------------
> > - Don't put multiple statements on the same line.
> > 
> > - Restrict the scope of variables as much as reasonably possible and
> > declare
> > 
> >  them as late as reasonably possible.
> > 
> > - Use enums for manifest constants, not const or immutable.
> > 
> > - Prefer anonymous temporaries to named values within reason.
> > 
> > - Prefer ? : to if/else within reason.
> > 
> > - Prefer compact, effective code to verbose code within reason. Make
> > every
> > 
> >  line count.
> 
> I completely disagree with this. I don't see why it's so important to save
> vertical space. I would say prefer readable code to compact code. Don't be
> afraid of having long descriptive names of variables and having vertical
> space in your code. I don't mean you should put three newlines between two
> function declarations but I usually put a newline before and after
> statements:
> 
> int a;
> int b;
> 
> foo();
> bar();
> 
> if (a == b)
>    foo();
> 
> else
>    bar();
> 
> int c = 3;

I do think that we don't want tons of extra space in our code, but I 
definitely favor having some blank lines in code - though I'd never put a 
blank line before an else like that. I think that trying to be particularly 
restrictive about vertical space leads to code which is harder to read (and 
being restrictive about both line length and vertical space at the same time 
is definitely a bad combination for readability). I do think that trying to 
keep functions to a page of code or so (however long that's supposed to be) is 
generally a good idea, but I confess that I do not really understand Andrei's 
preference for incredibly compact code. I find it harder to read and maintain.

I would think that ideally we'd want code that is both short and clear, but 
those are often contradictory requirements, and personally, I tend to prefer 
clear to short. Andrei seems to prefer short, or at least finds more compact 
code to be clearer and easier to read than some of the rest of us do. So, I'm 
not quite sure what we want to do about this. It seems like some sort of 
compromise is required.

> > - Avoid having 2+ empty lines in a row and reasonably minimize how many
> > empty
> > 
> >  lines are within a function.
> > 
> > - Try to fit functions loosely on one editor page.
> > 
> > - Comments should be high level (describing 3-10 lines) instead of
> > low-level
> > 
> >  (describing the mechanics of the next line, which are already obvious in
> >  code).
> > 
> > - Avoid meaningless aliases. Use aliases when reasonable, but we don't
> > want
> > 
> >  to pollute the namespace with unnecessary aliases.
> > 
> > - Prefer to follow the convention that [] and * go with the type name
> > rather
> > 
> >  than the variable name (e.g. int* a; instead of int *a;).
> > 
> > - Do not use Hungarian notation.
> > 
> > - All public declarations should have proper ddoc documentation.
> 
> And protected methods as well, they're a part of the API just as much as
> the public methods.

Good point.

> > - If you need to use a version block for documentation, use
> > version(StdDoc),
> > 
> >  not version(D_Ddoc).
> > 
> > - DDoc documentation should be generatable for all OSes, so if you have
> > 
> >  multiple versions of a function for differing OSes or if a function
> >  doesn't exist on all OSes, then either put the version blocks within
> >  the function or use a version(StdDoc) with a declaration of the
> >  function (without a body) and the documentation.
> > 
> > - Unit test as much as is practical.
> > 
> > - Generally avoid using else with versions (as in else by itself, not
> > 
> >  else version(x)) with version blocks unless you use static assert(0) in
> >  the else block. We want to avoid cases where a new OS is used with
> >  Phobos and it uses the version block for another OS without a
> >  programmer properly looking at it and verifying that it's valid for the
> >  new OS.
> > 
> > - Make functions pure, nothrow, and const (if it's a member function) as
> > much
> > 
> >  as reasonably possible, so that they work with pure, nothrow, and const
> >  code.
> > 
> > - Make as many function parameters const (or scope) as reasonably
> > possible so
> > 
> >  that you can pass const and immutable values to them.
> > 
> > * Note: The rules in this style guide are guidelines which we want to be
> > 
> >        generally followed so that we have consistent code in Phobos, but
> >        they are not generally hard-and-fast rules which can never be
> >        broken. They are guidelines that we wish to follow. So, you can
> >        break them _within reason_ but should generally follow them.
> > 
> > ============
> > 
> > Personally, I'd prefer a line's character limit to be more like 100 (if
> > not more). I also like putting two empty lines between functions (as the
> > old, online style guide says to do), so the restriction eliminating two
> > empty lines in a row doesn't appeal to me. I also am not fond of the
> > tendency of some (such as Andrei) to eliminate all extra vertical space
> > within a function (though I do understand not wanting to have tons of
> > empty lines in functions), and that combined with restrictions on line
> > length is a nasty combination for code readability. _Most_ of the rest,
> > I agree with. However, there are obviously going to be compromises made
> > by pretty much everyone involved. What we need is a general consensus
> > that we're generally willing to code to and which is clear.
> > 
> > So, that's my initial draft. After we've discussed it a bit and are more
> > firm on what we want to do, I can create a version using DDoc which is
> > nicer looking and can be put on the website if we want to.
> > 
> > - Jonathan M Davis
> > _______________________________________________
> > phobos mailing list
> > phobos at puremagic.com
> > http://lists.puremagic.com/mailman/listinfo/phobos

Overall, I get the impression that most of the style stuff that Phobos devs 
have expressed interest in having rules on are in the public API whereas what 
Andrei seems most interested in is the internal coding style. So, I don't know 
where we want to go with this. Personally, I think that consistency the public 
API is by far the most important thing, but we also don't want a complete 
hodge-podge in the code itself. I'm generally inclined not to get all that 
picky about style rules such as the number of characters per line or the exact 
naming of private member variables.

However, we need to actually get a decent discussion and agreement from the 
Phobos devs as a whole, otherwise it's just a few of us determining it for the 
group as a whole, and we could end up with something that only a few of us 
like (or are willing to stick to). Once we've agreed and have a final 
document, hopefully we can pretty much drop the issue and just point people to 
the document in the future.

- Jonathan M Davis