OGRE coding and style guidelines

Discussion area about developing or extending OGRE, adding plugins for it or building applications on it. No newbie questions please, use the Help forum for that.
User avatar
:wumpus:
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 3067
Joined: Tue Feb 10, 2004 12:53 pm
Location: The Netherlands
x 1

OGRE coding and style guidelines

Post by :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.
Last edited by :wumpus: on Wed Sep 08, 2004 8:42 pm, edited 2 times in total.
User avatar
Noiprox
Halfling
Posts: 64
Joined: Mon Apr 05, 2004 6:25 am
Location: North Vancouver, Canada

Post by 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."
Last edited by Noiprox on Sat Sep 04, 2004 9:36 am, edited 1 time in total.
Dieter Buys
Image
"Death, be not proud, though some have called thee mighty and dreadful, for thou art not so." - John Donne
User avatar
Kentamanos
Minaton
Posts: 980
Joined: Sat Aug 07, 2004 12:08 am
Location: Dallas, TX

Post by 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.Â
User avatar
ggambett
Kobold
Posts: 32
Joined: Tue May 25, 2004 3:03 pm
Location: Montevideo, Uruguay

Post by 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.
Gabriel Gambetta
Mystery Studio - Home of Betty's Beer Bar and Wild West Wendy
User avatar
Noiprox
Halfling
Posts: 64
Joined: Mon Apr 05, 2004 6:25 am
Location: North Vancouver, Canada

Post by Noiprox »

Kentamanos wrote: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.
Dieter Buys
Image
"Death, be not proud, though some have called thee mighty and dreadful, for thou art not so." - John Donne
User avatar
:wumpus:
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 3067
Joined: Tue Feb 10, 2004 12:53 pm
Location: The Netherlands
x 1

Post by :wumpus: »

Noiprox wrote: 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.
User avatar
bad_camel
Halfling
Posts: 74
Joined: Tue Dec 17, 2002 11:57 am
Location: Somerset, England
Contact:

Post by 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 :D
User avatar
ggambett
Kobold
Posts: 32
Joined: Tue May 25, 2004 3:03 pm
Location: Montevideo, Uruguay

Post by ggambett »

:wumpus: wrote:
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)
:wumpus: wrote: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.
bad_camel wrote: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 :)
Gabriel Gambetta
Mystery Studio - Home of Betty's Beer Bar and Wild West Wendy
User avatar
temas
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 390
Joined: Sun Oct 06, 2002 11:19 pm
Location: The Woodlands, TX
Contact:

Post by 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.
User avatar
Kentamanos
Minaton
Posts: 980
Joined: Sat Aug 07, 2004 12:08 am
Location: Dallas, TX

Post by Kentamanos »

Noiprox wrote: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.
User avatar
temas
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 390
Joined: Sun Oct 06, 2002 11:19 pm
Location: The Woodlands, TX
Contact:

Post by 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
Kobold
Posts: 34
Joined: Wed Jun 25, 2003 5:17 pm
Location: Berlin, Germany

Post by 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
User avatar
scriptkid
Bronze Sponsor
Bronze Sponsor
Posts: 88
Joined: Fri Jun 25, 2004 8:16 pm
Location: The Hague, Netherlands
Contact:

Post by 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?
User avatar
temas
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 390
Joined: Sun Oct 06, 2002 11:19 pm
Location: The Woodlands, TX
Contact:

Post by 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
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 1263
Joined: Wed Sep 24, 2003 4:00 pm
Location: Halifax, Nova Scotia, Canada

Post by nfz »

I agree wih you, Temas. A consistent style makes reading source code a lot easier.
velis
Gnoblar
Posts: 1
Joined: Wed Oct 01, 2003 8:30 pm
Location: Radovljica, Slovenia

Post by 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. :roll:
User avatar
temas
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 390
Joined: Sun Oct 06, 2002 11:19 pm
Location: The Woodlands, TX
Contact:

Post by 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
User avatar
bad_camel
Halfling
Posts: 74
Joined: Tue Dec 17, 2002 11:57 am
Location: Somerset, England
Contact:

Post by 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
Kobold
Posts: 38
Joined: Wed Jun 16, 2004 5:21 pm

Comments

Post by 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
User avatar
:wumpus:
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 3067
Joined: Tue Feb 10, 2004 12:53 pm
Location: The Netherlands
x 1

Post by :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.
User avatar
tuan kuranes
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 2653
Joined: Wed Sep 24, 2003 8:07 am
Location: Haute Garonne, France
x 4
Contact:

Post by 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
Kobold
Posts: 38
Joined: Wed Jun 16, 2004 5:21 pm

Stuff

Post by 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
User avatar
sinbad
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 19269
Joined: Sun Oct 06, 2002 11:19 pm
Location: Guernsey, Channel Islands
x 66
Contact:

Post by 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.
User avatar
:wumpus:
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 3067
Joined: Tue Feb 10, 2004 12:53 pm
Location: The Netherlands
x 1

Post by :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
OGRE Expert User
OGRE Expert User
Posts: 557
Joined: Wed May 05, 2004 3:19 pm
Location: Portland, OR, USA
Contact:

Found a typo

Post by Chaster »

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

should be:

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