Coding style

[Quintus]

@brianvanderburg2 brought up the coding style question in the forums. Personally, I hate tabs. And I hate if and for braces on new lines (this is only OK for functions, classes, namespaces, i.e. large elements). I also prefer an indentation of 2 spaces.

I’ve started a coding styleguide for SMC here. I would love to hear comments on that, but note it’s not complete yet.

Valete, Quintus

[datahead8888]

My view is that tabbing for indentation should be consistent (at least within a file), and that it should be done with the tab key rather than pressing the space bar X times. Should everyone's editors easily work with the tab key and the 4 space standard?

I've been using the Java conventions generally for naming C++ classes and methods, but I'm aware that C++ != Java and that people are going to like other conventions. These naming and capitalization conventions need to be closely followed to avoid confusion and frustration amongst developers.

I was going to say you might run into some backlash on using Hungarian notation, but your proposed standard looks simpler than some of the conventions used by some people. I would like to avoid to avoid prefixes that look like Greek (ie "rgfpBalances" - http://en.wikipedia.org/wiki/Hungarian_notation). Is there a "Hungarian Lite" standard out there?

I used to always code if statements and for loops with a { on the line of the for or if statement, since this is how I was originally taught:

if (condition) {
    logic;
}

I eventually decided to change my own coding habits and write them like this:

if (condition) 
{
    logic;
}

My view is that:

• The { symbols line up here at the level of indentation
• It's consistent with the convention Quintus proposed for methods
• Having a { symbol on the line above (first example) looks less uniform and is thus slightly more confusing when glancing at code

I realize I'm clashing with Quintus, but that's how these things go...

I've also gotten in the habit of always using { and } for if statements (even if they only have one statement). I've heard of corporations having major bugs because people misread single line if statements. I don't really feel strongly one way or the other on this point.

Are you saying that normal source files will only #include core/global_basic.hpp rather than any other library header file? You are correct that order of includes easily becomes an issue in projects.

I love your documentation requirement. I saw some code that appeared to be missing it. We should also require comments within methods if they are not trivial so as to explain sections of code.

There should also be a standard not to hard code text that is visible to the user. All text should come from translation files. This is obvious, but people new to it might miss this.

[brianvanderburg2]

My style is usually as this:

• I usually use 4 spaces instead of tabs.
• I usually code with the opening slash on the next line instead the same line of if/for/while/etc.

if(...)
{
    while(...)
    {
    }
}

• In classes, public/private are on the same line as the bbrace but declarations are indented. Macros may not be indented

class MyWindow : public wxFrame
{
public:
    MyWindow(...);
protected:
    MyObject m_data;

DECLARE_EVENT_TABLE();
};

• I do switch statements a little differently I guess

switch(...)
{
    case 1:
        ...
        break;
    case 2:
        ...
        break;
    default:
        break;
}

I think the opening brace on a new line looks better especially when there are multiple lines in the condition.

if((condition1 < something) && (condition2 > something) &&
   (condition3 < something) && (condition4 > something))
{ // This is the start of the block
    perform_some_action();
    return results;
}

if((condition1 < something) && (condition2 > something) &&
   (condition3 < something) && (condition4 > something)) { // This is the start of the block
    perform_some_action(); // This is really kind of messy
    return results;
}

Since there are so many styles, no matter what the choice used for this project is, it will be different than that of some developer somewhere. I'd recommend including the style guidelines in the source code as well, such as codestyle.txt or something. Maybe add to the top of each source file a vim line or something that sets tab stop, shift width, and whether to expand tabs to spaces or not.

[datahead8888]

I would say that for some things like class/method naming and having comments that everyone pretty much needs to follow agreed upon standards but that for other things like whether { is on the same line as an if or the next we should let developers each choose. Indentation spacing should be consistent within a file so that things line up; I'd prefer not to have to press the space bar repeatedly one way or the other, but making things line up is what is more important there.

[datahead8888]

I found old coding standards: http://www.secretmaryo.org/wiki/index.php?title=Coding_Standards

[Quintus]

I found old coding standards: http://www.secretmaryo.org/wiki/index.php?title=Coding_Standards

I didn’t even know them. Well, we can freely discuss what to use and what not to use.

Put a space after the opening parenthesis and the closing one

This drives me mad. In 96% of cases I simply forget this space, and I don’t see the advantage of it being there. Use a proper editor that highlights matching parentheses for you if that’s a problem.

When declaring pointer data or a function that returns a pointer type, the preferred use of '*' is adjacent to the data name or function name and not adjacent to the type name.

Appearently, this is a point where C++ coders divide up into two halfs. I’ve seen both styles equally distributed, and I prefer the star to be placed at the type name, as I view it more of being part of a variable type rather than being part of a variable name. This seems more logical to me.

Indent your code with a tab character and default tab size is 4 spaces.

I’m not sure if we already reached consent on indenting with spaces rather than tabs. Tabs have various problems:

• You have to ensure everyone uses the same tab width.
• GitHub always uses 8 spaces per tab, and there is no way to change that. The code will look completely apart when viewed via GitHub.
• You can’t properly align a multi-line expression. Take something like this:

MyAwesomeFunction(arg1,
                  someotherverylongargument,
                  yetanotherlongcomputationalargument
                  arg4)

You can easily align that with spaces, but with tabs you are forced to first indent with tabs and then pad the last distance with spaces. Awful.

Super-long lines should be avoided, code readability is drastically diminished by them.

This gets even worse when you want to align variables assignments.

int myfirst_variable       = 3;
std::string other_variable = "foobar";
float x                    = 0.0f;

Aligning assignments this way looks much better than having the equal signs distributed over various distant places. However, shall you align that with tabs? What do you do if an equal sign does not exactly fall onto a tab width, but a quarter of a tab or so?

I used to always code if statements and for loops with a { on the line of the for or if statement, since this is how I was originally taught:

SMC’s code has some very, very long if-elseif-elseif-elseif-else statements, from which some are only one-liners. This results in something like this (but image this over several hundred lines):

if (condition)
{
  stuff();
}
else if (condition)
{
  otherstuff();
  if (othercondition)
  {
    dothisandthat();
  }
  if (xxx)
  {
    foofoo();
  }
}
else if (condition)
{
  evenotherstuff();
}
else if (condition)
{
  blubb();
}
else
{
  foo();
}

During the libxml++ move, I reformatted many of them to look like this:

if (condition)
  stuff();
else if (condition) {
  otherstuff();
  if (othercondition)
    dothisandthat();
  if (xxx)
    foofoo();
}
else if (condition)
  evenotherstuff();
else if (condition)
  blubb();
else {
  foo();
  bar();
}

This looks much more consise, and it eases the overview over this piece of code. Not having to scroll all the time to understand the code is an advantage.

Vale, Quintus

[brianvanderburg2]

What line ending encoding should files use. I've checked a lot of the files and many seem to be UNIX line endings. I just pulled changes from quintus/devel into my devel and it looks like one of the files brought in was almost entirely line ending changes only. Also, I think the file level_exit.hpp is DOS while many others (even level_exit.cpp) are UNIX.

[Quintus]

I would prefer UNIX line endings, i.e. \n. That’s how most programming languages represent line endings anyway.

and it looks like one of the files brought in was almost entirely line ending changes only

I did not do such a thing. I think you refer to the lvl_3 file I edited, but that wasn’t line ending correction, but simply editing the file in the level editor to add some scripting. lvl_3 has not been touched for a long time, so on that occasion the level file has been converted to 2.0.0 level format from whatever it was previously. Level files are more like binaries, so don’t count on their diffs much.

Vale, Quintus

[brianvanderburg2]

I prefer UNIX endings as well. And it was the level file that you mentioned so that makes sense. When saving level files, does the save code use the same ending on every platform, or will a level saved on Windows use DOS endings while a level saved on Linux use UNIX line endings?

[Quintus]

When saving level files, does the save code use the same ending on every platform

This is something that’s completely left to libxml++. I don’t know what it does in this regard, but for XML it doesn’t matter anyway. You could leave off the newlines alltogether and it would still work.

Vale, Quintus

[Luiji]

I'm generally good with any tabulating style, but if there's one thing that bothers me is inconsistent bracing style in a single block.

Specifically, I hate this:

if (x == 15)
  printf("OK\n");
else if (x == 16) {
  printf("Multi-line");
  printf("\n");
}
else
  printf("Single line again\n");

Personally, I prefer to use braces in all situations (including single-line statements) for maximum consistency, but I'm always willing to give that up. However, inconsistency in the bracing style is unclean and can result in bugs.

Also, one clarification that would be nice is whether or not assignments are preferred right before or within an if statement. E.g.:

int x = superFunction();
if (x == 42)
  doSomething();

// or

if ((x = superFunction()) == 42)
  doSomething();

I generally do the former but I have no strong preference. I just think it's something that would be useful to clarify in the documentation.

Also, is our Doxygen configuration using the auto-brief feature, or should we manually specify @brief/\brief? And if we do use such syntactic constructs, should we use the @ style or \ style?

Lastly, it would be useful to clarify whether we want to allow multiple declarations on a line or not. If we do, we should move the pointer/reference part to the name, since it doesn't transcend the whole line.

int* x, y, z; // sort of looks like they're all pointers, but only x is
int *x, *y, *z; // makes it more clear

// alt. just have everything on a different line
int* x;
int y;
int z;

[Quintus]

Also, one clarification that would be nice is whether or not assignments are preferred right before or within an if statement.

I tend to do this with methods returning pointers, something like this:

foo* p_foo = NULL;
if ((p_foo = Do_Something()) {
  p_foo->Cool_Method();
  // ...
}

I’m open for criticism against that, but I find that’s a nice shortcut.

Also, is our Doxygen configuration using the auto-brief feature

Currently no autobrief, but /// lines in headers (triple / is the same as using the \brief command) and full /** documentation blocks in cpp files. This allows to use the raw headers as a mini-reference while programming, so you don’t always have to switch to a browser or search the source file for the corresponding comments. Headers should always give a brief overview over all functions available, plus a short description. SMC already did this, but without employing Doxygen to collect the brief statements. As I’m a person who likes to keep one or two header files open next to the sourcefile I’m editing currently, I like it if the header file is not cluttered with large comment blocks, but still provides a brief statement on what a function does if it’s not obvious from the signature.

Lastly, it would be useful to clarify whether we want to allow multiple declarations on a line or not.

When you declare a lot of variables of the same type, having them all on different lines tends to be an annoying repetition. I would vote for allowing multiple declarations on one line.

Specifically, I hate this:

You will find I used exactly this style in SMC’s code quite a few times. However, I will abandon that habit if you think this is too bad. Maybe we can agree to always use braces except for this case:

// Code...

if (condition)
  SomeOtherCode()

// Further code

That is, for real one-line if statements without further else if or else parts.

What about brace position? I already pointed out I think that having braces on the next line blows up large if statements quite a lot, especially for one-line ifs with many else-if parts. The more code I get on a single screen page, the more overview I get and the less I have to scroll around.

Vale, Quintus

[Luiji]

assignment in if statement I’m open for criticism against that, but I find that’s a nice shortcut.

The only real argument I recall against it is that it's difficult to quickly find mistakes where = is used where == is intended, but it honestly looks fine to me. (I can quickly find those expressions and determine whether it should be an == or = myself.)

doxygen stuff

OK. Yeah, I don't like too much clutter in the function documentation, either. What's your opinion on \param documentation? Would you rather the parameter information be quickly overviewed in the brief?

Personally I think briefs should be a quick introduction and the implementation should be clear enough that further details can be found by examining it.

brace stuff

I'm fine with ignoring braces when all of the conditions ignore them, e.g.:

if (condition1)
  code1();
else if (condition2)
  code2();
else
  code3();

and

if (condition) {
  code1();
  code2();
}
else if (condition) {
  code2();
}
else {
  code3();
}

I just don't like mixing braces in a single block:

if (condition) {
  code1();
  code2();
}
else if (condition)
  code2();
else
  code3();

I do have one general exception to that rule: when blocks are indented further like a statement as in GNU:

if (condition)
  {
    code1();
    code2();
  }
else if (condition)
  code2();
else
  code3();

This is because that coding style treats compound statements as a unique indent-able statement. It's all about how it's treated, in my opinion.

I like placing the braces on the same line as the expression. I think it saves a lot of room.

What's your opinion on brace cuddling? E.g.

if (c) {
  // code
}
else {
  // code
}

// or...

if (c) {
  // code
} else {
  // code
}

I've found a lot of code looks cleaner with the later (cuddled) mechanism than the former (uncuddled) mechanism, but either is completely fine by me.

Also, how should we handle switch indentation?

switch (expression) {
  case indented:
    // like this
    break;
  default:
     break;
}

// or...

switch (expression) {
case indented:
  // like this
  break;
default:
  break;
}

Personally I find the former better for shorter switchs and the later better for longer switchs, but I'm open to sticking to one particular style.

[datahead8888]

if (condition)
  stuff();
else if (condition) {
  otherstuff();
  if (othercondition)
    dothisandthat();
  if (xxx)
    foofoo();
}
else if (condition)
  evenotherstuff();
else if (condition)
  blubb();
else {
  foo();
  bar();
}

@Quintus, a good portion of your savings here are from converting single line if statements with { and } to single line if statements with no braces. Honestly, in some ways I still like single line if statements with no braces (probably because I taught an Intro to C++ class 4 times in which the course materials never told students they can do this), but I honestly still can't agree with a { on the same line as an if statement or for loop for general readability reasons.

My thought is to not standardize these 2 items with curly braces and let developers choose which way to code them. By not forcing people to code one way or another for the much smaller things like this, it will encourage people to want to come on board and help out. As long as they don't do something really, really weird with their curly braces, I don't foresee it becoming a problem. If necessary, we could make a standard about not doing barbaric things.

 Put a space after the opening parenthesis and the closing one

I also agree that having to write code this way would drive me mad. It wouldn't bother me, though, if someone else wrote their code this way. Thus I wouldn't suggest making it a standard either way.

When declaring pointer data or a function that returns a pointer type, the preferred use of '*' is adjacent to the data name or function name and not adjacent to the type name. 

Is it saying to do this:

int * myPtr;

Above is how I have always done it. How else would you declare your pointers?

 I’m not sure if we already reached consent on indenting with spaces rather than tabs. Tabs have various problems

Per Tom Wijsman in a reply at http://programmers.stackexchange.com/questions/57/tabs-versus-spaces-what-is-the-proper-indentation-character-for-everything-in-e: Tabs

• allow mistakes to be more noticeable, it's annoying to micro manage to spaces.
• are consistent, they are only used for indentation, unless you are commenting wrong.
• are customizable, you can specify in the options of the editor how wide they are.
• are more productive, why type 4 spaces when ye can hit 1 tab.
• take less size than multiple spaces.
• take more space than a space.
• take less time to go through.
• tend to work better in IDEs.

My only point is that there are a lot of different views out there on this. What concerns me is hammering the space key again and again and again in order to achieve every indentation level. The point about 8 spaces being used for tabs in Github is a very good point; could everyone configure their tabs to be 8 spaces in their text editors?

If there's a way to map the tab key to spaces in text editors and that can auto adjust these spaces to a width like setw does in C++, I'd have no concern with using spaces.

I think all line endings should be unix based. Seeing ^M's can be annoying. As far as I know, text editors are available in Windows that can properly deal with Unix based line endings.

Also, one clarification that would be nice is whether or not assignments are preferred right before or within an if statement.

It actually does take a little more effort generally to read code that has assignments in an if statement, but this is another one I would not suggest standardizing.

Lastly, it would be useful to clarify whether we want to allow multiple declarations on a line or not.

I think that multiple declarations on the same line should be allowed but would not suggest standardizing this.

What's your opinion on brace cuddling?

It's not a way I would want to code, but I don't have a problem with other people writing their code that way.

 Also, how should we handle switch indentation?

I like the first choice that Luiji gave.

Lastly, we will need to decide whether to use:

using namespace std;

cerr << "Tor!" << endl;

or

std::cerr << "Tor!" << std::endl;

I think the latter is verbose unless you have a lot of namespace conflicts that require it.

[Quintus]

OMG so much text!

What's your opinion on \param documentation? Would you rather the parameter information be quickly overviewed in the brief?

No \param in the brief line, the method signature should speak for itself. Always use parameter names when you declare a function, even though it’s not required! You can put \param in the more complete documentation comment block in the .cpp files.

Luiji wrote:

I just don't like mixing braces in a single block: datahead8888 wrote: My thought is to not standardize these 2 items with curly braces and let developers choose which way to code them.

OK. We will not mix brace style in a single block, but otherwise don’t require a specific one. Can we agree on that?

What's your opinion on brace cuddling?

I’m slightly irritated. I’ve not yet seen this, and not having the else keyword at the beginning of the line confuses me.

Also, how should we handle switch indentation?

I prefer the latter one, because that’s how Emacs indents it by default :-P. Really, choose whatever you want, I don’t think this makes any difference.

I also agree that having to write code this way would drive me mad. It wouldn't bother me, though, if someone else wrote their code this way. Thus I wouldn't suggest making it a standard either way.

Agreed. We will lift this hard requirement and let people do it to their likening.

int * myPtr;

This is how mruby does it, and it’s a good compromise. Previously, I always did it like this:

int* myPtr;

If there's a way to map the tab key to spaces in text editors and that can auto adjust these spaces to a width like setw does in C++, I'd have no concern with using spaces.

Every good text editor allows you to do this. Emacs and vim can do that for sure.

I think all line endings should be unix based. Seeing ^M's can be annoying. As far as I know, text editors are available in Windows that can properly deal with Unix based line endings.

Yep.

---

We probably don’t want to standardize too much. Instead I suggest we agree on what should not be done, to ensure a minimum coding standard is maintained across SMC’s codebase.

Vale, Quintus

[Luiji]

OMG so much text!

Well, this is one of the most-argued, most-opinionated topics in the whole of programming. =P

My main things about tabs vs. spaces is this: any good text editor allows you to press tab to indent by spaces. Any bad text editor isn't worth using or encouraging the use of, but since the tab indent is only two spaces it's only the difference between one key press and two.

The primary advantage of spaces is that they're consistent by default. With spaces, you'll never have to configure your editor to simply read it, only when you want to write it. With tabs, you'll have to configure it both when reading and writing.

Don't forget about the environments where you can't make these configurations, like the web browser. In these situations, tabs are absolutely the most useful. We don't want to make people download the text, a better editor, or anything just to read it the way it was intended, do we?

Of course, though it may sound like I'm passionate about this, I'm not, and I'd happily work with normal tabs. =)

[Quintus]

OK. We will use spaces. Any objections that arise even if using a proper editor?

Next thing: We will use only UNIX line endings. OK?

Doxygen style accepted as suggested. /// for headers for brief summary, /** for .cpp files where needed.

Finally: We forbid mixing styles in a single logical block, but otherwise don’t enforce a specific style. Put braces wherever you want them, but stay consistent.

I will update the styleguide accordingly.

Valete, Quintus

[datahead8888]

Sorry for the slow response.

 OK. We will not mix brace style in a single block, but otherwise don’t require a specific one. Can we agree on that?

If a block is defined as a "working area", yes, I can agree with that standard. A block may be one while loop within a larger function or 3 levels of tightly coupled for loops - it's relative. If a function is sprawling, I think it's reasonable to have different styles within different working blocks inside of it. I hope this still doesn't give rise to an edit war :P

 Every good text editor allows you to do this. Emacs and vim can do that for sure.

Just to be clear

   j          //3 leading spaces
     j        //5 leading spaces

For both of these, if I configure an editor such as vim or emacs to map tab to spaces, if the text cursor is to the right of the letter j, will the tab key line these up evenly? This is what I was getting at when I talked about having something like setw in the C++ iomanip library.

[Quintus]

For both of these, if I configure an editor such as vim or emacs to map tab to spaces, if the text cursor is to the right of the letter j, will the tab key line these up evenly?

I can’t speak for Vim, but Emacs handles it exactly this way. If you want a real tab in your line, you have to quote it with Ctrl-q <tab>.

Vale, Quintus

[Luiji]

Vim can handle it, too. I've only recently started using Emacs, by the way. They have pretty much equivalent indentation settings, though Emacs seems to have simpler (or at least more in-your-face documented) procedures for auto-indentation.

[Quintus]

default0 was of the strong opinion I should enforce at least parts of a coding style in SMC, especially with regard to naming conventions. I see from the discussion in this ticket that it will be hard to reach a consent on stuff like brace positioning, and I still advocate you shall do that as you like, just do it consistent with the surrounding code. If SMC’s codebase thereby enters a way to unreadability, I will intervene there and set up a style guideline for the specific problem at hand.

Regarding naming, default0 is right in saying that changing naming conventions later is difficult. This already applies to the existing codebase, so I would strongly advise we keep the naming scheme that already exists in SMC’s code. This mainly boils down to the following conventions:

• Classes are named cClass_Name.
• Methods use uppercase naming. My_Awesome_Function()
• Member variables start with m_. I also think member pointers should start as mp_.
• Variable names are lower_snake_case.
• Constants and macros use UPPER_CASE().

These are the most important naming things off the top of my head. Please feel free to point out other naming stuff that needs to be mentioned.

I propose this naming scheme as a strong suggestion as changing SMC’s current code naming conventions will impose a real lot of work. There may be other strong arguments to do that, though. Feel free to discuss.

I would also be very happy if default0 would finally give himself a small touch and join the discussion about coding style right here in the tracker rather than only doing it excessively on IRC.

Vale, Quintus

[datahead8888]

Conventions for class names and function names are important to make code modifiable, since they get called in other source files. It probably is a good idea to require people to use m for member variable names because these are used across many methods in source files, and this means non member variables cannot use m. I like the constants capitalization standard -- I think this is what most people do. Global variables should probably be outlawed unless they're required to make old C API's work correctly.

I agree we should not standardize braces for the reasons we already discussed. If additional standards need to be added because of problems that start emerging, we should be conservative with them.

We will need to decide how standards will be enforced. Will we send it back to the coder and tell them to rewrite code per standards, or will we update it for them? Some of the people deep into the project will probably say, "okay", and make the changes. Other people will implicitly say, "Fix it yourself" and sort of disappear, leaving us to decide what to do with potentially useful code that does not abide per standards. I think we will have to use a combination of the two enforcement standards in order to use whatever code we get from people while still working together as a team to improve code together. I'd also be interested in using github's code review features to streamline code reviews; this may eventually reduce the burden on Quintus.

@Quintus - there is a simple solution to get people with ideas to post in github. Just make it a rule that if someone has a non trivial suggestion and wants it to be considered, they are required to post in the correct place. In some cases that may be github; in others it may be the official forums or even the alternative forum. IRC generally will be for auxiliary discussions (supported by postings somewhere) and for really simple suggestions. If someone feels strongly about something, they'll post in the appropriate place.

[Quintus]

We will need to decide how standards will be enforced. Will we send it back to the coder and tell them to rewrite code per standards, or will we update it for them?

It will be a per-case decision. Simple things can easily be fixed by us, for more complex things the author is more into the code than other people. Also, GitHub’s per-line comment feature can play in here very hand to point out specific problems right at the line where they are. You can also comment on single commits; a PR streamlines this and allows for a good overview.

Just make it a rule that if someone has a non trivial suggestion and wants it to be considered, they are required to post in the correct place

I agree to this. Even worse, the lengthy discussion I had with default0 was by PM, so it is not even in the chatlogs

Vale, Quintus

[datahead8888]

 Also, GitHub’s per-line comment feature can play in here very hand to point out specific problems right at the line where they are. You can also comment on single commits; a PR streamlines this and allows for a good overview.

Can anyone participate this way in code reviews, or is it just people with write access? I assume it will always go back to someone with write access ultimately before committing, but if they see it's code reviewed, they would be able to just commit it, theoretically.

[Quintus]

Can anyone participate this way in code reviews, or is it just people with write access?

Everyone can participate, unless I personally "lock" an issue/PR. Then only people with write access will be able to comment.

Vale, Quintus

[default0]

We should keep bracketing consistent. I don't care what style is used, but we should enforce a particular style throughout the whole project.

The reasoning behind this is that 1) With OOP doing lots of implementation hiding and virtual calls you cannot rely on scopes to be a valid area for consistent bracing as you will have tons of different scopes and classes written by different people with different bracing styles that you need to potentially cover to understand a given segment of code. 2) That bracketing + indentation are really just fancy ways for highlighting control flow most of the time, and thus keeping them consistent really helps you figure out all the scopes and control flow just by the looks of the code without even reading anything about the ifs and fors and switches, changing it up across multiple files is gonna counteract this natural "preprocessing".

Incase people don't follow me on 1: Assume you have: a->b.c();

You have to check out: -What a can be in the current context (+ eventual subclasses etc) -What b is for a (+ eventual subclasses etc) -What the call to c is for b

So you have to scan through potentially 3 whole hierarchies to understand what is really going on in this code, which means that keeping scoping only consistent within the same class or function is really not enough.

About 2, well I cannot really demonstrate this effect to you inside a single file, because this is kind of what we agreed to not do for now, however its equivalently bad across multiple files (side-by-side view, crawling through hierarchies to understand implementations/consequences of calls etc) due to 1.

How bad it is, well see for yourself:

And, as I said, due to 1, this is as bad across mutliple files as it is across a single file.

[Quintus]

Personally, I do not find that code too hard to read; but also note that this code appears to be a single function implementation. As per what has been written prior in this discussion, that defines a logical block and as such would have a single coding style anyway.

Apart from that, you still have to consider some things.

1. We need programmers. Rejecting PRs because they have their braces wrong is probably not of much help here; if I tell people they have to rewrite their code because of minor stylish issues we risk losing them.
2. Coding styles must be enforced. I cannot go through all commits and look whether everyone uses the proper coding style. The same goes for PRs: There will be people that just make a PR and then disappear forever. If the code doesn’t match our style, we will have to handfix. Requires more than one person reviewing PRs.
3. It is impossible to agree on which coding style to use. Everyone wants to write code differently.

With OOP doing lots of implementation hiding and virtual calls you cannot rely on scopes to be a valid area for consistent bracing

I think you somehow miss the point of OOP. Yes, OOP does lots of implementation hiding. But that is for the goal of not having to dig around in the implementation of something. You rely on the interface on itself. If you have to dig around in the implementation of a class to understand the class, then this is bad OOP design. With regard to C++ usual code layout, this means that the header file must convey enough information to properly use the class. As a consequence, the brace style or other things used in the implementation can come from /dev/urandom, you don’t have to care.

Also note SMC’s codebase is really large. It is impossible to keep track of all implementation details, so trying does not make sense. OOP eases this by defining interfaces (classes) you can rely on.

You have to check out: -What a can be in the current context (+ eventual subclasses etc) -What b is for a (+ eventual subclasses etc) -What the call to c is for b

Your example is flawed, or better incomplete. If you have to look up the implementations of either a, b, or c, the interface is badly designed (and/or badly documented). You look at the required class and function definitions, and be done with it. You do not care about the control flow inside the c() function.

Also note that GitHub has a Code highlighter, you don’t have to use pictures. Have a look here.

Vale, Quintus

[default0]

"I think you somehow miss the point of OOP."

No, I don't, you miss my reasoning. You read code mainly when you either try to get familiar with a codebase or when you try to debug code. In both these cases the actual implementation does matter quite a lot. If I am new to the codebase and want to implement an enemy that has some combined traits of other enemies, I go look up their implementation, because I need to familiarize myself with how things are done in the codebase. If I see some entity or sprite behaving buggily, I go look up its implementation, not its interface.

The one case where OOP comes into play and shows its golden glory is when modifying or extending working code by further subclasses or implementations. In every other case where you actually read code, you do very much care about your specific implementations (especially when debugging).

So yeah, my example is not flawed, your understanding of the intention of code readers is.

[Quintus]

No, I don't, you miss my reasoning.

I don’t think so. On the contrary, you do not take position to the arguments 1-3 I listed.

You read code mainly when you either try to get familiar with a codebase or when you try to debug code. In both these cases the actual implementation does matter quite a lot.

Yes. In case of debugging, the implementation is the thing that really matters of course. Would be weird if it wouldn’t. But still, you do not read the entire codebase. You read those files that affect the buggy behaviour in question and then dig around that. You are not reading all the class implementation (some classes are just too large), but only the methods that could affect the buggy behaviour, reyling on the interface defined by OOP. You only "break into" the implementation of an interface where absolutely required, which even for bugfixing will only be a fraction of classes and methods involved. Plus, inside these logical structures, you will only ever find the same coding style. When you cross a logical border, such as when you pin down your bug to a method call inside a method, style may change, but this does not really hinder debugging. Code style is a mere indicator to control flow, and you should never rely on it. Your task as a debugging programmer is to understand the code, not guess what it does from the coding style. There are cases where it even can get extrelemy misleading, e.g. with unintended formatting errors such as spaces being inserted instead of tabs due to editor misconfiguration. If the bug in question even is a crash, you can fire up gdb or similar, and you will have a very distinct view of what implementations to look at.

As for learning the codebase, I assure you getting familiar with a codebase as large as SMC’s takes a very long time. I work on it since over a year and still have no thorough understanding of the code in its totality. What coding style is used in which files becomes a very minor question when dealing with that much code.

You also probably underestimate the power of simplicity. If you edit code for whatever reason (bugfixing, feature adding), you will just adapt the code style found around the code you want to insert/modify, unless you have a very strong opinion on a particular styling issue, which you should probably raise in public then, as has been done with tabs in this issue and the spaces between the parentheses. Relying on common sense, PRs that use appearently extraordinary weird coding style, as opposed to readable code with whatever style the author preferres, will just not get in.

So yeah, my example is not flawed, your understanding of the intention of code readers is.

I object to this statement. You have an indication as of what object behaves buggy. In most of the cases, you will be able to rule out two of {a, b, c} and then look up the implementation of the thing remaining.

If I am new to the codebase and want to implement an enemy that has some combined traits of other enemies, I go look up their implementation, because I need to familiarize myself with how things are done in the codebase.

Yes. You can do that, even if each enemy is coded using another coding style. Does this hinder your understanding of the code? There are much more grave problems than coding style; people abusing exceptions or breaking core OOP principles such as secrecy. These do hinder the understanding of code, and should be prevented. Coding style, which you can’t agree upon without having some people always object to the particular style chose, is only a very minor point on the understanding of code.

Finally, I have a very simple argument. Coding style is not a problem currently, and we want SMC to move forward now. We should care about problems when we have to deal with them, not months or even years in-beforehand. This discussion is an example of over-planning; we want to solve something that may not ever arise as an urgent problem. If we plan and plan and plan and discuss and discuss, we will probably never get around to code something.

Vale, Quintus

[default0]

Or if we have a project that requires you to set up 10+ dependencies before you can get it to compile, that also kinda hinders you to code something... much more so than code style does.

Aside from this, let me recap then.

"1) We need programmers. Rejecting PRs because they have their braces wrong is probably not of much help here; if I tell people they have to rewrite their code because of minor stylish issues we risk losing them."

I doubt that people generally throw in 1k+ lines of code, so fixing the code they send in manually is a thing. If you don't plan on doing that, work in a less open environment. Also, as far as I know many editors allow you to change bracketing style rather easily through autoformatting, so you don't need to tell them to change their code, you just need to change their code yourself.

"Coding styles must be enforced. I cannot go through all commits and look whether everyone uses the proper coding style. The same goes for PRs: There will be people that just make a PR and then disappear forever. If the code doesn’t match our style, we will have to handfix. Requires more than one person reviewing PRs."

Again, for bracketing, which is the issue at hand, there is autoformatting, and since a single person is usually consistent in their bracketing, quickly browsing through their code is likely gonna tell you whether their bracketing style is consistent with the project and if not fixing it with auto-formatting.

"It is impossible to agree on which coding style to use. Everyone wants to write code differently."

Which is why projects have leaders that at some point step up and enforce a set of guidelines that are supposed to be strict throughout the whole project, because consistency is the #1 thing you should be a sucker for when writing code.

Now, for the new post

"You read those files that affect the buggy behaviour in question and then dig around that."

Oh, whats this, youre supporting my argument that keeping bracketing style consistent throughout multiple files should be a thing indirectly, otherwise you would just be talking about a single file. Most often you do want to not just look at the direct code that causes some issue (ie some enemy) but also at its caller, which also leads you to multiple files and potentially different styles yet again. And I will be demotivated about debugging and fixing stuff if I need to look at multiple files for control flow and switch between bracketing styles inside them, which, again, when only enforcing consistent bracketing in single files or scopes, is a thing.

"I object to this statement. You have an indication as of what object behaves buggy."

You're forgetting that most often multiple objects are involved through multiple calls, which renders this argument pointless (as this causes you to dig around in multiple files, again with potentially varying bracketing)

"Does this hinder your understanding of the code?"

Generally, all code is understandable. This is not a valid argument because there is nothing that "hinders" my understanding. It hinders the convenience and reduces the overview across these multiple files, which is a hassle and annoying. It does not stop me from understanding the code, however.

"Coding style is not a problem currently, and we want SMC to move forward now."

Step up, enforce coding styles that make sense to you, and be consistent throughout the whole project and there probably won't ever be any issues with coding style ever, because consistency is the key component to this whole thing.

[Quintus]

I give up. You won, please suggest what brace style to use now.

Vale, Quintus

[default0]

The bracket style currently used in SMC (as far as I have seen is) opening and closing brackets on new lines everywhere, except in try..catch where Ive seen the opening brackets on the same line and closing brackets on a new line (which may have been an artifact of that bit of code being inserted later by someone other than FluXy).

I don't see any issues with keeping that just the way it is (other than maybe making try..catch actually have the brackets on a new line as well).

As per regarding the brace style function( param1, param2 ) I agree with everyone else that function(param1, param2) is the way to go. It doesn't really do anything to readability and is what roughly everyone is used to coding with anyways.

[datahead8888]

I support Quintus's decision.

If we are going to use a curly brace style consistently, it needs to be the same for if statements and try catch blocks. We'd need to confirm which convention we want to use and add it to the coding standards document.

As @Quintus pointed out, it will probably be difficult in some cases to enforce this standard for PR's. My suggestion is to make the enforcement a little more flexible. If feasible/appropriate, the code reviewer or original developer can go back and fix this kind of standards violation. If this is not feasible/appropriate, a github issue ticket should be opened documenting the violation. If and when someone has time, it can then be fixed. We will probably never be 100% compliant with our own standards but can approach that as a goal.

[Luiji]

This conversation's spanned about 2 1/2 months and seems to be one of the most discussed topics in the tracker. This should've been as simple as voting between K&R, Allman, etc. and how strict we want to be. The amount of conversation that's going into this has become ridiculous.

Coding style is not nearly as important as the code itself. You can write bad code in any style. What makes a coding guide useful is to provide a level of consistency so jumping from code block to code block won't get as confusing and to guide people who are new to the code base on what style its development community prefers.

Also, who cares if the pull requests completely disregard the coding standards? It's more important that people contribute at all. We can stylize it ourselves when we accept it (a task for which there are many automation tools).

The importance of this subject is microscopic compared to the more vital design decisions such as package management structure, external dependencies, the physics system, etc.

Perhaps these statements would have been more relevant a bit earlier in the thread, but I want to say it anyway to get a point across: if we spend this much time on every minor detail, we'll never get the final product out. We need to learn from how much mental effort's been wasted on this subject and learn to focus on actually writing code.

[Quintus]

@Luiji, I am so glad someone else stepped into this discussion. Allow me to quote myself on the scope of this discussion:

Coding style is not a problem currently, and we want SMC to move forward now. We should care about problems when we have to deal with them, not months or even years in-beforehand. This discussion is an example of over-planning; we want to solve something that may not ever arise as an urgent problem. If we plan and plan and plan and discuss and discuss, we will probably never get around to code something.

But everyone yells at me I shall define a coding standard so I find myself forced to do so... :-(

This should've been as simple as voting between K&R, Allman, etc. and how strict we want to be.

Yes. Oh yes. If you could post links to the styleguides so everyone can have a look at them, we will vote on that and will forever bury this insane discussion into the depths of the issue tracker.

Vale, Quintus

[datahead8888]

Yes, we probably should have just voted or found some other means to come to a decision sooner.

Also, who cares if the pull requests completely disregard the coding standards? It's more important that people contribute at all. We can stylize it ourselves when we accept it (a task for which there are many automation tools).

That's a good point. My suggestion would have overly legalized it. Thanks for your input, Luiji.

[default0]

http://strawpoll.me/2266849 http://strawpoll.me/2266861 lol.

[Luiji]

Yes. Oh yes. If you could post links to the styleguides so everyone can have a look at them, we will vote on that and will forever bury this insane discussion into the depths of the issue tracker.

I think the Wikipedia article on indentation style should suffice to describe each different general category.

https://en.wikipedia.org/wiki/Indent_style

The names are kind of funny: Horstmann, Pico, Whitesmiths...

[Quintus]

Very nice link, though I find it confusing Wikipedia has this under indent style, whilst the article describes programming style in general...

OK. I think we should exclude some appearently nonfitting styles from the vote, as they are not really meant for C/C++. These are the styles towards the end of the linked article.

This leaves us with the following options:

1. Kernel style
2. Pure K&R style
3. K&R 1TBS variant
4. K&R Stroustrup variant
5. Allman style (style used by most of, but not all, SMC currently, with modifications)
6. BSD KNF style
7. Whitesmiths style
8. GNU style
9. Horstmann style

Not all of these styles make suggestions about indentation, but the consent of this ticket was to go with spaces anyway. Therefore, we should also decide which number of spaces to use for indentation. The following options:

1. 2 spaces
2. 4 spaces
3. 6 spaces
4. 8 spaces

So. For each question, everyone has one vote. Between the two most-chosen options, we will vote again.

I want to point out that modifications of the chosen style are possible after the decision if the need arises. There will be times when it is necessary to break the coding style for readability.

Also note SMC’s existing style should not be a limitation on the decision. There are tools to automatically convert between the styles.

End of the first voting phase shall be 2014-08-10 00:00 UTC.

Vale, Quintus

[Quintus]

As pointed out prior in this ticket, I don’t like these brace-cuddling styles. The control-flow keywords belong to the beginning of a line (after the indentation) in my opinion, everything else is hard to read as quite unexpected IMO.

My vote goes to the K&R Stroustrup variant (option 4). It’s also the only variant that explicitely targets C++ :-)

For the indentation, I vote for 4 spaces (option 2).

Vale, Quintus

[carstene1ns]

My votes:

• use 4 spaces for indentation.
• use Kernel style/1TBS for braces (it's actually both the same and also like Stroustrub, but has “cuddled else”)

---

I recommend to use a clang-format script to take care of the conversion:

STYLE="{
UseTab: Never,
IndentWidth: 4,
BreakBeforeBraces: Attach,
IndentCaseLabels: false,
[...more...]
}"
clang-format -i "-style=$STYLE" [the source files]

[Quintus]

use Kernel style/1TBS for braces (it's actually both the same and also like Stroustrub, but without “cuddled else”)

I’ve looked at the site again, and both Kernel style and 1TBS style use cuddled else. So you have actually voted for a cuddled else. But you said this:

without “cuddled else”)

Stroustrup style does not have the cuddled else. Which is why I voted for it. Your answer is confusing :-?

As for the conversion, we will worry about it when we finished voting. @Luiji had a conversion tool also, I just forgot its name.

Vale, Quintus

[carstene1ns]

---without
+++has

Sorry for the confusion, the sentence in braces was actually splitted into 3 parts before and I messed up while assembling. :smile:

[Luiji]

[vote] K&R Stroustrup variant, 4 spaces

Personally I prefer 3 spaces but the width doesn't matter too much to me. I use cuddled braces in some programming languages like Perl but for some reason it always looks weird in C++.

Astyle is an automated C++ formatting tool that we can use: http://astyle.sourceforge.net/ It actually directly accepts options like --style=allman and --style=stroustrup.

[datahead8888]

[vote] Allman style. It lines things up with braces and has a simple, easy to follow model of indentation. [vote] 4 spaces.

@Luiji had a really good suggestion about using automated tools to do formatting conversions. My thought is to maybe let people submit PR's as they feel comfortable and then run a converter afterwards. If we were to expose a server script for the conversion, we could click a button on Quintus's site to do it for us. In the interim, we will probably have to work together to run the formatting tool locally (we could do it periodically to the codebase or add it to PR's as we see fit).

@default0, @brianvanderburg2 - you need to vote. You have 3 hours left to do so.

[Quintus]

-- Vote summary --

Main style:

• K&R Stroustrup variant: 2 votes
• 1TBS: 1 vote (I count @carstene1ns’ vote here as Kernel Style has tabs)
• Allman style: 1 vote

Indentation:

• 4 spaces: 4 votes

---

The indentation has been settled on at 4 spaces. No need to further investigate there, everyone has voted for that.

As for main style, the K&R Stroustrup style got the most votes. I wanted to do a second vote on the two most-voted styles, now we actually have two equally-voted styles on the second place (Allman and 1TBS). Therefore, we will make the second vote on these three styles. Now everyone who hasn’t voted on the first run can do it here. Available options:

1. K&R Stroustrup variant
2. 1TBS
3. Allman style

Today is sunday, and during the work week people generally have less time to look into the tracker. The end of the vote time shall therefore be 2014-08-16 23:59:59 UTC (I saw I messed up with the voting finish last time by using 00:00 wrongly, so I now use the unambigous 23:59:59). [or didn’t I? Hell!]

In case no style gets the most votes, the result from the previous vote will be used as the final result (i.e. K&R Stroustrup variant).

Valete, Quintus

[Quintus]

Again, my vote is for K&R Stroustrup variant (option 1).

Vale, Quintus

[datahead8888]

Option 3 - Allman Style

[brianvanderburg2]

My vote: Allman indent style, 4 spaces, UNIX line endings

// This looks better
if(test1 || test2 || test3 || test4 || test5 ||
   test6 || test7 || test8 || test9 || test10)
{
    call_some_function();
    do_something_else();
}

// than this.
if(test1 || test2 || test3 || test4 || test5 ||
   test6 || test7 || test8 || test9 || test10) {
    call_some_function();
    do_something_else();
}

Spaces between operators and arguments, but not between parenthesis, prefix/postfix operators, unary prefixed operators (!, ~, -)

1 + 2
1 + (2 - 3) * -(14 - data++) // not 1 + ( 2 - 3 ) * -( 14 - data++ )
test1 && !test2
0xFE | ~bitmask
if(test) // not if ( test )
function(x + y, y - x) // not function ( x + y, y - x )
result = condition_1 ? result1 : result2

Local variables: lowercase with underscore Class names: upper camel case, no underscore, optional prefix. Member functions: upper or lower camel case, no underscore Function callbacks for C: lower case with underscore Member variables: m_direct, mp_pointer, ms_static, msp_static_pointer Global variables: g_direct, gp_pointer, gs_static, gsp_static_pointer Access specifiers: line up with class, body indented. Abbreviations: first letter uppercase only, cVfs not cVFS, cHtml not cHTML

class cVfs_RWops
{
public:
   SDL_RWops* Creae(...);
   bool TestIfExists(...); // no underscore

   int sdl_read(...); // Callback for SDL_RWops

private:
    int m_data;
    void* mp_pointer;
    static ms_instances;
};

cVfs* gp_vfs; // not gp_Vfs

Underscores okay when showing a relationship class.

• cVirtualFileSystem (no underscore)
• cVirtualFileSystem_RWops
• cVirtualFileSystem_streambuf

Another idea may be to use nested classes instead:

• cVirtualFileSystem
• cVirtualFileSystem::RWops
• cVirtualFileSystem::streambuf

Switch statement: case indented, body indented again, break aligned with body. default above case if the same, braces only used for scoping if needed and aligned to case:

switch(x)
{
    case 1:  // case indended
        do_something(); // body indented
        break // break also indented

    default: // default before case if they are the same
    case 2:
        do_something();
        break;

    case 3:
    { // braces line up with case
        auto_ptr<int> ptr(new int);

        break; // auto_ptr will be destructed
    }
}

Pointers/references: I prefer keeping it with the type, but I also understand that when declaring multiple values, that doesn't work:

int *x, *y, *z; // three int pointers
int* a, b, c; // one int pointer and two int

[Quintus]

@brianvanderburg2 We will care for all these little details later, for now we just decide on the major coding style. As for naming, we will just use what is the prominent style in SMC currently to prevent major refactoring. I will post about that after the coding style decision is complete.

Vale, Quintus

[Luiji]

I'm still voting for Stroustrup.