OGRE coding and style guidelines

[:wumpus:]

With the upcoming code review phase in mind, we are making up a few coding and style guidelines which apply (or should apply) to all code in the OGRE codebase.

These rules serve to improve the readability and consistency of the code, and seek to remove common non-optimal or deprecated constructs.

A preliminary version can be found here:
http://temas.obelisk.net/ogre/CR/docs/howto.html

http://jabberstudio.org/~temas/howto.pdf (pdf version, may be out of date)


Any suggestions and comments are very welcome and can be posted in this topic.

[Noiprox]

I have some minor observations, though they are hardly even worth mentioning.

It's pretentious to call one style "CORRECT" and others "INCORRECT". How about "Acceptable" and "Unacceptable" as in example 8?

In example 11, you say "prefer STL functions", but the I/O library is not part of the STL, it predates it by years. You could simply say "prefer the C++ standard libraries over the C ones."

[Kentamanos]

Just a readability question...

Any particular reason all the example numbers have A's with "rooftops" (not sure the proper name for this symbol ) around them. For instance:

Example  6.Â

[ggambett]

A couple of suggestions and comments...

Braces or not
The rule I use for multi-block statements (if/else if/else) is that if at least one of the blocks uses braces, all of them must use braces.

Unpleasant :

Code: Select all

if (a == 1)
{
	foo();
	bar();
}
else
	quux();

Pleasant :

Code: Select all

if (a == 1)
{
	foo();
	bar();
}
else
{
	quux();
}

A related one, not addressed in the HOWTO - separate conditions from their blocks :

Unpleasant :

Code: Select all

if (a == 1) b = 2;

Pleasant :

Code: Select all

if (a == 1)
	b = 2;

Variable naming
This is a lost cause but I'll say this anyway. I use the following prefixes, which I feel add to code readability :

Code: Select all

m_ for members
s_ for static variables
g_ for global variables

{ int | byte | char } nInteger;
{ float | double | Real } fFloat;
{ String, string, char* } sText;
{ pointer | object } pObject;
{ vector } vPosition;
{ hash | map } hMapping;
{ list | array } lThings;

Public members don't have m_, therefore pObj->vPosition instead of pObj->m_vPosition

Deleting pointers
I strongly suggest setting pointers to 0 or NULL after deleting them. I have macros which do delete x; x = NULL and delete [] x; x = NULL;

Warnings
Code should compile without warnings in almost all cases.

[Noiprox]

Just a readability question...

Any particular reason all the example numbers have A's with "rooftops" (not sure the proper name for this symbol ) around them. For instance:

Example  6.Â

I had that too. The reason is because your browser is displaying it under a character coding scheme other than UTF-8. I checked the source, which indentifies the document as UTF-8, so it seems to be browser misbehavior, not a problem with the document.

[:wumpus:]

It's pretentious to call one style "CORRECT" and others "INCORRECT". How about "Acceptable" and "Unacceptable" as in example 8?

AGREED

In example 11, you say "prefer STL functions", but the I/O library is not part of the STL, it predates it by years. You could simply say "prefer the C++ standard libraries over the C ones."

Sure, the full order of preference actually is
1. STL
2. C++ standard library
3. C standard library
4. if the situation is really really desperate: Platform/compiler dependent magic

The rule I use for multi-block statements (if/else if/else) is that if at least one of the blocks uses braces, all of them must use braces.

I think that's a sane rule.

Variable naming
This is a lost cause but I'll say this anyway. I use the following prefixes, which I feel add to code readability :

I (personally) don't like the behaviour of putting a type indicator in the variable names, a variable name should convey semantic not type. Types might change, and usually the type can be concluded from the name. For example, mPosition is as clear as m_vPosition to me.

Deleting pointers
I strongly suggest setting pointers to 0 or NULL after deleting them. I have macros which do delete x; x = NULL and delete [] x; x = NULL;

