Best Practices approach for conducting Effective Code Reviews for Software Development Success!

Code Reviews

By John Arrizza

Purpose

The intent of code reviews is to ensure the source code is as easy to understand as possible and therefore finding defects requires minimal effort.

To achieve that goal, code reviews typically focus on:

1. Readability
2. Compliance to architectural conventions
3. Compliance to coding conventions
4. Compliance to unit testing conventions
5. Compliance to project conventions

Readability

Source code is read by a diverse set of engineers and developers. Having a consistent style, formatting, etc. ensures that there is minimal effort to read and understand the source code.

1. Consistent formatting – check that coding style conforms to a common standard across all source files. E.g.
   1. indent 2 spaces, do not use tabs
   2. Maximum line length
   3. Brace placement
   4. Mandatory braces “{ }” on all if/else/while/for statements
   5. Specific formatting standards for various document types:
      1. .c
      2. .cpp
      3. .h
      4. .mk
      5. .json
      6. .xml
      7. etc.
2. Documentation style – check that documentation in the source code conforms to a common standard across all source code. E.g.
   1. Every file has a block comment at the top with a set of mandatory fields and optional fields as applicable:
      1. Mandatory – copyright notice
      2. Mandatory – module or component this file belongs to
      3. Etc,
   2. Every class has a block comment with mandatory and optional fields as applicable
   3. Every method has a block comment with mandatory and optional fields as applicable
   4. All comment blocks shall be compliant with doxygen parsing and layout standards (Note: doxygen is an automated documentation generation utility)
   5. etc.
3. Tracing Conventions – check that all files/classes/methods conform to documentation conventions for tracing to other documents e.g. to the sub-system SDS, to architectural or other high-level design document(s). E.g. Block comments could have a field that indicates:
   1. an SDS-id to reference that design element
   2. A reference to a document name in a block comment
   3. An id to a Unit Trace file or unit test function
4. Naming Conventions – check the names of various entities conform to a common standard. E.g.
   1. Directories e.g. capitalization, snake case, vs camel case
   2. Source files e.g. snake case vs camel case
   3. File extensions e.g. .h, vs .hpp
   4. For C/C++: .h file names match .cpp file names
   5. Class names e.g. class name matches enclosing file name
   6. Function names e.g. snake case vs camel case, names for getters and setters and other common behaviors
   7. Variable names e.g. member names vs external names vs public/global names
   8. Etc.
5. Abbreviation conventions – check that abbreviation standards are followed and/or others are disallowed. E.g.
   1. “_ptr” – indicates a pointer
   2. “-fd” – indicates a file descriptor
   3. Etc.

Compliance to architectural conventions

Architectural conventions ensure that to achieve behaviors required of the system, various implementation techniques and standards are consistently followed.

1. Only use certain architectural features
2. Multithreading and Multiprocessing conventions
3. Globals and singleton conventions
4. Startup, shutdown and abort conventions
5. Data content and format conventions

Compliance to coding conventions

Coding conventions ensure that low level behaviors that are required of the module or component,  various implementation techniques and standards are consistently followed.

1. Compliance to external standards
2. Switches and nested ifs conventions
3. Looping conventions
4. Preconditions and guard clause conventions
5. Memory allocation and deallocation conventions
6. Linking conventions

Compliance to unit testing conventions

Unit Testing conventions ensure that various testing outcomes can be obtained by consistently following various implementation techniques and standards.

1. Branch vs line coverage
2. Mocks and stubs conventions
3. Documentation standards and conventions
4. Naming conventions

Compliance to project conventions

Each project will have various other conventions to ensure consistent behaviors and adherence to various implementation techniques and standards.

1. I18N (Internationalization) conventions – check that all literal and other strings conform to standards that ensure multi-language compatibility. E.g.
   1. Use Unicode or UTF-16 etc.
   2. use wchar_t
   3. All strings for each language are in a property file and are identified by a text id. All source code uses text ids, not literal strings
   4. etc.
2. Use of exceptions – check that exceptions are thrown and handled in a consistent way. E.g.
   1. Only use exceptions defined by the project, not language standards
   2. Do not have empty catch blocks
   3. etc.
3. Communication protocol conventions – ensure any inter-process or client-server communication is consistently implemented:
   1. Do not use buffered I/O
   2. Read all input one character at a time
   3. Only  use buffered I/O, only read entire buffers at a time
   4. Prefer fixed size buffers
   5. etc.
4. Thread and process oriented conventions – ensure that threading and process models and standards are enforced. E.g.
   1. Only use pthread or boost threads or another common threading library
   2. Each thread shall have a thread name
   3. Each thread id is stored in a global queue
   4. IPC (inter-process communication) shall be via local sockets or shared memory or message queues or another common IPC library
   5. Mutexes or semaphores or critical section shall be used in various scenarios
   6. Signals and signal handlers shall be used in various scenarios, e.g. ctrl-c handling, sigabort signals
   7. Inter process or inter thread communication shall have queues protected by mutexes.
   8. Etc.
5. RTOS (Real-time operating system) conventions – ensure that various RT conventions and standards are enforced. E.g.
   1. ISRs (Interrupt Service Requests) shall not use stdout or logging facilities
   2. All incoming events from external devices shall be saved in a mutex protected queue
   3. Certain RTOS functions are disallowed
   4. Certain RTOS functions must be used in certain scenarios
   5. All task priorities shall be N except in certain, justified, scenarios
   6. Etc.
6. OS (operating system) conventions – ensure that interactions with the OS are consistent. E.g.
   1. All OS interactions shall use system() or a project specific function
   2. The following OS functions shall not be used
   3. etc.
7. Stdin/stdout/stderr conventions – ensure the use of printf, scanf or other language specific facilities follow a consistent standard. E.g.
   1. Do not use printf, scanf, etc.
   2. Only allow printf in certain, justified, scenarios
   3. Only use project specific logging facilities e.g. LOG()
   4. Use printk in drivers, do not use printf, etc.
   5. Etc.
8. Versioning and version strings – ensure that all components in the system follow a consistent standard for versioning and version strings. E.g.
   1. Version strings shall be Major.Minor.Fix.Build
   2. Major is incremented in various scenarios
   3. Etc.
9. Language features – ensure that the use of certain language features are consistently followed or are disallowed.

John Arrizza is a (software development lifecycle) Process Improvement consultant for Advantu, and supports our clients on an ongoing basis. His expertise helps client teams reduce costly rework and methodically shrink time-to-market, while dramatically increasing customer satisfaction with their products.

Want to verify your Coding Practices are Up to Best Practice Standards?

Take a minute and request a Free Coding Practice Assessment (form below) – you may be closer than you think