Generalized Software Style Guide
Contents
Style
Overall, things should be pleasant to look at. If things are hard to read, it's more likely there will be bugs. If you can't read it at a glance, it's bad and should be redone so it's clear.
Keep line lengths in check. Don't feel forced to keep it 80 chars wide, but don't go crazy since people put code side by side a lot. 120 chars max.
Variable / Class Names
Variable and class names should be fully descriptive of what they represent. If a glance at the name can't fully describe the functionality of the code, it should be changed. It's better to have long names rather than unreadable shortened version.
Good
ballPos robotBallVec robotRadius velXCovariance
Bad
pt1 pt2 r1 r2
Aligning Code
Align code along good boundaries so it's clean to read. Some examples are below.
CameraFrame(RJ::Time tCapture,
int cameraID,
std::vector<CameraBall> cameraBalls,
std::vector<CameraRobot> cameraRobotsYellow,
std::vector<CameraRobot> cameraRobotsBlue)
: tCapture(tCapture), cameraID(cameraID),
cameraBalls(cameraBalls),
cameraRobotsYellow(cameraRobotsYellow),
cameraRobotsBlue(cameraRobotsBlue) {}
Q_k << dt3, dt2, 0, 0,
dt2, dt1, 0, 0,
0, 0, dt3, dt2,
0, 0, dt2, dt1;
ball_init_covariance = new ConfigDouble(cfg, "VisionFilter/Ball/init_covariance", 100); ball_process_noise = new ConfigDouble(cfg, "VisionFilter/Ball/process_noise", 0.1); ball_observation_noise = new ConfigDouble(cfg, "VisionFilter/Ball/observation_noise", 2.0);
Includes
Includes should be grouped into a specific order separated by white-space.
- Standard C++/Python libraries
- Project Libraries / External Includes
- Project / Module Headers
Within each group, the includes should be in ABC order.
Only include what is needed inside a header file. Everything else that may be needed should be included within a .cpp file.
#include <vector> #include <Configuration.hpp> #include <Geometry2d/Point.hpp> #include "KalmanBall.hpp" #include "vision/robot/WorldRobot.hpp"
Constructor Initialization
Initialize variables in the same order they are defined in the header.
RJ::Time lastUpdateTime;
RJ::Time lastPredictTime;
// Keeps track of this for kick detection stuff
boost::circular_buffer<CameraBall> previousMeasurements;
KalmanFilter2D filter;
int health;
unsigned int cameraID;
// Max time in seconds that a filter can not be updated before it is removed
static ConfigDouble* max_time_outside_vision;
...
KalmanBall::KalmanBall(unsigned int cameraID, RJ::Time creationTime,
CameraBall initMeasurement, const WorldBall& previousWorldBall)
: lastUpdateTime(creationTime), lastPredictTime(creationTime),
previousMeasurements(*VisionFilterConfig::slow_kick_detector_history_length),
health(*VisionFilterConfig::filter_health_init), cameraID(cameraID) {
Comments
Things do what their comments say
Module Level Documentation
Modules should each have a full doc page describing the operation of the module in terms of the overall organization as well as data flow and algorithms. A person reading this should be able to fully understand a high level overview of what the algorithm is. The "why?" behind decisions should be described. Average length is a few paragraphs targeted towards people who are familiar with the topic, but not this specific algorithm.
[Example doc page here in markdown].
File Level Documentation
Each file (.hpp and/or .py) should have a couple lines at the top describing the contents of the file as well as how it specifically fits in overall. The below example is a little overkill, but the general idea is shown.
/**
* Uses a seperate thread to filter the vision measurements into
* a smoother velocity/position estimate for both the ball and robots.
*
* Add vision frames directly into the filter call the fill states functions
* to push the newest estimates directly into the system state.
*
* Note: There may be a 1 frame delay between the measurements being added
* and the measurements being included in the filter estimate.
*/
class VisionFilter {
Function Level Documentation
Each function definition in a file (.hpp and/or .py) should contain a short summary of what the function does in simple terms, a list and description of each arguement, a description of the return, and any notes about the assumptions used or weird edge cases. An example with a note is shown below also.
/**
* Calculates whether the given kalman ball will bounce against another robot and
* the resulting velocity vector
*
* @param ball Kalman ball we are trying to test
* @param yellowRobots Best estimation of the yellow robots states
* @param blueRobots Best estimation of the yellow robots states
* @param outNewVel Output of the resulting velocity vector after bounce
*
* @return Whether the ball bounces or not
*/
static bool CalcBallBounce(const KalmanBall& ball,
const std::vector<WorldRobot>& yellowRobots,
const std::vector<WorldRobot>& blueRobots,
Geometry2d::Point& outNewVel);
/** * Predicts one time step forward * * @param currentTime Time at the current frame * * @note Call either this OR predictAndUpdate once a frame */ void predict(RJ::Time currentTime);
Code Block Level Documentation
Code blocks should be commented to describe their collective use. This isn't school so you don't have to comment every line, but blocks of lines should be commented if it's more than a very simple or standard operation. Someone should be able to read through the code and understand what the code does without reading other function definitions or reverse engineering the existing code. Any "funky" code should be commented to describe why it's needed. A few examples are seen below.
// Figures out if there is an intersection and what the resulting velocity should be
auto findEndVel = [&ball, &outNewVel] (const std::vector<WorldRobot>& robots) {
// Make sure ball is intersecting next frame
if (!BallInRobot(ball, robot)) {
continue;
}
// If the ball really isn't moving, just assume no intersection
// since the math will go to inf
if (abs(dr) < 1e-5) {
return out;
}
For complicated math, a link to a paper or wikipedia article should exist if it is based off of one. Otherwise, the math should be fully explained through words (or even better, ASCII art)
// _____ // / \ // | Robot | // \_____/ // B // /|\ // / | \ // / | \ // / | \ // / D \ // A C // Ball moves from A->B // Bounces off the robot // Moves from B->C // Line B<->D is the line of reflection // // // Line B<->D goes from the center of the robot through the point of intersection between the robot and the ball // Since the robot is round (with a flat mouth), any time it hits the robot, we can assume that the it will reflect as if // it hit a flat surface
Configuration Variable Documentation
Each config value should have an associated comment where it describes what the config value does in words, the range of the config value, and what each edge of the range represents in IRL effects.
// Linear velocity dampen // Body is bouncing off the circular shell // Mouth is bouncing off the front mouth // 1 means 100% of the velocity is kept after collision // 0 means 0% of the velocity is kept after collision static ConfigDouble* robot_body_lin_dampen; static ConfigDouble* robot_mouth_lin_dampen;
Structure
All new code doesn't have to be accepted
New features don't have to be added. This is significantly more subjective, but if the new feature does not fit with the rest of the file, it should be weighed carefully. Each file should only be responsible for a single action. Don't add stuff if it doesn't fit with the rest of the file's actions.
Private / Public
Unless something needs to be public, it should be private. In general, variables should be private unless there is a very good reason not to. If you have to make stuff public, double check that the overall architecture is not bad (It probably is).
General class format should be...
class Foo {
public:
Foo();
private:
Func();
}
Pointers / References
Raw C pointer style should be avoided. Things such as malloc and delete should not be used if possible, there is almost always a better way. Most of the time, a smart pointer will work better in these cases as the ownership is explicit.
In functions, care should be taken to decide whether a raw pointer is correct compared to a reference. A reference allows the nullptr case to be handled automatically where as you should specifically handle the nullptr case.
Large data structures or objects should be passed around by reference to save cpu cycles spent copying the data.
Const
Any data being passed which isn't being changed should be passed as const &.
void Foo(const A& var)
Any function where you return something without editing data in the object the function belongs to, decorate the function with const.
A Foo() const;
Unit Tests
Any testable functionality should have a corresponding unit test. "Testable functionality" is somewhat vague, but it should be anything where the input output relationship of a function is fully defined. This should be done for any function where a significant amount of math is done.
Test's should...
- Cover basic cases of no inputs or minimal inputs (if applicable)
- Cover every single code path
- Contain any corner cases that could be thought of
- Any fixed bug should have a corresponding test that exhibited the broken behavior
Depending on a few factors, the test file should have a function for every test case or a single function for every function being tested.
Function for every test case example
TEST(KickEvaluator, no_robots) {
...
}
TEST(KickEvaluator, eval_pt_to_our_goal) {
...
}
TEST(KickEvaluator, eval_calculation) {
...
}
TEST(KickEvaluator, eval_kick_at_pi_transition) {
...
}
TEST(KickEvaluator, check_best_point) {
...
}
Function for every function being tested (Opponent.py is a util type file with a collection of functions including "num_on_offense" and "get_closest_opponent")
class TestOpponent(unittest.TestCase): def test_num_on_offense(self): ... def test_get_closest_opponent(self): ...
Magic Numbers
No magic numbers. Everything should be a variable. Depending on the scope of the magic number and how likely it is to be changed, it can either be a const variable in the function, class, or the general config (what this entails depends on the team). A short description of what the config value represents should be included as well.
Loops
While loops aren't inherently dangerous, but if you mess them up and miss an edge case, you can easily stall the program. Extra care should be taken to make sure this never happens.
Don't re-implement functions
In general, if code feels way too complicated for the action that it does, someone most likely has already implemented this in some function somewhere. Feel free to ask older member if it already exists.
Include simple optimizations
Don't go crazy, but switching out push_back with emplace_back when pushing to a std::vector is a smart idea. Simple things like that.