Generally a good idea which fits with the cause "member variables should always have a meaningful initialised value". I'd make an exception for destructors though.

Warnings
Code should compile without warnings in almost all cases.

nothing beats a clean compile this is very hard to evaluate for individual programmers though. Compilers are picky about different things. Someone with VC cannot see wether it produces warnings in GCC, vice versa.

[bad_camel]

Yeah, I can tell you it is pretty hard to get a clean compile out of gcc/g++ with all the warnings on, and impossible with the pedantic flag(since we use a lot of dependancies). In a lot of cases the only way to acheive this is to restructure whole blocks of code awkardly.

It really comes down to what the warning is and how easy it is to use an alternative non-warning prone piece of code.

But lets give it a go anyways

[ggambett]

Deleting pointers
I strongly suggest setting pointers to 0 or NULL after deleting them. I have macros which do delete x; x = NULL and delete [] x; x = NULL;

Generally a good idea which fits with the cause "member variables should always have a meaningful initialised value". I'd make an exception for destructors though.

I think it's especially important in destructors! I know I've lost many hours debugging code that tried to use a deleted object (before I started using smart pointers everywhere, that is). Spotting these cases is much easier if the pointers are NULL (or set to an easily recognizable pattern - 0xDEADBEEF comes to mind)

Nothing beats a clean compile this is very hard to evaluate for individual programmers though. Compilers are picky about different things. Someone with VC cannot see wether it produces warnings in GCC, vice versa.

Hmm, true. I didn't realize this because I use GCC in all platforms.

and impossible with the pedantic flag(since we use a lot of dependancies

Well, I wasn't suggesting compile with -fpedantic... that's a little too... pedantic

[temas]

I'm back from my beach vacation time, so I'll get this stuff rolled into my working copy. I wont' update the PDF again for a while, so make sure you guys are checking the HTML.

As to the encoding issue, I thought I told the server to send the correct encoding-type finally, but I'll poke it again.

[Kentamanos]

I had that too. The reason is because your browser is displaying it under a character coding scheme other than UTF-8. I checked the source, which indentifies the document as UTF-8, so it seems to be browser misbehavior, not a problem with the document.

Was on vacation, sorry for the slow response.

I had this problem on IE (6.x) and Firefox (0.9x). When you view source, you see the actual symbols in the source as well, so it's in the actual HTML stream and not something weird like the browser rendering a &# value incorrectly or something like that.

EDIT: just read temas' last response. Looks like you're looking into it.

[temas]

Ok I think I've got it all added, and the hatted A's are gone. Let me know what else you guys think.

[mac]

Most of the points seem quite reasonable to me.

Some (minor) additions:
- Formating general: aim for (human) readability
- Use one whitespace after , and ; e.g. Vector3(1, 2, 3)
- Use one whitespace before and after operator e.g. if ((a % 2) == 0) ...
- Use brackets to improve readability, nobody knows the operator precedences by heart...
- Be paranoid: Use asserts where applicable e.g. if a condition never should happen, prove it! Check return values... (don't over-use asserts in some inner loops...)
- When you have decided everything: include a longer example of a full class, how it should be
- And what about include protection and documentation guidelines? ...

PS: "Each class should be contained in its won file" should be: own

[scriptkid]

About the namespaces: for large libraries it's a good thing to avoid namespace polution, which means that you only put "using namespace xxx" in your .cpp file, and fully prefix the used stuff in the headers.

E.g.

// in your header file:
void setName (std::string name);

// in your cpp file
using namespace std;
void setName (string name);

I got this from the great book (IMO) "Large Scale C++ Applications" by John Lakos.

And i personally think (this is not a flame) that the bracet- and space wars are a waste of time and energy. They are fully a matter of taste. Have you ever not read an article or a book because it wasn't in your favorite font?

[temas]

The bracket and spacing is just boring style stuff, but to have it documented in one place makes it easier for us to get new patches to use it. Plus it should be something we can quickly handle with indent to get everything consistent as it's reviewed. Further, the whole point of this code review is both getting things solid and consistent. Just cause we're open source doesn't mean we can be sloppy and inconsistent.

[nfz]

I agree wih you, Temas. A consistent style makes reading source code a lot easier.

[velis]

Just a couple of remarks:
Why use 0 instead of NULL? Is the code not more readable if null pointers are represented and queried by NULL instead of 0?
I also can't understand why all names must start with lowercase letter. I always use upper case for each word in an ident (eg. SuperImportantVar). What's the trick with starting the first word lowercase? OK, I admit, I didn't read the books suggested, so don't flame me if they mention this on each and every page.

[temas]

- 0 is more compliant with the C++ spec, NULL is archaic C This is something that was discussed on the core team forum before posting here, and it was unanimously accepted.
- We are all used to one style of variables, so we're imposing it on everyone that wants to contribute. Just something people have to deal with

[bad_camel]

The starting with lowercase letter is the descriptor for the variable, although up to now no complete standard was used. The 'm' means member, as in a member of the class.

[geefunk]

Hi all,

Nice guidelines.

I just have one suggestion right now.

When adding comments prefer // over /* */

/* */ is fine for license texts, or for large function explanations before each function or method. But inside of a code block is a nono.

If at some point a developer wants to comment out a large block of code to test something different, nothing is more annoying than having /* */ strewn throughout the code.

Peace

[:wumpus:]

I don't have an opinion on // or /* at all, but IMO code blocks should always be commented out with #if 0, followed by a comment on the next line why the section was commented out. Preprocessor is ideal for commenting out code because
1) the comments are nestable, unlike /* */ . No problems arise with any comments in the disabled section.
2) it's easy, no need to ruin your fingers by typing // before each line.
3) most modern editors syntax highlight #if 0 sections as comment just like the other two
4) you can (temporarily) reenable the section by just changing the '0' to '1'.

