Coding Style Standards

[kmdurand]

It seems to me that establishing a set of style standards for the CalChart code will help make it more readable. This issue is intended as a forum for making style-related decisions.

@rmpowell77 PEP 8 serves as pretty popular set of style guidelines for Python; is there any popular set of C++ style guidelines online?

[noahsark769]

Maybe https://google-styleguide.googlecode.com/svn/trunk/cppguide.html?

[gmeeker]

I've been thinking about this for the past few years (not in CalChart but for work). That Google style guide is a lot to digest at once, but the biggest issue I think is indentation and whitespace.

I wrote CalChart using Emacs and it defaults to the GNU style, tab width 8, and indentation of 2. Here are some links about different indentation styles:

https://en.wikipedia.org/wiki/Indent_style https://davidha.wordpress.com/2009/05/15/emacs-cc-modes-built-in-styles-gallery/ http://www.gnu.org/software/emacs/manual/html_node/ccmode/Built_002din-Styles.html

GNU is probably not what most people expect, indenting after 'if' or 'for':

if (...) { code; }

so I tended to follow something more like Stroustrup (but using '} else {' to save space) and that's easier to edit with the default settings in Emacs.

A few years ago we hired someone and I reformatted a bunch of code to improve editing with the default setting in Visual Studio and Xcode, mainly using tab width 4 and indentation 4. I believe these are the only customizations available, nothing affecting auto-indentation. (A few years ago, Xcode defaulted to using spaces only, not tabs, but that can be changed.)

In my opinion that made a lot of code harder to read, mainly because 'if (' is 4 characters, so:

if (something || test2) { code; }

becomes:

if (something || test2) { code; }

So I think the editor you use really affects your choice. It's common for open source projects to require spaces only but Visual Studio might not be the best editor in that case. Personally, I started putting Emacs comments in the code like:

/* -- mode: C++; c-basic-offset: 2; indent-tabs-mode: nil -- */

As well as creating a custom Emacs style (e.g. to avoid indenting a namespace block). I can send that if anyone here uses Emacs.

On Mon, Sep 28, 2015 at 5:24 PM, Noah Gilmore notifications@github.com wrote:

Maybe https://google-styleguide.googlecode.com/svn/trunk/cppguide.html?

— Reply to this email directly or view it on GitHub https://github.com/calband/calchart/issues/163#issuecomment-143909675.

[noahsark769]

I have some opinions that I can contribute!

1. Whitespace: I think it's pretty clear that spaces are the way to go: in my experience (which admittedly is not a lot with C++), tabs never make things easier to read because they show up with different widths depending on your editor configuration (e.g. github renders tabs as 8 spaces in pull requests, which I think is very hard to read). It seems in general that spaces are better for editor compatibility also.
2. How many spaces? My opinion is 4, pretty much always. Having worked with xcode more lately, I can see why 2 spaces is preferred by some, but I think 4 is very easily readable (I can recognize indents more clearly with 4 spaces than with 2), while 2 is sometimes confusing and 8 is almost too spread out. Doesn't really matter that much though, as long as it's consistent.

3. If/while/braces/etc: I think that it's most clear/clean to have the braces on the same line, e.g.

   if (something) {
      // something
   } else {
      // something
   }

   The problem @gmeeker mentioned can be remedied by indenting multi-line if conditions are too long, e.g.

   if (something &&
          somethingElse && somethingOther) {
      // something
   }

   Another option that I like, though is implemented in pretty much no standard anywhere, is moving the brace to a new line if there is a huge condition, e.g.

   if (
      something &&
      somethingElse &&
      somethingOther
   ) {
      doSomething();
   }

   Of course, probably a better option is just to combine the huge condition into a boolean variable instead, and put that in the if's parentheses.

4. Naming conventions: I think these are actually very important. As I recall, there are a lot of places in calchart where things are distributed between CapitalCamelCase, camelCase, snake_case, etc. I think it's important to always have class names as CapitalCamelCase, and everything else as camelCase (macros, possibly, could be UPPER_SNAKE_CASE).
5. Member variables - what do you guys think about using this->variable vs. mVariable? I think that regardless of the use of this, m_Variable or m_variable is a little bit much, and should probably be stayed away from.

