Apply a consistent code style

[J5lx]

Quick Links

• Summary of current status
• Spreadsheet of current status

Something that really stands out to me when working on Pencil’s code is that there is no universal code style. While there are certain conventions that are more prevalent than others, it seems that it is never universally consistent. Since I think that everyone will agree that any style is better than no style at all, I compiled a list of the various conventions in use so we can have a discussion as to what to use (I’m aware I might be opening Pandora’s box here). Once there is consent on the style to use, we can unify the style of the existing code and add an editorconfig to help new contributors stay consistent with it (Qt Creator plugin available at editorconfig/editorconfig-qtcreator). I’d happily volunteer to take care of the unification and the editorconfig myself.

Ordering is arbitrary, numbers are only there to make it easier to refer to a particular item if necessary.

1. Indentation
   1. four spaces per indent (currently prevalent by far)
   2. one tab per indent
2. End of file
   1. No newline at end of file
   2. One newline at end of file (currently prevalent by far)
   3. Two newlines at end of file (why would anyone want that?)
3. Whitespace in parentheses
   (sometimes also in other kinds of brackets)
   1. One space after each opening parenthesis and before each closing parenthesis: ( example ) (currently prevalent)
   2. No spaces after opening parenthesis or before closing parenthesis: (example)
4. Whitespace before parentheses
   (sometimes also before other kinds of brackets)
   1. No whitespace before the opening parenthesis of function calls, one space before the opening parenthesis of control structures (if, while etc.) (currently prevalent by far)
   2. No whitespace before any opening parenthesis
5. Braces
   1. Always one newline before the opening brace, never omit optional braces (variation of Allman) (currently prevalent by far)
   2. One newline before mandatory opening braces (e.g. function declarations), one space before optional opening braces (e.g. conditionals), never omit optional braces (OTBS)
   3. On rare occasions variations of the above with no whitespace before opening braces or omitted optional braces
6. Trailing whitespace
   1. No trailing whitespace anywhere (currently prevalent by far)
   2. Have indentation even on otherwise empty lines, otherwise no trailing whitespace
   3. Trailing whitespace? Who cares!
7. Aligning consecutive assignments
   1. Don’t align consecutive assignments (currently prevalent by far)
   2. Align consecutive assignments
8. Multi-line function calls / declarations
   1. First argument on initial line, each remaining argument on a line of its own and aligned with the first argument, closing parenthesis on same line as last argument (currently prevalent by far)
   2. First argument on initial line, each remaining argument on a line of its own and indented by one indent, closing parenthesis on same line as last argument
   3. First argument on initial line, each remaining argument as well as closing parenthesis on a line of its own and indented by one indent
   4. Every single argument on its own line and indented by one indent, closing parenthesis on its own line with same indentation as first line
   5. Arbitrary
9. Line length limit
   (For obvious reasons, I have no idea if there ever was a line length limit in Pencil’s code so I’m only including the “none” option here which is easy to spot)
   1. Who cares about line length! 428 characters on one line are nothing!
10. File encoding
    1. ASCII / UTF-8 w/o BOM (currently prevalent by far)
    2. UTF-8 w/ BOM (1 file)
11. Whitespace around operators
    1. One space before and after each operator (currently prevalent by far)
    2. No whitespace before or after each operator
    3. Arbitrary whitespace before and after each operator
12. Enum case
    1. SCREAMING_SNAKE_CASE
    2. PascalCase (currently prevalent)
13. Enum member case
    1. SCREAMING_SNAKE_CASE (currently prevalent by far)
    2. PascalCase
14. Member var names
    1. camelCase (currently prevalent for structs)
    2. m_camelCase (quite rare)
    3. mPascalCase (currently prevalent for classes)
15. Pass-by-reference in parameter lists
    (apparently no prevalent style here)
    1. type& param
    2. type &param
    3. type & param (quite rare, almost didn’t notice it because it looks so much like bitwise and)
16. Pointers
    (apparently no prevalent style here)
    1. type *identifier, type* when no identifier is given
    2. type* identifier
    3. type * identifier (quite rare)
17. Pointer pass-by-reference
    (this is currently not in master but I need it for my lav exporter so I’m including the choice that I made for discussion)
    1. type *&identifier
18. Aligning consecutive declarations
    1. Don’t align consecutive declarations (currently prevalent)
    2. Align consecutive declarations
19. Naming of non-boolean accessor functions
    1. get prefix (e.g. getColor(), currently prevalent)
    2. No prefix (e.g. color())
