Generalized Software Style Guide

General

What they do isn’t already done by a function that they don’t know about Any simple optimization is done

Style

Overall, things should be pleasant to look at. If things are hard to read, it's more likely there will be bugs.

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",      .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.

1. Standard C++/Python libraries
2. Project Libraries / External Includes
3. 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

No chances for infinite loops / errors

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

Pointers should almost never be used. There are very few cases where a reference is not a better idea. References are "basically" the same as pointers without the possibility of nullptr's. If you must, use smart pointers.

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):
		...

TODO things

Things should look nice overall, if you can’t figure out what code is doing at a glance, it’s bad Variables are named nicely. I want to be able to look at it and know what it represents Keep line lengths in check All magic numbers are config values or const’s at top of file Things are initialized in the constructor in the same order they are defined in the header Single responsibility principle is often lacking