-
Notifications
You must be signed in to change notification settings - Fork 29
Coding Standard
Every developer has his own preferred style of coding. The objective of this document is not to contradict with your style of coding, but to define a code-style for all code within the SimMobility project. It is important for all programmers in the team to follow a uniform style of coding for the source-code of the project to look uniform, clean, readable and professional.
The C++ Style Guide explains how to structure your code in SimMobility. C++ is a very expressive language, and there are some things which should be done one way and not another. This document tries to cover all of these cases. If there is still some confusion, please add your questions to the Discussion page.
Some of the points here were inspired by the [Google C++ Style Guide] (https://google.github.io/styleguide/cppguide.html).
Do not go overboard and try to fix everything right away. For example, if you are editing someone's code, and that person does not use const-correctness, then do not re-write every function call to be const-correct. If someone's code uses C-style elements like char* instead of std::string, there is no need to "fix" everything just because you are making a few minor changes.
That said, there is plenty of existing code that was written before these guidelines came into place which should be re-written. If you intend to do a stylistic re-write, please email the list first and explain what you intend to do.
If you need help, ask for it. In other words, if you don't understand something in the Coding Standards, or if, for example, you don't know how to program something without using macros, then send an email to the coders list and ask for help. (Try it first yourself, of course, but if you're really stuck we're happy to help you.)
Header and source files that contain C++ code shall use the extensions .hpp and .cpp. Only use the extensions .h and .c if the header or source file in question contains only C code and it is intended for C-style compilation.
NOTE: Currently, some temporary files use the .h extension.
When naming a class, struct, union or enum, the following guidelines apply:
- Name the class after what it is or what it accomplishes.
- A class should stand on its own. It doesn't matter what it derives from.
- Use UpperCamelCasing. The first letter of each word in a class's name (including the first word) should be uppercase. Remaining letters are lowercase, except where uppercase makes sense (e.g., acronyms).
- An underscore, "_", should be used as a separator between two words if the last letter of the first word is uppercase. Underscores should not otherwise be used in class names.
Right | Wrong |
---|---|
class AutonomousVehicle {}; | class Autonomousvehicle {}; |
class TypeO_Positive {}; | class TYPE_O_POSITIVE {}; |
class MainGPS_Receiver {}; | class MainGPSReceiver {}; |
struct PhD_Student {}; | struct PhdStudent {}; |
Constant names should be spelled using all uppercase letters with underscores, "_" as separators. Enum elements are also considered as constants. All elements within the same enum should be given identical single-word prefix.
Right | Wrong |
---|---|
#define DEBUG_ON | #define DEBUGON |
const int SIMULATION_END_TIME = 10; | const int SimulationEndTime = 10; |
enum DriverTypes { DRIVER_AGGRESSIVE, DRIVER_DEFENSIVE, DRIVER_SITUATIONAL }; |
enum DriverTypes { AGGRESSIVE, DRIVE_TYPE_DEFENSIVE, DriveT_situational }; |
Method and function names follow a set of simple guidelines:
- Generally speaking, a function or method exists to perform an action, so its name should clearly indicate what it does. Since classes are usually nouns, treating functions as verbs will allow the source code to be read more naturally.
- Use lowerCamelCasing. The first word in the function name must be lowercase. The first letter of each of the following words in the function name should be uppercase. Remaining letters of the following words are lowercase, except where uppercase makes sense (e.g., acronyms).
- An underscore, "_", should be used as a separator between two words if the last letter of the first word is uppercase. Underscores should not otherwise be used in method or function names.
- Suffixes can help to bring attention to two related functions; e.g.,
- Max – highest possible value in a list
- Count – count of values in a the list
- Prefixes must be used for wrapping access to private fields. They may also be used to structure functions:
- set – Change a value
- get – Retrieve a value
- is/has/can (depending on what makes sense for the variable being tested) – Test a value
Right | Wrong |
---|---|
//Good: self-descriptive void saveDataToFile(); |
//Bad: vague, uninformative void dataFile(); |
//Good: proper use of suffixes int getRetryCount(); int getRetryMax(); |
//Bad: unclear; suffixes not used int currentRetryCount(); int maxNumberOfRetries(); |
//Good: prefixes used correctly class NetworkReader { public: int getRetryCount(); void setRetryCount(int newVal); bool isRetryLimitExceeded(); private: int retryCount; private: int retryMax; }; |
//Bad: Prefixes used inconsistently class NetworkReader { public: int getRetryCount(); void setRetryNumber(int newVal); bool isNotHitRetryCountLimit(); public: int retryCount; private: int retryMax; }; |
Names for arguments of a function or macro should be in lowerCamelCasing.
- The first character should be lower case.
- All words after the first should begin with an uppercase letter.
- Underscores should be used similarly to how they are used for class names.
Right | Wrong |
---|---|
#include "Engine.hpp" class SimulationEngine { public: int start(Engine& someEngine, const Engine* anotherEngine); }; |
#include "Engine.hpp" class SimulationEngine { public: int start(Engine& SomeEngine, const Engine* another_engine); }; |
All classes should be defined directly under the sim_mob namespace or in one of its sub-namespaces. The three most common sub-namespaces are sim_mob::short_term, sim_mob::mid_term, and sim_mob::long_term. For example, If the primary intended usage of a component is in the short term simulator, then it should be defined under the sim_mob::short_term namespace. Components which are generic and usable in across sub-namespaces can be directly defined the sim_mob namespace. The closing brace for a namespace must be commented to indicate which namespace is being scoped out. An example is below.
namespace sim_mob
{
class RoadNetwork {};
namespace short_term
{
class PedestrianModel {};
} //short_term namespace
} // sim_mob namespace
If a named entity represents a quantifiable measurement in some unit (such as time, length, or mass) then the name may specify the unit as a suffix. Coders should understand that this requirement is meant to prevent careless unit conversion errors, and they should use their discretion. Some cases that require discretion include:
- The function void GetLength(Meters& result), which writes the return value into a parameter of a specific unit type. This does not need to be named GetLengthM; the parameter type already limits the unit.
- A confusing suffix, even if it is standardized, such as DM. Does this mean decimeter or decameter? In this case, the coder should spell out, e.g., GetLengthDeciM().
Right | Wrong |
---|---|
int timeoutMs = 10; | int timeout = 10; |
float timeoutSecs = 0.01; | float timeoutQ = 0.01; |
int getLengthKm(); | int getLengthLY(); |
int getLengthMiles(); | int getLengthDM(); |
When naming constant pointer variables, you have two options:
- Traditional Syntax - Put const before the variable and after the pointer.
- Suffix Syntax - Put const after the variable and after the pointer.
For non-pointer variables, the syntax choice seems arbitrary. "Traditional" syntax seems easier to read naturally in English, since const int& x; can be read as "a constant integer reference named x". The following table compares non-pointer syntax:
Traditional | Suffix | Meaning |
---|---|---|
int radius = 10; | int radius = 10; | Non-constant variable, not a pointer. |
const float PI = 3.14; | float const PI = 3.14; | Constant variable (cannot change its value). Not a pointer |
int& radius = 10; | int& radius = 10; | Non-constant variable reference. |
const int& test = radius; | int& const test = radius; | Constant variable reference (cannot change its value). |
For pointer types, the choice of syntax becomes more important:
Traditional | Suffix | Meaning |
---|---|---|
int* len = new int(10); | int* len = new int(10); | Non-constant pointer to integer. |
const int* myLen = len; | int const * myLen = len; | Pointer to constant integer. (Cannot change integer's value.) |
int* const yourLen = len; | int *const yourLen = len; | Pointer to an integer, pointer is constant. (Cannot change what pointer is pointing to.) |
const int* const ourLen = len; | int const *const ourLen = len; | Pointer to constant integer, pointer is constant. (Cannot change integer's value, cannot change what pointer is pointing to.) |
There are pros and cons to each approach in naming. Suffix-style naming is easy to understand, since the "const" always modifies whatever it comes after. So, for:
int const *
...the "const" modifies the "int", since it comes after it. However, traditional-style naming has the benefit of spacing the consts as far apart as possible, since, these two declarations:
int const *
int * const
...look very similar to novice programmers. In addition, traditional naming reads very naturally for all examples except:
const int* const
However, this type of naming is much rarer than all other forms, since pointers which cannot be re-assigned are similar to references.
We do not require the use of one syntax over the other, but we recommend Traditional syntax to those who do not feel strongly either way. However, if you are editing existing code, then matching the style of that code ''is'' required.
Anonymous (un-named) namespaces are allowed as a means of declaring constants or functions which will only be used in the same source file where it is defined. (The alternative, is to declare them as private and static within the class. This is allowed as well). Anonymous namespaces are guaranteed to be unique to the translational unit (e.g., the file), and are thus hidden from all other files.
//file: Signal.hpp
enum Phases
{
PHASE_RED = 1,
PHASE_YELLOW = 2,
PHASE_GREEN = 3
};
class Signal
{
bool testChangePhase(int elapsedTime, Phases currPhase);
};
//file: Signal.cpp
namespace
{
const int PHASE_TIMING[ ] = {30, 10, 20}; //PHASE_TIMING is visible only in Signal.cpp
} //anonymous namespace
bool Signal::testChangePhase(int elapsedTime, Phases currPhase)
{
if (elapsedTime > PHASE_TIMING[(int)currPhase])
{
return true;
}
return false;
}
Declare local variables as close to their use as is reasonably possible. This makes it much easier to determine the purpose of each variable. If possible, initialize the variable when you declare it. In some cases (e.g., variables local to a loop) you may choose to declare a variable earlier than normal.
Right | Wrong |
---|---|
int x = 10; int y = 12; /* some code */ int radius = 30; int circumference = 2 * 3.14 * radius; |
int x, y, radius, circumference; x = 10; y = 12; /* some code */ radius = 30; circumference = 2 * 3.14 * radius; |
//Initialize a variable on the same line if possible. int x = 10; |
int x; x = 10; |
Agent ag; //It is ok to declare this "early" for (size_t i=0; i<1000; i++) { ag.setPos(i, i*2); ag.doSomething(); } |
for (size_t i=0; i<1000; i++) { Agent ag; //ag will be constructed 1000 times. ag.setPos(i, i*2); ag.doSomething(); } |
int items[20]; for (int id=0; id<20; id++) { items[id]++; } |
int items[20]; int id; for (id=0; id<20; id++) { items[id]++; } |
Avoid declaring global variables, global functions, and macros (which are global by default). Most global variables or functions can be represented as static class items (e.g., sim_mob::Driver::MAX_DRIVER_SPEED). Most macros can be represented as enums, constants, or inline functions. If you must #define a macro, try to do it within a local scope and then immediately #undef it.
See also:
- C++ FAQ - Macros are Evil #1 [http://www.parashift.com/c++-faq-lite/inline-functions.html#faq-9.5 ]
- C++ FAQ - Macros are Evil #2 [http://www.parashift.com/c++-faq-lite/misc-technical-issues.html#faq-39.4 ]
- C++ FAQ - Macros are Evil #3 [http://www.parashift.com/c++-faq-lite/misc-technical-issues.html#faq-39.5 ]
- C++ FAQ - Macros are Evil #4 [http://www.parashift.com/c++-faq-lite/misc-technical-issues.html#faq-39.6 ]
NOTE: The current code has a lot of globally-scoped data. This is slowly being shuffled into classes.
A “header guard” is a way to prevent linking the same file multiple times. All header files (.hpp) must begin with:
#pragma once
...as the first non-commented, non-empty line of text. This is preferred over defining constants, since it does not introduce arbitrary definitions into the global namespace.
There are a set of specific, limited circumstances in which #pragma once is not effective; in these cases you may use define-based header guards as long as the source includes a comment justifying your needs.
Right | Wrong |
---|---|
///file: test.hpp #pragma once class Test { }; |
///file: test.hpp #ifndef _test_hpp_ #define _test_hpp_ class Test { }; #endif //_test_hpp_ |
Use forward declarations when possible. Consider the following code segment:
class RoadSegment
{
public:
Link* parentLink;
};
In order to access the Link class, you have two options:
- Header Include: Add the line #include "Link.hpp" to the top of the file.
- Forward Declaration: Add the line class Link; to the top of the file (within the namespace).
Using forward declarations leads to faster compile times, because changing Link.hpp will not force RoadSegment.hpp to recompile. You should always use forward declarations, except in these cases:
- If you are defining a sub-class.
- If you are using a class by value (e.g., Link parentLink)
- If you are accessing a static method of a class
- If you are creating or deleting a pointer to an object using new. (This includes deleting pointers to objects which are passed in to functions).
In each of the above cases, you must use a header include, because the compiler will need specific information about an object's size or layout.
Note: There's a good example of using a forward declaration in a .hpp file and then including the header files in the cpp file. (We use this in the config file loader). Might want to put that in its own section on ''where'' to include header files.
You should always include a file by giving its fully qualified path (from the project's root folder), unless that file is in the same folder as the file you are including it from.
Right | Wrong |
---|---|
///file: entities/roles/Stagecoach.hpp #pragma once #include "entities/Agent.hpp" #include "StagecoachProperties.hpp" //Same folder //Also ok, but the previous line is preferred. //#include "entities/roles/StagecoachProperties.hpp" |
///file: entities/roles/Stagecoach.hpp #pragma once //Going backwards is always wrong. #include "../Agent.hpp" //Technically correct, but confusing. #include "./StagecoachProperties.hpp" //Going backwards is always wrong. #include "../../entities/roles/StagecoachProperties.hpp" |
You should include any needed files in the following order:
- Include the class header file if this is an implementation of this class.
- Include any system libraries with angle brackets.
- Include anything else which uses angle brackets (3rd party libraries, etc.)
- Include files which start from the top level directory.
- Include files which are in the same folder as this file.
Right |
---|
///file: entities/roles/Stagecoach.cpp #pragma once #include "Stagecoach.hpp" #include <vector> #include <cmath> #include <boost/thread.hpp> #include <boost/bind.hpp> #include "entities/Agent.hpp" #include "geospatial/Point2D.hpp" #include "StagecoachProperties.hpp" |
First, note that a function declaration includes only the function name, its parameters, and return value. This is sometimes called a ''function prototype''. On the other hand, a function definition contains the entire body of the function.
As a general rule, your header files should contain only declarations. There are a few exceptions to this rule:
- Simple "get" and "set" functions may be defined in the header file if they do not require another file to be #included as a result.
- Constructors which do not contain any logic should be defined in the header file. Initializer lists are ok, but if you are doing anything inside the braces of the constructor it should go in the cpp file.
- Un-implemented functions should be defined inside the header file (and should throw a runtime_exception).
- Templates should usually be defined inside the header file.
Also, as a general rule, you should avoid putting a lot of logic into constructors, unless you fully understand how C++ manages constructor chaining.
Every file in the code base must include a copyright info comment in the following format as the first lines in the file.
//Copyright (c) <YEAR> Singapore-MIT Alliance for Research and Technology
//Licensed under the terms of the MIT License, as described in the file:
// license.txt (http://opensource.org/licenses/MIT)
<YEAR>
must be replaced with the year in which this file was developed.
Every non-trivial user defined type (class, struct etc.) must include documentation comments in the following sample format.
/**
* Reasonably detailed explanation of what this class is for
*
* \author <Name Of Developer>
*/
class SomeClass
{
//class code
};
/**
* Saves data to the given file.
*
* @param fileName name of file to save the data.
* @return true if data was saved to the file; false otherwise.
*/
bool saveDataToFile(const string& fileName);
It is not mandatory to add documentation comments to every data member of classes. However it is recommended to add documentation comments for non-trivial members. Programmer discretion is advised. An example is shown below.
/**
* zone code --> zone id map
*/
const std::unordered_map<int, int>& zoneIdLookup;
When including C system libraries, prefer the syntax #include <cmath> over #include<math.h>. This makes it easier to determine when we are using system libraries and when we are using third-party libraries.
Every developer in the team must be familiar with [Git and our code review process] (https://github.com/smart-fm/simmobility-prod/wiki/Git-and-Code-Review). Git distinguishes between the local repository on your computer, and the shared repository on the server. Make sure you understand the difference, or the following guidelines will not make much sense.
Each commit to your local repository should attempt to accomplish one single task or less. Do not lump all your changes into a single commit. Finally, once you are sure that your changes do not break existing code, push all your commits to the repository.
If, for example, you encounter an undocumented bug while adding a new feature, do not present both the bug fix and the feature as one commit. Instead, do one of the following:
- Stash your changes. Then, fix the bug and commit the bug fix. Un-stash your earlier changes, finish adding the new feature, and commit that. Now, push both commits to the shared repository.
- Commit your work on the feature. Then, fix the bug and commit the bug fix. Finally, finish your feature and commit that. Now, push all three commits to the shared repository.
Remember, the more local commits you make, the easier it is to detect which change caused a specific regression. That change can then be rolled back independently (or backported) if the commit history is sane.
Finally, multiple small commits are easier to merge than one big commit, in the event that someone else is working on the same file as you.
todo
todo
todo
todo
todo
todo
todo
todo: if a driver has a related controller, avoid writing Hanldler controller, otherwise we risk to let the driver be controlled by any entity, for example, a bus controller.
TODO: see the comment TaxiDriver::clone (cpp) in the commit of the 1st of Semptember
TODO: the controller does not change with respect to who sends this
TODO: if (length<0) length=euclidean. No, we should raise an issue
If I put spaceship as mode, SM simulates cars