Anyway, I think code style is a really good investment! There is a lot of tooling we could put it to make sure that code conforms to style before being pushed.

[rmpowell77]

We shouldn’t really worry about the style, but be consistent. The best way to enforce it is with clang-format, a tool that will apply consistent style to all of the code. See: http://clang.llvm.org/docs/ClangFormat.html

I recommend we go with WebKit.

I attended a talk by some Google engineers who basically came to the conclusion that humans can’t be trusted to format code. Let a tool do it.

When I get a chance, I'll make a script that just goes through the code and format everything as webkit.. Then, on every commit, you’ll just need to run that script and everything will be formatted the right way.

Richard

On Sep 29, 2015, at 10:07 AM, Noah Gilmore notifications@github.com wrote:

I have some opinions that I can contribute!

Whitespace: I think it's pretty clear that spaces are the way to go: in my experience (which admittedly is not a lot with C++), tabs never make things easier to read because they show up with different widths depending on your editor configuration (e.g. github renders tabs as 8 spaces in pull requests, which I think is very hard to read). It seems in general that spaces are better for editor compatibility also. How many spaces? My opinion is 4, pretty much always. Having worked with xcode more lately, I can see why 2 spaces is preferred by some, but I think 4 is very easily readable (I can recognize indents more clearly with 4 spaces than with 2), while 2 is sometimes confusing and 8 is almost too spread out. Doesn't really matter that much though, as long as it's consistent. If/while/braces/etc: I think that it's most clear/clean to have the braces on the same line, e.g. if (something) { // something } else { // something } The problem @gmeeker https://github.com/gmeeker mentioned can be remedied by indenting multi-line if conditions are too long, e.g. if (something && somethingElse && somethingOther) { // something } — Reply to this email directly or view it on GitHub https://github.com/calband/calchart/issues/163#issuecomment-144123165.

[noahsark769]

+1 for clang-format: we use that at work and it seems to work very well. I think the naming conventions are still something we should think about though, since clang-format won't doc those (I don't think). On Tue, Sep 29, 2015 at 11:42 AM rmpowell77 notifications@github.com wrote:

We shouldn’t really worry about the style, but be consistent. The best way to enforce it is with clang-format, a tool that will apply consistent style to all of the code. See: http://clang.llvm.org/docs/ClangFormat.html

I recommend we go with WebKit.

I attended a talk by some Google engineers who basically came to the conclusion that humans can’t be trusted to format code. Let a tool do it.

When I get a chance, I'll make a script that just goes through the code and format everything as webkit.. Then, on every commit, you’ll just need to run that script and everything will be formatted the right way.

Richard

On Sep 29, 2015, at 10:07 AM, Noah Gilmore notifications@github.com wrote:

I have some opinions that I can contribute!

Whitespace: I think it's pretty clear that spaces are the way to go: in my experience (which admittedly is not a lot with C++), tabs never make things easier to read because they show up with different widths depending on your editor configuration (e.g. github renders tabs as 8 spaces in pull requests, which I think is very hard to read). It seems in general that spaces are better for editor compatibility also. How many spaces? My opinion is 4, pretty much always. Having worked with xcode more lately, I can see why 2 spaces is preferred by some, but I think 4 is very easily readable (I can recognize indents more clearly with 4 spaces than with 2), while 2 is sometimes confusing and 8 is almost too spread out. Doesn't really matter that much though, as long as it's consistent. If/while/braces/etc: I think that it's most clear/clean to have the braces on the same line, e.g. if (something) { // something } else { // something } The problem @gmeeker https://github.com/gmeeker mentioned can be remedied by indenting multi-line if conditions are too long, e.g. if (something && somethingElse && somethingOther) { // something } — Reply to this email directly or view it on GitHub < https://github.com/calband/calchart/issues/163#issuecomment-144123165>.

— Reply to this email directly or view it on GitHub https://github.com/calband/calchart/issues/163#issuecomment-144151777.

[rmpowell77]

Ok, I’m going to make this change shortly. I’m in favor of webkit style myself.

Be aware that this will have wide formatting change implications. If you have any outstanding changes that need to get into the build, it’s going to be difficult to do a merge after this change goes in. So let me know if you’ve got something outstanding.

Richard

On Sep 29, 2015, at 5:50 PM, Noah Gilmore notifications@github.com wrote:

+1 for clang-format: we use that at work and it seems to work very well. I think the naming conventions are still something we should think about though, since clang-format won't doc those (I don't think). On Tue, Sep 29, 2015 at 11:42 AM rmpowell77 notifications@github.com wrote:

