The time has come for all good men to come to the aid of their favorite rendering engine. The OGRE Project needs you to help with an ongoing code review. This document will outline how everyone will be able to participate and what guidelines need to be followed. It will also outline what will need to be looked for during the review process.
The code review will center around this website and use a fairly simple voting process to decide when a file has passed review. Here is a quickstart guide to understanding the process:
- Create a normal user account on the website.
- Pick a file that needs more reviewers and flag that you are beginning to review it.
- With the other reviewers leave comments and/or patches to make the file compliant with the process.
- When you are done reviewing a file flag your review complete.
- When an administrator has decided you have been helpful in the review process they will promote you to a voter.
- Once a voter you can still continue the normal process of commenting and submitting patches, but you can now vote on the completion of a file.
- When a file has received a set number of votes it will be flagged for an administrator to finalize.
- When an administrator finalizes a file it is considered to be fully satisfying the reveiw rules, and may be reopened when major changes are made.
The following areas are where the focus of the review should be directed. Many will have very specific criteria to be considered complete, but others will be more open to a general concensus of the reviewers. The current list of review points is fairly short and will be updated as the review develops. Many of the review points come from the Effective C++, More Effective C++ and Effective STL by Scotty Meyers. If you don't already have these books, you really want to get them. Other points come from decisions by the core team and practical experience. If you feel there should be additions or changes, please contact <temas@ogre3d.org>.
These rules deal with the general look of the files, including issues such as indenting, bracket placement, variable naming, etc.
-
All indentation is done using 4 spaces, no tabs.
-
Braces are placed under any line that needs a new logical block. This includes if, else, for, while, class defnitions, function definitions, and most others.
Example?1.?Normal Brace Usage
while (expr)
{
// This is the logical block. It spans multiple lines,
// and is surrounded by braces.
}
-
The primary namespace declaration for a file should have the brace placed on the same line as the declaration.
Example?2.?Namespace Brace Usage
namespace Ogre {
-
Logical blocks that are only one line do not need braces, but should be on a new line and indented. If a previous and connected block statement (else to an if) has braces then the connected block must have braces, even if it is one line long.
Example?3.?One Line Logical Blocks
if (expr)
// One line block
else
// else may be handled the same.
Example?4.?Connected Blocks
if (expr)
{
// This part is multi line and with braces.
// So we must have braces on any connected parts now.
}
else
{
// Even though this is on line, it has braces.
}
-
Control and looping statements that are followed by a "(" should have a space before the open-parenthesis.
Example?5.?Parenthesis Space
if (expr)
// Space after the if
while (expr)
// Space after the while
for (expr)
// Space after the for
-
Control and looping expressions inside parentheses should not have a space before or after the expression.
Example?6.?Expression Spaces
if (expr) // This acceptable, no space before or after expr
if ( expr ) // UNACCEPTABLE, spaces should not be before or after expr
-
The "else if" control block should always be on a single line.
Example?7.?Else If Control Block
// Acceptable, else if on one line
else if
// UNACCEPTABLE else if on multiple lines
else
if
-
Switch statements abide by all the previous control block rules, and must have their case statements on the same indentation level as the actual switch statement. Statements inside a case should be indented one level.
Example?8.?Switch Statements
// Acceptable
switch (expr)
{
case CASE1:
// The case statement is on the same indentation level
// with the switch, and it's statements are indented
// one level.
break;
};
// UNACCEPTABLE
switch( expr )
{
case CASE1:
// The case statement should be on the same
// indentation level of the switch statement
break;
};
-
All names should be written in English.
-
Method and function names should be verbs that clearly state what they will do. The name should use a mixed case format, starting with a lower case letter. Methods not intended for general API usage, rather for internal engine usage, it should be prefixed with an underscore.
-
Variables should clearly state what information they contain and use a mixed case format, starting with a lower case letter. Private and protected member variables should be prefixed with a lower case "m" and be followed by an upper case letter. The names may use limited abbreviation, but should be quickly understandable. An exception to this is looping variables (such as in afor loop), this may be a simple single letter variable such as "i".
Example?9.?Variable Naming
loggingLevel // Most clear
logLvl // Acceptable, still quickly clear
ll // unacceptable
for (int i = 0; i < max; ++i) // Acceptable
-
Each class should be contained in its won file, unless the class is directly relevant (supporting) to another and is small.
-
Prefer initialization to assignment in constructors.
-
Use const when it is possible. This includes variables and method declaration.
-
Use pass-by-reference over pass-by-value when possible.
-
Prefix calls to standard library functions with "::".
-
Strongly prefer C++ style casts to C style casts.
-
For STL containers, use empty() instead of checking size() against zero.
-
When advancing iterators and variables prefer ++var to var++.
-
If a base class has virtual functions, derived classes should have the "virtual" keyword repeated for those functions.
Note
There are differing reports on whether this might break some compilations for platforms we still wish to maintain. If that is the case, we need to reevaluate this.
-
Base type names should not use any abbreviations.
Example?10.?Type Names
unsigned int // Acceptable
uint // UNACCEPTABLE
-
C++ style headers should be preferred over C style.
Example?11.?C++ Style Headers
#inclue <cstring> // Acceptable
#include <string.h> // UNACCEPTABLE
-
Prefer standard C++ library and STL functions over C stdlib functions.
Example?12.?Functions
// Acceptable
ifstream fil;
fil.open("filename.txt");
fil.close();
// UNACCEPTABLE
FILE* fil = fopen("filename.txt", "w");
fclose(fil);
-
All class variables should be initialized to a sane value in the constructor.
-
It is not necessary to check that a pointer value is not NULL before deleting it in a destructor, or elsewhere. C++ guarantees that delete on a NULL value is safe.
Example?13.?Deleting Pointers
// Acceptable, delete NULL is safe
Val* ptrValue = 0;
delete ptrValue;
// UNACCEPTABLE
if (ptrValue != NULL)
delete ptrValue;
-
0 should be preferred over NULL.
Example?14.?Prefer 0
Val* ptrValue = 0; // Acceptable
Val* ptrValue = NULL; // UNACCEPTABLE
-
When a member value pointer is deleted, it should be set to 0.