Difference between revisions of "Generalized Software Style Guide"

From RoboJackets Wiki
Jump to navigation Jump to search
Line 115: Line 115:
  
 
No chances for infinite loops / errors
 
No chances for infinite loops / errors
 +
 +
=== 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.
 +
 +
<nowiki>#include <vector>
 +
 +
#include <Configuration.hpp>
 +
#include <Geometry2d/Point.hpp>
 +
 +
#include "KalmanBall.hpp"
 +
#include "vision/robot/WorldRobot.hpp"</nowiki>
  
 
== Unit Tests ==
 
== Unit Tests ==
Line 161: Line 178:
 
Things should look nice overall, if you can’t figure out what code is doing at a glance, it’s bad
 
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
 
Variables are named nicely. I want to be able to look at it and know what it represents
Includes are in a specific order. Group by stdlib, system/library, local headers, then ABC order inside each group
 
 
Keep line lengths in check
 
Keep line lengths in check
 
All magic numbers are config values or const’s at top of file
 
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
 
Things are initialized in the constructor in the same order they are defined in the header
 
Single responsibility principle is often lacking
 
Single responsibility principle is often lacking
Comments
 
Args / rets should be described for functions with notes about assumptions made
 
Classes should describe their place in the program overall
 
Doc pages should be made for entire systems describing the way all the data / classes work together
 
Blocks of code should be commented to describe things on a low level
 
Math should be fully described (ASCII art for vectors)
 
https://github.com/RoboJackets/robocup-software/blob/master/soccer/vision/ball/BallBounce.cpp#L65
 

Revision as of 01:44, 28 February 2019

General

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

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

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.

#include <vector>

#include <Configuration.hpp>
#include <Geometry2d/Point.hpp>

#include "KalmanBall.hpp"
#include "vision/robot/WorldRobot.hpp"

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