Generalized Software Style Guide

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.

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

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.