Did you know that Codethink is able to review software code independently for your projects?
That is precisely what Ben Brewer (Head of Embedded at Codethink) did successfully for another of our clients, ASL Holdings Ltd recently, following a referral through our Microchip Design Partnership.
Codethink’s decades of collective software expertise has qualified us to independently review and scrutinise code, enabling us to provide sound advice on how to improve code before the code, quite often within products, ultimately hits the market. Additionally, we often provide guidance to our clients on how to ease the burden of long-term software maintenance during code review analysis.
Whilst writing and reviewing code is standard practice for our team at Codethink, not all companies are able to follow this methodology for various reasons. We’ve often found that an independent review provides not only a sanity check on the code but higher confidence and quality assurance for many of our clients.
Why Do We Bother With Code Review?
Code reviews have until recently always been a controversial issue especially amongst programmers, on the one hand a review from someone with more experience and expertise can be useful, but often-times rushed code reviews can lead to low-quality nitpicking and can become a hindrance to the developer, or worse: A code review can easily become a battle of ego’s and wills.
This article will discuss code reviews in general but will reference a recent in-depth code review we performed for ASL Holdings Ltd over the course of 2 weeks.
What is the point of this code review?
When performing a code review we should always ask ourselves what the point of the review actually is, and try to answer some basic questions.
Who is the target audience for the review?
A junior developer may request a review as a learning experience, and would expect any response to be as verbose and educational as possible.
A senior developer may request a review as a sanity check and will often expect a review to be as technically in-depth as possible to catch/discuss potential issues.
A manager may request a review either directly or as part of a company process, in this case they will need a high level summary to provide them feedback on the overall code quality on the project and as a feedback mechanism to feed into any personal development programs.
A company may request a review as part of an evidence chain for safety/security certification or approval, such a review would be expected to be as formal and verbose as possible as it will need to be referenced in the future.
In practice all code reviews are a mixture of the above, and before undertaking a code review it’s important to gauge the audience as the wrong review focus or format could lead to a bad code review.
In our work with ASL Holdings we were working to review their power monitoring devices for the purposes of both safety and quality. Since we knew the audience for this review from the start, it became clear that this review would need to be very technically in-depth and formal.
What does a successful review look like?
Once we know the target audience it becomes easier to determine what results are expected. In some cases this may be covered by company process or by the request for the review, but it’s often worthwhile to directly ask the main stakeholders what they expect to get out of a code review.
Typical expected outcomes from a code review can vary a lot, and include:
- Fixing known bugs
- Finding unknown bugs
- Describing potential maintenance issues
- Simplification of the codebase
- Providing optimizations
- Ensuring common conventions/styles are used
For a longer form review it’s often worthwhile to state the intentions of the review up-front as this will prepare the reader for what to expect from the review.
Where to Begin?
For a small code review it can be obvious where to begin if there’s relatively little to tackle, but this question can be more tricky to answer for large projects/commits.
In our work with ASL Holdings we were reviewing a mature codebase with a large number of C source and header files and no prior knowledge of the codebase. The lack of prior knowledge can be an important factor in providing an unbiased review and may be a requirement for safety, security or quality processes.
A larger code review should always begin with a discussion with the original author where possible as they will be able to point out any areas of concern, any known issues or any concerns they have about the code in general.
When digging into a large codebase it’s good to have a number of entry points as this can focus the mind, and once a high-level review of these areas is written the experience should allow for a wider understanding of the codebase.
Due to the requirements for the ASL Holdings code review it became obvious early on that the only acceptable outcome would be to review every single line of code.
Reading large quantities of code may seem daunting at first, however most codebases will have a relatively consistent style whether due to a specific code style being followed or just due to the limited number of authors. The important thing when tackling a large quantity of code is to keep very brief notes so as not to break your flow when reading through the codebase.
For the ASL code review we kept very brief notes indicating line-number and a couple of words to describe each issue; by keeping minimal notes this allowed us to read through the codebase quickly without interruption. When it came time to write the formal review; using only these notes it was possible to work backwards and focus on specific parts of the codebase in the review itself.
Reading through the codebase first also allows us to determine any recurring issues, and get a picture of how much time we’ll have available to dig into each review point.
Understanding how much time we had to focus and think about specific issues was important as we could plan to provide the most in-depth feedback in the time given. This allowed us to provide very in-depth feedback for the most critical point by referencing appropriate technical documentation and providing code improvement snippets.
Where to End?
By determining the audience, stakeholders and expectations of the review, it becomes easier to determine where the end-point should be.
Typically a code review shouldn’t end until you have a full understanding of the reviewed codebase such that you can read and understand every function/file. Where this is not the case, the level of depth should be clearly mentioned in the code review.
Often for shorter code reviews which may be part of a company commit process there’s a temptation to review briefly and accept a change, however if it’s not clear that this has been done, then bugs and technical debt can easily build up unnoticed.
In the case of our review for ASL Holdings we knew that a review of the entire codebase was required for quality and safety purposes, so it became our obligation to review with a full understanding.
While we had gathered requirements up-front to determine what a successful review might look like, it was important to leave some time for feedback to the code review as the output in our case was a formal document.
In our work with ASL Holdings we were given two weeks to review a PIC codebase for a couple of projects based on a PIC18 microcontroller. This review was referred to us through our partnership with Microchip Technologies due to both our expertise in providing embedded solutions and due to ASL Holdings being a local UK based company.
We provided an in-depth 57 page review for a codebase we had no prior experience of within 2 weeks and managed to provide bug fixes, code improvements and RAM/ROM optimizations which allowed space for ASL Holdings to consider adding additional features without increasing BOM cost.
Special thanks to Shaun Clements (Principal Design Engineer at ASL Holdings Ltd), Allan Sydney (CTO at ASL Holdings Ltd) and Andy Walton (Head of Operations and Quality at ASL Holdings Ltd) for this opportunity to work with ASL Holdings Ltd. It was a pleasure to work with you all.
ASL is connecting the world one device at a time, providing premier and innovative IoT connected technology solutions. Purchased by Halma plc in March 2013, ASL is now part of HWM-Water Ltd who are a global leader in the manufacture of data loggers and leak detection products for the water industry, within Halma’s Environmental & Analysis sector. For more information please visit their website http://www.aslh.co.uk
“Codethink provided us with a very valuable and comprehensive report after having independently reviewed our firmware code. After the team assessed the report internally and implemented the recommendations, we felt fully confident to release our products into the field. The service, communication and support from start to finish was excellent. Shaun and Ben worked together and understood each other very well, and all work was completed within the agreed time and budget. We would highly recommend using Codethink for all things software related and especially in our case where they needed to have a very good understanding of the firmware. A sanity check is always a good shout too. We would not hesitate to use Codethink again and they proved to be a great Microchip Design Partner. Thanks again” Andy Walton (Head of Operations and Quality at ASL Holdings Ltd).
If you’d like Codethink to undertake an independent code review on behalf of your project or require any software engineering or consultancy support, please feel free to get in touch with us, we’d be delighted to help:
firstname.lastname@example.org or +44 161 660 9930
In a nutshell, Codethink provides consultancy and software engineering services to international-scale customers. We help to design and deliver systems and software at all scales, from dedicated microcontroller/DSP/FPGA solutions to embedded medical devices, in-vehicle systems, network and cloud infrastructure. Codethink is one of the few firms in the market with the expertise to reliably deliver complete custom Linux systems and Open Source solutions for critical devices and services. For more information, please visit our website https://www.codethink.co.uk
- Using Git LFS and fast-import together
- Testing in a Box: Streamlining Embedded Systems Testing
- SDV Europe: What Codethink has planned
- How do Hardware Security Modules impact the automotive sector? The final blog in a three part discussion
- How do Hardware Security Modules impact the automotive sector? Part two of a three part discussion
- How do Hardware Security Modules impact the automotive sector? Part one of a three part discussion
- Automated Kernel Testing on RISC-V Hardware
- Automated end-to-end testing for Android Automotive on Hardware
- GUADEC 2023
- Embedded Open Source Summit 2023
- RISC-V: exploring a bug in stack unwinding
- Adding RISC-V Vector Cryptography Extension support to QEMU
- Introducing Our New Open-Source Tool: Quality Assurance Daemon
- Long Term Maintainability
- FOSDEM 2023
- Think before you Pip
- BuildStream 2.0 is here, just in time for the holidays!
- GNOME OS & Atomic Upgrades on the PinePhone
- Flathub-Codethink Collaboration
- Codethink proudly sponsors GUADEC 2022
- Tracking Down an Obscure Reproducibility Bug in glibc
- Web app test automation with `cdt`
- FOSDEM Testing and Automation talk
- Protecting your project from dependency access problems
- Porting GNOME OS to Microchip's PolarFire Icicle Kit
- YAML Schemas: Validating Data without Writing Code
- Deterministic Construction Service
- Codethink becomes a Microchip Design Partner
- Hamsa: Using an NVIDIA Jetson Development Kit to create a fully open-source Robot Nano Hand
- Using STPA with software-intensive systems
- Codethink achieves ISO 26262 ASIL D Tool Certification
- RISC-V: running GNOME OS on SiFive hardware for the first time
- Automated Linux kernel testing
- Native compilation on Arm servers is so much faster now
- Higher quality of FOSS: How we are helping GNOME to improve their test pipeline
- RISC-V: A Small Hardware Project
- Why aligning with open source mainline is the way to go
- Build Meetup 2021: The BuildTeam Community Event
- A new approach to software safety
- Does the "Hypocrite Commits" incident prove that Linux is unsafe?
- ABI Stability in freedesktop-sdk
- Why your organisation needs to embrace working in the open-source ecosystem
- RISC-V User space access Oops
- Tracking Players at the Edge: An Overview
- What is Remote Asset API?
- Running a devroom at FOSDEM: Safety and Open Source
- Meet the codethings: Understanding BuildGrid and BuildBox with Beth White
- Streamlining Terraform configuration with Jsonnet
- Bloodlight: Designing a Heart Rate Sensor with STM32, LEDs and Photodiode
- Making the tech industry more inclusive for women
- Bloodlight Case Design: Lessons Learned
- Safety is a system property, not a software property
- RISC-V: Codethink's first research about the open instruction set
- Meet the Codethings: Safety-critical systems and the benefits of STPA with Shaun Mooney
- Why Project Managers are essential in an effective software consultancy
- FOSDEM 2021: Devroom for Safety and Open Source
- Meet the Codethings: Ben Dooks talks about Linux kernel and RISC-V
- Here we go 2021: 4 open source events for software engineers and project leaders
- Xmas Greetings from Codethink
- Call for Papers: FOSDEM 2021 Dev Room Safety and Open Source Software
- Building the abseil-hello Bazel project for a different architecture using a dynamically generated toolchain
- Advent of Code: programming puzzle challenges
- Full archive