20. Naming of boolean accessor functions
    1. get prefix (e.g. getPlaying())
    2. No prefix (e.g. playing())
    3. is prefix (e.g. isPlaying(), currently prevalent)
    4. getIs prefix (e.g. getIsPlaying(), currently unused)
21. Naming of boolean mutator functions
    1. set prefix (e.g. setPlaying()`, currently prevalent)
    2. setIs prefix (e.g. setIsPlaying(), currently unused)
22. Naming of slots that are used exclusively for catching one specific signal
    1. on<SignalName> (e.g. onDurationChanged())
    2. No prefix (e.g. durationChanged()). Obviously this will have to be different from the signal name, or in a different class from the signal.
23. Documentation comment style
    1. Javadoc style

       /**
       ...
       **/

    2. Qt style

       /*!
       ...
       */

    3. Three slashes

       ///
       ///

    4. Two slashes and one exclamation mark

       //!
       //!

    5. Asterisk spam

       /********************************************//**
       ...
        ***********************************************/

    6. Slash spam

       /////////////////////////////////////////////////
       /// ...
       /////////////////////////////////////////////////

24. Intermediate asterisks
    Only effective if i, ii, or v is chosen in previous item (23).
    1. Include intermediate asterisks

       /**
        * ...
        */

    2. Skip intermediate asterisks

       /**
       ...
       */

25. Brief description format
    1. Always use \brief
    2. Autobrief, ends the brief description at the first dot (“.”) encountered. Use \brief if you want more than one sentence.
    3. A /// or //! comment before the main comment block
26. Member variable documentation comment style
    1. Regular style (22), before variable

       /** Description of var */
       int var;

    2. Regular style (22) except with <, after comment:

       int var; /**< Description of var */

27. Special command format
    1. @ prefix (Javadoc style, e.g. @param)
    2. \ prefix (e.g. \param)
28. Parameter descriptions
    1. @param command

       /**
        * @param[in] var String to print
        */
       void print(string var);

    2. Inline comment

       void print(string var /**< [in] String to print */);

For reference, there are a few things that do seem to be consistent throughout. For instance, line endings are always LF rather than CRLF, class case is always PascalCase, method case is always camelCase. I also didn’t include a few things that are almost universally consistent and should be a given, such as no whitespace before semicolons vs one space before semicolons.

If you think I made a mistake or forgot about / missed something that should be included, let me know. (Edit 2017-07-29: Added items suggested by @scribblemaniac)

Of course the prevalent styles might give a good direction, but I suggest we don’t take them as ultimate authority if we think there is a good reason to use another style, since someone (presumably me) is going to go over all the code to apply the style anyway, so we should take that opportunity.

Now, let the clash of beliefs begin. (But please do try to be flexible and keep it civil 🙂)

[J5lx]

As for my own opinion, I can live with every single convention I marked as prevalent. However there are some cases where I’d still prefer a different convention:

• For whitespace in parentheses (3) I’d prefer the variant without spaces; firstly because it is less verbose and secondly because I have yet to discover a single editor that can insert those spaces automatically
• For braces (5) I’d prefer OTBS because it offers a nice compromise between visual clarity for “top-level” structures like functions and reduced verbosity for the more common control structures. This is especially noticeable for stuff like else.
  Allman:

  }
  else
  {
  

  OTBS:

  } else {
  

  Much less verbose!
• For enums (12+13) I’d prefer EnumName::memberName for consistency with ClassName::memberName and StructName.memberName

But as I said before, these are just really-nice-to-have to me and I can live without them if it is decided that way. As for the topics where I couldn’t determine a prevalent style:

• Line length limit (9): I’d suggest the traditional limit of 80 characters per line as an advisory limit and 1.5 * 80 = 120 as a soft limit. Similar conventions are common in other style guides (PEP 8 and PSR-2/PSR-12 come to mind from the top of my head)
• Pass-by-reference (15), pointers (16), pointer pass-by-reference (17): I’d suggest always binding &/*/*& to the identifier. This is incredibly common for pointers in C (where they are omnipresent) and it helps with declaration lists where type *var1, *var2 yields to pointers as expected while type* var1, var2 confusingly results in one pointer and one non-pointer

In this case the second one is somewhat important to me, but I can live with other length limits.

[MrStevns]

Thank you for doing this! @J5lx

I've thought about it too but never got around it.
So far i've just tried to stay consistent with whatever style each individual class has had, but having a defined coding style would make everything much easier.

As for my preferences:

1. ii, I personally think tabs are easier and faster than having multiple spaces. That said, the editors I use can automatically convert from either way so whatever we choose here is fine with me.
2. i, although it has never bothered me.
3. prevalent, while ' i ' can be somewhat easier to read, it can be such a pain to keep track of.
4. ii
5. ii
6. prevalent
7. prevalent
8. prevalent, although I lead toward iv (arbitrary) as it mostly comes down to line length for me. if there is space enough for two arguments on first line, then I would probably put it there.
9. xD looks fiiine... - it's widely known to be 70-80 but that rule is so damn old, and it was likely based on the low resolution back then. I propose 100-150 max though.
10. prevalent
11. prevalent
12. prevalent
13. i, I've gotten used to see Foo:BAH for enum member that it's grown on me.
14. can't decide.. I think that mFoo is fine if only that prefix, but if we are to use c for contants, i for iterators etc... then it can get verbose and visually unappealing.
15. ii
16. i, I agree that it should always be bound to the identifier.
17. i, sure, fine by me.
18. prevalent

I'm no nazi when it comes to coding style, as long as it's not too verbose :)

[scribblemaniac]

Since I started contributing code here, I attempted to be mindful of the coding styles. Certainly nothing is completely consistent here, but I came to almost identical conclusions on what conventions were most prevalent. A lot of the conventions are contrary to the way I prefer to do things, but it's trivial for me to add a few spaces or whatever as necessary. I definitely agree with your suggestion for 15-17. A length of 80 characters is fairly standard, but I wouldn't mind a bit more room to work with 😉 . I have no opinion on enum naming, and I don't think 5 is worth the effort of changing. I feel fairly strongly that all files should have a terminating newline (2ii). Looking forward to having official guidelines for all this!

[MrStevns]

Any update on this @J5lx? I've already started to apply some of the prevalent things we've agreed on in my newest PR and also made a config file for "Artistic Style" Formatting for Qt Creator, for those who use Beautifier.

[J5lx]

For the most part I was interested in hearing from @chchwy before making any final decisions. But while I’m at it I might as well address some points right now.

• (1.) I personally really like the advantage of spaces taking the same amount of columns everywhere

• prevalent, while ' i ' can be somewhat easier to read, it can be such a pain to keep track of.

  To me, this reads a little like you prefer i because it is a pain. That seems a little weird, so could you confirm if this is (not) what you mean?

• It’s true that a line limit (8) of 80 characters has an ancient origin (related to currency rather than programming), but I also find it very, very useful for side-by-side diffs and other cases with limited horizontal space. I agree that it can be quite little, though, so that’s why I suggested 80 recommended limit, 120 soft limit and no hard limit.

• I feel fairly strongly that all files should have a terminating newline (2ii)

  I agree completely.

• I don't think 5 is worth the effort of changing.

  Could you clarify what you mean by this, @scribblemaniac? Are you referring to switching to OTBS or are you referring to unifying?

[MrStevns]

Ah right, yes that makes sense.

(1) As I mentioned above, my editors auto convert from tabs to spaces, so if you guys prefer spaces then that's fine with me.

(3) whoops that a typo, I meant I prefer ii (no spaces). I've never personally liked to use the prevalent and I consider it an annoyance to add spaces around all parenthesis.

(8) I'm fine with a recommended (80) and soft limit (120).

[chchwy]

Happy to see some guys raise this issue.

I won't be surprised the coding styles we prefer are different. You might notice that I followed a minimal coding style, which is a subset of my company's coding style so I don't need to configure my editor to 2 settings lol.

1. prevalent
2. prevalent
3. prevalent
4. prevalent
5. a brace always has its own line.

void func2()
{
    if
    {
    }
    else
    {
    }
}

6. prevalent
7. all ok for me
8. all ok for me
9. seems 80 and 120 are good
10. UTF8 without BOM (good for cross-platform)
11. prevalent
12. prevalent
13. prevalent
14. mMemberVariable because my editor doesn't highlight members
15. & is always next to the type type& a;
16. * is always next to the type type* a; for me & and * are a part of type, not name.
17. same as previous
18. prevalent

No matter what the final coding style is, I'd suggest using a style that source files could be automatically formatted by a tool, like AStyle. So that we don't need to worry about the style of pull requests.

[J5lx]

Okay so I took some time to look at this again. First of all, to address a few points:

[…] so I don't need to configure my editor to 2 settings lol.

Many editors allow loading pre-defined code styles per project. The editorconfig I mentioned previously is a file in the project root that is loaded by editors that support it to automatically adjust the code style settings according to the project. A Qt Creator plugin is available, more information and plugins are on the website. @candyface also mentioned a config he made for some other tool.

I'd suggest using a style that source files could be automatically formatted by a tool, like AStyle.

I agree that tooling for this is definitely helpful, ideal would be something that can be run locally and has both a check-only mode for control and a check-and-correct mode for comfort. I’m not familiar with AStyle so far.

Other than that, I compiled the preferences that have been voiced so far in a document on Google Sheets for a better overview. So far we have consent on just under half of the items. Here are the ones without consent so far (where * indicates that other styles are explicitly also okay):

• Pass-by-reference in parameter lists (15), Pointers (16), Pointer pass-by-reference (17)

  • @candyface, @J5lx and @scribblemaniac: * and & bound to identifier
  • @chchwy: * and & bound to type

  It is true that, logically, * and & are part of the type, but as I explained in Apply a consistent code style #683 (comment) binding them to the type can sometimes lead to very confusing code, which is why I feel quite strongly about this.

• 19-28: Need more opinions

Done:

• End of file (2)
• Whitespace in parentheses (3)
• Whitespace before parenthesis (4)
• Braces (5)
• Multi-line function calls / declarations (8)
• Line length limit (9)
• Member var names (14)

In order to achieve consent on these items as well I suggest this:

• @scribblemaniac, it would be nice if you could clarify your position on 5 and 9 (see above) Now clarified!
• Everyone else, consider the above items again and try to think about what is really important to you and what is not.
• I’ll keep this post and the Google sheet updated accordingly

[MrStevns]

To get this going, I've changed my stance on these:
(1) prevalent
(2) one newline (ii)
(3) unchanged
(4) (i)
(5) unchanged
(8) (i)
(9) unchanged
(14) (iii)
(15-17) unchanged

I still feel very strongly about not having spaces and using a coding style that doesn't waste too much vertical nor horizontal space i.e. (OTBS)

I am okay with spaces after comma, before and after operators and outer brackets

[scribblemaniac]

Position clarification
5. i (Allman variation)
9. 80 recommended limit, 120 soft limit, no hard limit

Changed positions
3. ii. I forget to add those spaces all the time. The spaces do make it slightly more readable, but it's not enough of a benefit in my mind to justify the effort.
14. iii (mPascalCase). While it's true that myStruct->member is unambiguous, member variables can still be referenced within struct as simply member which is no different from classes. Plus there is an argument to be made for consistency.

New options
First off I'd like to add some options for function naming conventions. These aren't things that can be added to editor configurations, but they are still part of the code style and they also seem to be inconsistent so I think it's worth talking about.

19. Naming non-boolean accessor functions
    i. get prefix (ex. getColor(), prevalent)
    ii. No prefix (ex. color())
20. Naming boolean accessor functions
    i. get prefix (ex. getPlaying())
    ii. No prefix (ex. playing())
    iii. is prefix (ex. isPlaying(), prevalent)
    iv. getIs prefix (ex. getIsPlaying, currently not used)
21. Naming boolean mutator functions
    i. set prefix (ex. setPlaying(), prevalent)
    ii. setIs prefix (ex. setIsPlaying(), currently not used)
22. Naming slots that are used exclusively for catching a specific signal
    i. on<signalName> (ex. onDurationChanged())
    ii. No prefix (ex. durationChanged()). Obviously this will have to be different from the signal name, or in a different class from the signal.

While were having this talk about code style, we should really talk about what is by far the most important thing for code readability, documentation! The codebase for this project is sadly very lacking in documentation in my opinion. A while back I added Doxygen support for Pencil (https://scribblemaniac.github.io/pencil/docs/), however it never got merged upstream because its useless without at least some documentation of functions and classes. You can learn more about Doxygen here. Here are some options related to the styling of documentation (all of which are equally prevalent since there is no documentation 😥 ):

23. Special comment block
    i. Javadoc style

    /**
    ...
    */
    

    ii. Qt style

    /*!
    ...
    */
    

    iii. Three slashes

    ///
    ///
    

    iv. Two slashes and an exclamation mark

    //!
    //!
    

    v. Asterisk spam

    /********************************************//**
    ...
     ***********************************************/
    

    vi. Slash spam

    /////////////////////////////////////////////////
    /// ...
    /////////////////////////////////////////////////
    

24. Intermediate asterisks. Applicable only if you selected i, ii, or v for the previous question.
    i. Include intermediate asterisks

    /**
     * ...
     */
    

    ii. Skip intermediate asterisks

    /**
    ...
    */
    

25. Brief description format
    i. Always use \brief
    ii. Autobrief, ends the brief description at the first '.' encountered. Use \brief if you want more than one sentence.
    iii. A /// or //! comment before the main comment block
26. Documenting member variables
    i. Add comment before variable (using same style as 22)

    /** Description of var */
    int var;
    

    ii. Add comment after variable (using same style as 22 except with <)

    int var; /**< Description of var */
    

27. Special command format
    i. @<command> (ex. @param)
    ii. \<command> (ex \param)
28. Parameter descriptions
    i. Param command

    /**
     * @param[in] var String to print
     */
    void print(string var);
    

    ii. Inline comment

    void print(string var /**< [in] String to print */);
    

Positions on new options

19. i
20. i but iii is fine too
21. i
22. i
23. i but ii is fine too
24. i
25. ii
26. i
27. i (flexible)
28. i

[J5lx]

Thanks for the update, @scribblemaniac, I updated everything and also changed my own stance on 14 to iii, so we now also have consent on 9 and 14. Also thanks a lot for bringing up those other points which I totally didn’t think about, especially the documentation ones. Speaking of which, I think you shouldn’t wait too long with PR’ing that Doxyfile; it’s more motivating to work on something like documentation when there is some tangible result like a nicely browsable site. I added all your points to the OP for a better overview. As for my own positions on those points, I mostly agree with you:

19. i
20. i but iii is fine too
21. i
22. i
23. i
24. i
25. ii but i is fine too
26. i
27. i (flexible)
28. i

There are actually a number of doc options there that I have never personally seen used in the wild: 24ii, 25iii and 28ii not at all and 23v and vi only when no documentation generator was used. As for styles, it seems there are a handful of C++ projects using Qt style and \command but the Javadoc style with @command seems to be the de-facto standard elsewhere including pretty much all other language communities (that don’t have their own home-baked format) (e.g. Java (duh), PHP, JavaScript, just to name a few popular ones). As a result, I’m much more used to the Javadoc format than to the Qt one, and I imagine this might also be true for other people.

[scribblemaniac]

@J5lx Thanks for the updates and providing your positions.

I think you shouldn’t wait too long with PR’ing that Doxyfile; it’s more motivating to work on something like documentation when there is some tangible result like a nicely browsable site

I agree, although there is still a bit of work to be done before I would consider it ready. I will bump it up on my todo list.

[MrStevns]

You indeed are raising a valid issue @scribblemaniac, documentation would be nice to have, especially for newcomers that doesn't get the codebase. What are you missing before you would consider it ready?

My stance on the new options are the same as yours.
19. i
20. i
21. i
22. i
23. i
24. i
25. ii
26. i - although I would argue that if a variable needs documentation, then it should probably be rephrased to something more self explained.
27. i
28. i

[scribblemaniac]

What are you missing before you would consider it ready?

I'll go over all of the options again just to make sure they are all what I want them to be. I would like to modify the styling a bit more because it is ugly right now I my opinion, but I might just leave it if don't have enough time. Most importantly, I would like to set up some way of automatically regenerating the documentation on the gh-pages branch when the code on master is changed. Unfortunately I do no have very much experience with travis and all that so if someone would like to help me with that I would very much appreciate it.

[J5lx]

In order to keep this already long discussion focused on code style, I created a separate issue about the documentation: #716.

[MrStevns]

I've noticed recently that you've been making some style changes @chchwy, specifically it seems you've changed your stance on 3. One space each (i) to no spaces.

am I correct in that assumption?

[chchwy]

@candyface yes, it's correct. :)

[J5lx]

I updated the spreadsheet and the summary post. I should really try and find the time to get this going again. Not to mention the exporter…

[MrStevns]

It's been a while now since any progress has been made here, hasn't it?

While browsing Krita source code some time ago, I bumped into their "HACKING" file.
I suggest that we make a similar file, naming it STYLE or CODING_STYLE or something instead though to make it a bit more obvious.
https://github.com/KDE/krita/blob/master/HACKING

If you all agree on something like this, I could try to put something together today based on our current agreed choices.

While i'm at it, because it doesn't seem like this will ever get finished if we don't make compromises, I'm changing my stance on (5) II -> I
That means that (5) should be done too.

We still need to agree on pointer and reference position though.

[J5lx]

Sounds like a good idea. I also updated the summary; it’s really just the pointer / reference position now where we don’t have consent and I’d also like to have @chchwy’s opinions on the documentation style (19-28) (or just a clear statement that it doesn’t matter, should that be the case).