Which leaves // for single line and /* for multiline user-informative comments.

[tuan kuranes]

4) you can (temporarily) reenable the section by just changing the '0' to '1'.

It can be wise to use REAL ifdefs variables. Some cvs collaborative experience can even force you to use real defined variable, using your name, like

Code: Select all

#ifdef _TUAN_KURANES_1
#endif
#ifdef _TUAN_KURANES_2
#endif
#ifdef _TUAN_KURANES_3
#endif
...

That you activate in your own makefile or project solution.

It prevents you from commiting by accident your "experiences" or "debugging help log" to the main cvs. (and make other people find you when problem arise...)

[geefunk]

I would think that when one says 'most modern editors' they would include the most widely used editors, i.e. Microsoft editors, which do not highlight #if 0 blocks as comments. Using #if 0 is begging for disaster if you ask me. Without highlighting, delete the wrong #endif and you may introduce bugs that it will take you, or more likely, your colleagues, weeks to figure out... for no good reason.

If you need verbose comments inside of a larger function or method, the code you are commenting is probably a prime candidate for it's own function or method anyway, where you can leave the verbose comments outside.

Anyway, it's just a suggestion from personal experience. I haven't noticed much in the Ogre code that is annoying like that, but I could have choked some of the Torque developers.

Peace

[sinbad]

I agree - /* */ is preferable to #if 0 in my book. Most modern editors (;)) have a comment/uncomment hotkey anyway for saving you // when you want to quickly hack out a large block which includes /* */ already. I think nested comment blocks are mostly useful when doing quick hacks, not for 'proper' commenting in any case.

[:wumpus:]

Well, from that viewpoint there is no 'proper commenting' for code at all. Code should either be there or not there. Comments should only be there to be helpful to the user/developer. If you want to save code for later use, put it in a seperate or backup file. That's the only clean way.

[Chaster]

" Each class should be contained in its won file..."

should be:

" Each class should be contained in its own file..."