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

"Beautiful code"... much more than formatting

By Davide Faconti

"Beautiful code"... much more than formatting

  • 647