We shouldn’t really worry about the style, but be consistent. The best way to enforce it is with clang-format, a tool that will apply consistent style to all of the code. See: http://clang.llvm.org/docs/ClangFormat.html

I recommend we go with WebKit.

I attended a talk by some Google engineers who basically came to the conclusion that humans can’t be trusted to format code. Let a tool do it.

When I get a chance, I'll make a script that just goes through the code and format everything as webkit.. Then, on every commit, you’ll just need to run that script and everything will be formatted the right way.

Richard

On Sep 29, 2015, at 10:07 AM, Noah Gilmore notifications@github.com wrote:

I have some opinions that I can contribute!

Whitespace: I think it's pretty clear that spaces are the way to go: in my experience (which admittedly is not a lot with C++), tabs never make things easier to read because they show up with different widths depending on your editor configuration (e.g. github renders tabs as 8 spaces in pull requests, which I think is very hard to read). It seems in general that spaces are better for editor compatibility also. How many spaces? My opinion is 4, pretty much always. Having worked with xcode more lately, I can see why 2 spaces is preferred by some, but I think 4 is very easily readable (I can recognize indents more clearly with 4 spaces than with 2), while 2 is sometimes confusing and 8 is almost too spread out. Doesn't really matter that much though, as long as it's consistent. If/while/braces/etc: I think that it's most clear/clean to have the braces on the same line, e.g. if (something) { // something } else { // something } The problem @gmeeker https://github.com/gmeeker mentioned can be remedied by indenting multi-line if conditions are too long, e.g. if (something && somethingElse && somethingOther) { // something } — Reply to this email directly or view it on GitHub < https://github.com/calband/calchart/issues/163#issuecomment-144123165>.

— Reply to this email directly or view it on GitHub https://github.com/calband/calchart/issues/163#issuecomment-144151777.

— Reply to this email directly or view it on GitHub https://github.com/calband/calchart/issues/163#issuecomment-144230495.

[kmdurand]

Thanks everyone for your input on this! After checking out the webkit style guidelines, I agree that it's a good direction to move. It corresponds really closely with the style patterns that we've already been more-or-less already using for our more recent CalChart commits. To highlight how WebKit deals with the style issues that have been mentioned in the comments above:

1. Indentation levels are marked by 4 spaces. Tabs are never used for formatting.
2. Multiline if conditionals should be clear, since they are either indented by an additional 4 spaces or headed by a boolean operator, like so:

if (var1 + var2 + var3 + var4
        var 5 + var 6 + var7
    < 9000) {
    // ...
}

1. Instance variables are given names like m_variable. I think this is a great convention: the underscore makes the instance variables stand out against local variables and arguments, and it's nice that the prefix doesn't have an effect on the casing for the rest of the variable name (as would be the case with mVariable). I admit that I like this->variableName better, but m_variable works for me just fine.
2. Names should be written in camelCase. Only the names of classes, structs, and namespaces should begin with a capital letter.

[kmdurand]

I will leave this open for a few more days just in case anyone has any last comments that they would like to bring up.

[rmpowell77]

As an aside, all the code is formatted with WebKit.

There’s a great book called “C++ Coding Standards” by Herb Sutter and Andrei Alexandrescu. It has 101 rules that are essential the definitive Coding standard. The first one is:

1. Don’t sweat the small stuff. (Or: Know what not to standardize.)

Some good quotes:

“Don’t over legislate naming, but do use a consistent naming conventions.”

So while I don’t think we should argue about what to use, we should probably use something consistently. I try to, but sometimes in the code you’ll see where I don’t… :)

On Nov 4, 2015, at 3:10 PM, kmdurand notifications@github.com wrote:

I will leave this open for a few more days just in case anyone has any last comments that they would like to bring up.

— Reply to this email directly or view it on GitHub https://github.com/calband/calchart/issues/163#issuecomment-153899090.