beautiful C++ code
Davide Faconti
This is what I think about my code:
"a unique and beautiful snowflake"
This is what other people thinks about my code...
WHY?
"Programmers write code that a machine can understand.
Good programmers write code that people can understand."
- Other team member can/should be able to understand code and contribute to it
- That person might be you in 4 months...
- "Software engineering is programming integrated over time"
What does "Beautiful code" mean?
- Code that is easy to understand (you don't count).
- Code that is easy to extend and modify.
- Code that requires less mental effort to be understood.
- And it just looks beautiful...
the "Marie Kondo rule"
Write code that make you (and the reviewer) feel good.
Rule #1
"Copy and paste" is always a bad smell
RULE #2
Try to remove the cognitive overhead of the code reviewer
Rule #3
Use reliable libraries,
do not reinvent the wheel
Eigen, Boost, Abseil, STL...
HOW to Reduce the cogNitive overhead
- Humans are NOT good at holding many states in their head.
- Write code that allow the reviewer to think as little as possible.
- Minimize the number of "Why?" and "WTF?"
- They don't know what you know; put yourself in their shoes.
- A good API is the best documentation.
"Don't you see? That is clearly a reverse sort function!"
But let's have a look to some real examples
for (int j = 0; j < _laserCloudHeight; j++) {
for (int k = 0; k < _laserCloudDepth; k++) {
int i = _laserCloudWidth - 1;
pcl::PointCloud<pcl::PointXYZI>::Ptr laserCloudCubeCornerPointer =
_laserCloudCornerArray[i + _laserCloudWidth * j + _laserCloudWidth * _laserCloudHeight * k];
pcl::PointCloud<pcl::PointXYZI>::Ptr laserCloudCubeSurfPointer =
_laserCloudSurfArray[i + _laserCloudWidth * j + _laserCloudWidth * _laserCloudHeight * k];
for (; i >= 1; i--) {
_laserCloudCornerArray[i + _laserCloudWidth * j + _laserCloudWidth * _laserCloudHeight * k] =
_laserCloudCornerArray[i - 1 + _laserCloudWidth*j + _laserCloudWidth * _laserCloudHeight * k];
_laserCloudSurfArray[i + _laserCloudWidth * j + _laserCloudWidth * _laserCloudHeight * k] =
_laserCloudSurfArray[i - 1 + _laserCloudWidth * j + _laserCloudWidth * _laserCloudHeight * k];
}
_laserCloudCornerArray[i + _laserCloudWidth * j + _laserCloudWidth * _laserCloudHeight * k] =
laserCloudCubeCornerPointer;
_laserCloudSurfArray[i + _laserCloudWidth * j + _laserCloudWidth * _laserCloudHeight * k] =
laserCloudCubeSurfPointer;
laserCloudCubeCornerPointer->clear();
laserCloudCubeSurfPointer->clear();
}
}loam_velodyne package
Do you know what this code does?
for (int j = 0; j < _laserCloudHeight; j++) {
for (int k = 0; k < _laserCloudDepth; k++) {
for (int i = _laserCloudWidth - 1; i >= 1; i--) {
size_t indexA = toIndex(i, j, k);
size_t indexB = toIndex(i-1, j, k);
std::swap( _laserCloudCornerArray[indexA], _laserCloudCornerArray[indexB] );
std::swap( _laserCloudSurfArray[indexA], _laserCloudSurfArray[indexB]);
}
}
}Believe it or not, this is exactly the same code, refactored
Even if this was not the goal, this code is faster than the previous one
pointSqDis = (laserCloudCornerLast->points[j].x - pointSel.x) *
(laserCloudCornerLast->points[j].x - pointSel.x) +
(laserCloudCornerLast->points[j].y - pointSel.y) *
(laserCloudCornerLast->points[j].y - pointSel.y) +
(laserCloudCornerLast->points[j].z - pointSel.z) *
(laserCloudCornerLast->points[j].z - pointSel.z);Before
pointSqDis = SquareDistance(laserCloudCornerLast->points[j], pointSel);After (can be slightly faster)
x1 = cos(transformTobeMapped[2]) * transformIncre[3] - sin(transformTobeMapped[2]) * transformIncre[4];
y1 = sin(transformTobeMapped[2]) * transformIncre[3] + cos(transformTobeMapped[2]) * transformIncre[4];
z1 = transformIncre[5];
x2 = x1;
y2 = cos(transformTobeMapped[0]) * y1 - sin(transformTobeMapped[0]) * z1;
z2 = sin(transformTobeMapped[0]) * y1 + cos(transformTobeMapped[0]) * z1;
transformTobeMapped[3] = transformAftMapped[3]
- (cos(transformTobeMapped[1]) * x2 + sin(transformTobeMapped[1]) * z2);
transformTobeMapped[4] = transformAftMapped[4] - y2;
transformTobeMapped[5] = transformAftMapped[5]
- (-sin(transformTobeMapped[1]) * x2 + cos(transformTobeMapped[1]) * z2);Before
Vector3D v1 = rotateZ( transformIncre.pos, transformTobeMapped.rot_z);
Vector3D v2 = rotateX( v1, transformTobeMapped.rot_x);
Vector3D v3 = rotateY( v2, transformTobeMapped.rot_y);
transformTobeMapped.pos = transformAftMapped.pos - v3;After (will probably be faster)
Remember the previous examples?
if (roboteq_can_hw::ControlMode::CLOSE_LOOP_VELOCITY ==
dual.m_param_dual.ch1_mode) {
hardware_interface::JointHandle velocity_handle_ch1(
joint_state_interface_.getHandle(dual.m_param_dual.ch1_location_id),
&setpoint_[dual.m_param_dual.ch1_joint_index]);
joint_velocity_interface_.registerHandle(velocity_handle_ch1);
} else if (roboteq_can_hw::ControlMode::CLOSE_LOOP_POSITION_COUNT ==
dual.m_param_dual.ch1_mode) {
hardware_interface::JointHandle position_handle_ch1(
joint_state_interface_.getHandle(dual.m_param_dual.ch2_location_id),
&setpoint_[dual.m_param_dual.ch1_joint_index]);
joint_position_interface_.registerHandle(position_handle_ch1);
}
if (roboteq_can_hw::ControlMode::CLOSE_LOOP_VELOCITY ==
dual.m_param_dual.ch2_mode) {
hardware_interface::JointHandle velocity_handle_ch2(
joint_state_interface_.getHandle(dual.m_param_dual.ch2_location_id),
&setpoint_[dual.m_param_dual.ch2_joint_index]);
joint_velocity_interface_.registerHandle(velocity_handle_ch2);
} else if (roboteq_can_hw::ControlMode::CLOSE_LOOP_POSITION_COUNT ==
dual.m_param_dual.ch2_mode) {
hardware_interface::JointHandle position_handle_ch2(
joint_state_interface_.getHandle(dual.m_param_dual.ch2_location_id),
&setpoint_[dual.m_param_dual.ch2_joint_index]);
joint_position_interface_.registerHandle(position_handle_ch2);
}
for( int i=0; i<2; i++)
{
auto& ch_location_id = ( i==0 ? dual.m_param_dual.ch1_location_id,
dual.m_param_dual.ch2_location_id);
int index = ( i==0 ? dual.m_param_dual.ch1_joint_index;
dual.m_param_dual.ch2_joint_index );
int mode = ( i==0 ? dual.m_param_dual.ch1_mode;
dual.m_param_dual.ch2_mode );
hardware_interface::JointHandle handle(
joint_state_interface_.getHandle(ch_location_id),
&setpoint[index]);
if(roboteq_can_hw::ControlMode::CLOSE_LOOP_VELOCITY == mode) {
joint_velocity_interface_.registerHandle(handle);
}
else if(roboteq_can_hw::ControlMode::CLOSE_LOOP_POSITION_COUNT==mode) {
joint_position_interface_.registerHandle(handle);
}
}You don't need to follow the "rule of three".
Sometimes it should be just 2 or even 1.
Rename things locally (when scope is SMALL) to make names shorter.
for(int i=0; i < trajectory.waypoints.size(); i++)
{
trajectory.waypoints[i].pose.point = transform( trajectory.waypoints[i].pose.point );
if( ! isInsideBoundary( trajectory.waypoints[i].pose.point) )
{
trajectory.waypoints[i].pose.point.setZero();
}
}// prefer instead
for(int i=0; i < trajectory.waypoints.size(); i++)
{
Point3D& point = trajectory.waypoints[i].pose.point
point = transform( point );
if( ! isInsideBoundary( point )
{
point.setZero();
}
}Use reference or const reference instead
MORE TIPS...
Keep variables in the smallest scope POSSIBLE
// Inefficient implementation:
for (int i = 0; i < 1000000; ++i) {
Foo f; // My ctor and dtor get called 1000000 times each.
int temp = f.DoSomething(i);
} int i, temp;
for ( i = 0; i < 1000000; ++i) {
temp = f.DoSomething(i);
}
// prefer instead
for (int i = 0; i < 1000000; ++i) {
int temp = f.DoSomething(i);
}...Unless construction/destruction is expensive
Avoid nested if-else
Nested if-else increase the amount of states to remember.
Simplify your logic or use
break, continue and return
when appropriate.
Avoid nested if-else
int findShape(unsigned flags, Point3D point, const std::list& list)
{
if(!findShapePoints(flags, point)){
if(!findFromGuide(flags, point)) {
if(!findInShape(flags, point)) {
if(list.count() > 0 && flags == 1)
doSomething();
}
}
}
}
int findShape(unsigned flags, Point3D point, const std::list& list)
{
if(findShapePoints(flags, point)) {
return;
}
if(findFromGuide(flags,point) {
return;
}
if(findInShape(flags, point)) {
return;
}
if (!(list.count() > 0 && flags == 1)) {
return;
}
doSomething();
}Avoid macros
They are ugly and dangerous.
#define make_char_lowercase(c) \
((c) = (((c) >= 'A') && ((c) <= 'Z')) ? ((c) - 'A' + 'a') : (c))
void make_string_lowercase(char* s)
{
while (make_char_lowercase(*s++)) {}
}// it is actually...
while (((*s++) = (((*s++) >= 'A') && ((*s++) <= 'Z')) ? ((*s++) - 'A' + 'a') : (*s++))) {}Use inline functions
Equally fast, more readable,
type safe and surprise-free
inline char make_char_lowercase(char& c)
{
if (c > 'A' && c < 'Z'){
c = c - 'A' + 'a';
}
return c;
}
void make_string_lowercase(char* s)
{
while (make_char_lowercase(*s++)) {}
}Don't #define parameters
#define RED 0
#define ORANGE 1
#define YELLOW 2
#define GREEN 3
#define BLUE 4
#define PURPLE 5
#define HOT_PINK 6
#define SUCCESS 1
#define FAIL 0
void setColor(int color);
int doOperation(); // it is not self documenting
void f()
{
setColor(HOT_PINK); // Ok
setColor(9000); // Undefined behaviour, but compiler can’t tell
if( doOperation == SUCCESS ) {
//
}
}Use enums
enum ColorType
{
RED = 0,
ORANGE = 1,
YELLOW = 2,
GREEN = 3,
BLUE = 4,
PURPLE = 5,
HOT_PINK = 6
};
enum ResultType{ FAIL = 0, SUCCESS = 1 };
ResultType doOperation();
void setColor(ColorType color);
void f()
{
setColor(HOT_PINK); // Ok
setColor(9000); // will not compile
if( doOperation == SUCCESS ) {
//
}
}Even better enums in C++11
enum ColorType
{
red, orange, yellow, green, blue, purple, hot_pink
};
enum TrafficLightState
{
red, yellow, green
};
// error C2365: 'red' : redefinition; previous definition was 'enumerator‘
// error C2365: 'yellow' : redefinition; previous definition was 'enumerator‘
// error C2365: 'green' : redefinition; previous definition was 'enumerator' enum class ColorType
{
red, orange, yellow, green, blue, purple, hot_pink
};
enum class TrafficLightState
{
red, yellow, green
};Use named enums instead
ENUMS are better than booleans
bool SendQueryToServer(const std::string& server_address,
const std::vector<uint8_t>& msg,
bool type);
//------------ VS ----------------------
enum class RequestType
{
SYNC, ASYNC
};
enum RequestResult
{
RECEIVED, TIMEOUT
};
RequestResult SendQueryToServer(const std::string& server_address,
const std::vector<uint8_t>& msg,
RequestType type); Use "invisible code"
Constructor and destructors
C++ Operators overload
struct Vector3D
{
double x,y,z;
}
// Simple implementation, verbose
Vector3D a,b,c;
a.x = 0;
a.y = 0;
a.z = 0;
b.x = 1;
b.y = 2;
b.z = 3;
c.x = a.x + b.x;
c.y = a.y + b.y;
c.z = a.z + b.z;
Smart pointers are a wonderful example of invisible code
A better Vector3D
class Vector3D
{
public:
double x,y,z;
Vector3D(): x(0), y(0), z(0) {}
Vector3D(double x_, double y_, double z_):
x(x_), y(y_), z(z_) {}
Vector3D(const Vector3D& other) = default;
Vector3D& operator =(const Vector3D& other) = default;
Vector3D operator+( const Vector3D& v){
return Vector3D( x + v.x, y + v.y, z + v.z );
}
}
// your code
Vector3D a; // all elements initialized to 0
Vector3D b( 1, 2, 3 );
Vector3D c( a+b );Comments (?)
Sometimes comments can improve the readability, but in general you should try to write code that is self explaining
int _read_nolock(int fh, void *inputbuf, unsigned cnt )
{
int bytes_read; /* number of bytes read */
char *buffer; /* buffer to read to */
int os_read; /* bytes read on OS call */
char *p, *q; /* pointers into buffer */
wchar_t *pu, *qu; /* wchar_t pointers into buffer for UTF16 */
char peekchr; /* peek-ahead character */
wchar_t wpeekchr; /* peek-ahead wchar_t */
__int64 filepos; /* file position after seek */
ULONG dosretval; /* o.s. return value */
char tmode; /* textmode - ANSI/UTF-8/UTF-16 */
BOOL fromConsole = 0; /* true when reading from console */
void *buf; /* buffer to read to */
int retval = -2; /* return value */what the hell is *p and *q?
what is the difference between *buffer and *buf?
Avoid useless comments
int main()
{
std::vector<int> v;
// Get the data from the input file:
v = get_data_from_input_file();
// Find the largest value in the vector:
int largest_value = INT_MIN;
for (auto const i : v)
{
if (i > largest_value)
largest_value = i;
}
// Print the largest value that we found:
std::cout << "largest value: " << largest_value << '\n';
}In general, try to avoid explaining what code does;
explain instead why interesting code is the way it is
Don't be afraid to use
long names...
Pose3D pose;
bool res = plan.compute(pose);
// vs
Pose3D target_pose;
bool planning_is_feasible = planner.compute( target_pose );Use common sense, but be sure that someone else but you can understand it.
Use CONST and write self documenting APIs
class Foo
{
public:
Result bar(const Parameters& param, const Vector& in, Vector *out) const;
private:
// ...
}- Inputs are constant, output are not.
- Use enum as shown earlier
- Const members are telling you that bar won't modify *this
LAST SUGGESTION...
Try to write readable code by default, but don't get obsessed with that.
Writing clean code is an iterative process. Be a gardener.
References
Making C++ Code Beautiful
Emotional Code