Engineering Practices: Code Owners Craft, Reviewers Polish
Reproduced from - https://2wisebit.wordpress.com/2025/01/01/engineering-practices-code-owners-craft-reviewers-polish/
In this comprehensive guide to code reviews, the importance of maintaining code health while balancing progress and quality is emphasized. The attainable nature of healthy code is highlighted, along with effective techniques for juggling speed and excellence. This guide is designed to help keep the codebase in top shape and ensure productivity within the team. Code reviews are shown to be an essential part of ensuring code quality and facilitating team collaboration.
Code Review Showdown: Who Does What?
In the domain of code reviews, both the reviewers and the owners play highly complementary roles for quality maintenance and fostering collaboration. Reviewers need to approve positive changes, stay focused on constant improvement, do technical evaluations with comments that create learning, work toward consensus for conflict resolution, verify design and functionality, update documentation, and respond promptly to review requests. The owner must ensure their code is high-quality and up to the team standards, maintain a mindset of continuous improvement, and adhere to coding standards. He or she must be open to feedback, clearly communicate about the design choices that have been taken, ensure changes are necessary, document updates very clearly, and respond to reviews in a timely manner while running all the required tests before submitting.
What to Look for in a Code Review
Design: Ensure logical integration and compatibility with the system. Avoid unnecessary functionality or premature additions.
Functionality: Verify intended outcomes and address edge cases, concurrency issues, and user-facing impacts. Request demos for user-facing changes if needed.
Complexity: Keep the code simple and understandable. Avoid over-engineering or speculative future requirements.
Tests: Ensure appropriate and correct tests are included. Verify that tests are maintainable and validate failures accurately.
Naming: Use descriptive, concise names that clearly communicate purpose.
Comments: Focus comments on explaining “why” over “what.” Make comments necessary, clear, and useful.
Style: Follow style guides strictly; avoid blocking CLs for personal preferences. Separate style changes from functional changes.
Consistency: Either follow style guides or be consistent with existing code. Encourage cleanup of inconsistent legacy code.
Documentation: Update documentation for changes affecting users or processes.
Review every line: Thoroughly review all human-written code. If code is difficult to understand or outside your area of expertise, ask for clarification.
Context: Consider the wider impact on the system and do not allow incremental degradation.
Praise: Acknowledge and appreciate good practices to encourage learning and improvement.
Final Checklist: The code is well-designed, functional, simple, and testable. Validate names, comments, and documentation. Ensure adherence to style guides and maintain consistency. Review every assigned line, consider context, and prioritize code health. Give constructive feedback and recognize excellence.
Navigate a CL in Review
Step 1: Take a Broad View
Step 2: Review the Key Components
Step 3: Review the Remaining Files
Key Tips:
Early intervention: Major design flaws should be addressed immediately.
Courtesy: Maintain respectful and constructive communication.
Efficiency: High-impact areas first, in order to optimize review effort.
Speed of Code Reviews
Quick code reviews are crucial as they optimize team efficiency over the velocity of an individual developer's speed. It prevents bottlenecks, which in turn delay the feature, bugs, and all further work. A quick response eliminates frustration and complaint from the review process, thus incentivizing the developer to put more quality into the work so they are not discouraged by cutting corners.
How Fast Are Code Reviews Supposed To Be?
One business day should be the maximum response time so that things run efficiently and keep the workflow smooth. If you are busy with something else, then try to address the review as soon as possible. When delays occur, the authors should be informed of the expected completion time to manage expectations and maintain efficiency.
Guarantee prompt responses:
To ensure that code reviews are quick and efficient, provide prompt feedback even if the entire review cannot be completed immediately. This can be done by offering initial general comments or proposing alternative reviewers.
Balancing speed with focus:
To balance speed with focus in code reviews, one should not interrupt in-depth tasks. Instead, respond at logical breakpoints, for example, after completing a task, following meetings, or during breaks. This will ensure that the reviews are done promptly without sacrificing the quality of work being undertaken.
Cross-time-zone reviews:
Try to give your feedback within the author's working hours, thus not causing delay. Approve the CL if there are just minor changes remaining or you believe the author can take care of the feedback.
Large CLs:
For large CLs, request that developers break them down into smaller, more manageable changes. If breaking down isn't practical, prioritize the design review. This way, the reviewer will unblock the developer while preparing to do a comprehensive review - and progress keeps on rolling from there.
Handling Emergencies:
Relax quality guidelines only when absolutely necessary and follow the team's definition of emergencies carefully.
The long-term benefits of following these principles include improved code quality, as developers learn to submit better Change Lists (CLs), which reduces the review workload. In addition, consistent practice leads to faster review cycles, as the team adapts to the process and becomes more efficient without compromising standards. By following these guidelines, code reviews can remain fast and effective to ensure high-quality outputs without delays.
How to Write Code Review Comments
To ensure effective code reviews, always maintain kindness and respect in your comments. Explain your reasoning to help developers understand the rationale behind your feedback. Strike a balance between giving explicit directions and allowing developers to determine how to address issues. Encourage simplicity by prompting developers to simplify their code or add meaningful comments, rather than just explaining complexities.
Courtesy: Focus on the code, not the developer.
Bad Example: "Why did you use this specific assembly instruction here when there's clearly a more efficient way?"
Good Example: "The assembly instruction used here might be adding unnecessary complexity without a clear performance benefit. It could be more efficient to use a simpler instruction set for boot initialization."
Be constructive. Don't use accusatory or very critical language. Use comments to build understanding and encourage improvement.
Explain Why
Provide context for your suggestions. This helps the developer learn and understand best practices.
Providing Guidance
Balancing Act: Point out problems without over-directing.
When Direct Guidance Is Better: If a solution is complex or the developer is less experienced, provide specific instructions or code snippets.
Reinforce Good Practices: Point out what you liked and why.
Label Comment Severity - Explain the severity of each comment:
Nit: Minor fixes (e.g., formatting, naming conventions). "Nit: Consider renaming this variable for clarity."
Optional/Consider: Recommendations that are not required. "Optional: You could refactor this into a helper function for reuse."
FYI: Informational comments for later use. "FYI: This library has a new API that might simplify this."
This helps the author prioritize and avoid misunderstanding the intent of the feedback.
Accepting Explanations
If you ask for an explanation, the reply should result in more readable code through simplification or insightful comments. Avoid relying on the review tool to explain things since future readers of the code will not have access to these discussions.
Exceptions
Explanations are acceptable in review tools if they cover context familiar to regular readers of the code but unknown to the reviewer.
Dealing with Pushback in Code Reviews
Handling Developer Pushback:
Think from Their Perspective:
When Developers Are Wrong:
Dealing with Developer Outrage:
Minimize Outrages:
Dealing with "Clean It Up Later" Opposition
Dealing With General Complaints About Stringency
Resolving Conflicts
When Disputes Persist: Refer to The Standard of Code Review for guiding principles. Focus on the shared goal of maintaining a healthy, sustainable codebase to reach consensus.
The CL Author's Guide to Getting Through Code Review
Keep it Small: Break up large changes into smaller, more manageable CLs. This keeps the reviewer focused and speeds up the process.
Be Clear and Descriptive: Give a good description of your CL. Explain why the change is necessary and what it does. This helps reviewers understand your intent without needing to ask for clarification.
Adhere to Coding Standards: Stick to your team's coding guidelines to make the review process smoother.
Test Your Code: Run all relevant tests before submitting the CL to ensure the changes work as expected and don't break anything else.
Remove Unrelated Changes: Don't include unrelated changes in your CL. Keep the focus on the task at hand to avoid confusing the reviewer.
Respond Quickly to Feedback: Generally, reviewers will respond quickly. So be sure to address the feedback as quickly as possible so that the process can move on.
Understand the Reasoning: When you receive feedback, be sure you understand the point of the reviewer. If you are unsure, ask for clarification before making the changes.
Be Open to Criticism: Code reviews are a learning opportunity. Even if the feedback feels harsh, remember it's about improving the code quality, not personal criticism.
Provide Context: If your CL contains complex logic or unusual design choices, explain why you made those decisions. This can help the reviewer understand your reasoning and avoid unnecessary back-and-forth.
Keep a Collaborative Tone: Frame your requests or justifications in a way that invites discussion. Code reviews should be collaborative, not adversarial.
Ask for Help if Needed: If you’re struggling with the review process or need additional input, don’t hesitate to ask your teammates for guidance.
Don't Fight, Explain: If you feel the reviewer's suggestion is wrong, why you want to do it that way. If the reviewer's suggestion seems reasonable, do it right away
领英推荐
Make a Change or Request More Information: If you're not sure of what the reviewer wants, don't guess: ask for more detail. If you are clear on what the reviewer wants, make the change and submit the CL.
Use Git for Iteration: Apply the changes you would make through git efficiently. Re-submit your CL quickly, should changes have been requested on your CL.
Ask for Final Review: Once all necessary changes are in place, ping the reviewer asking for a final review.
Patient yet Persistent: A review can get derailed - it is OK to send out follow-ups; just make sure you aren't pinging them too frequently.
Writing Good CL Descriptions
A Change List (CL) description is necessary for clear communication in version control, describing what was changed, why, and how it can be understood in the future. A good CL description helps future developers, including yourself, understand the context of changes made.
Key Components:
What: Briefly summarize the major change so readers understand without having to read the entire CL.
Why: Provide context and describe why the change was necessary and what decisions were important.
First Line:
Summary: Brief and concise. Should be standalone for ease of understanding.
Tone: Use an imperative sentence (e.g., “Optimize boot initialization by simplifying assembly instructions”).
Empty Line: Insert a blank line for clarity
Body of the Description:
Details and Context: Provide a description of the problem being addressed, describing why that approach was warranted and any pertinent background information.
Address Shortcomings: Mention potential shortcomings or areas for improvement.
Linking Resources: Provide links to external documents, noting future access limitations.
Avoid Vague Descriptions:
Avoid descriptions like "Fix bug" or "Add patch" as they lack sufficient information.
Examples of Good CL Descriptions:
Use of Tags:
Make use of tags in the original line minimally to keep things less messy; move tags in the body where necessary.
Review Before Submission:
The description should reflect the change and provide enough context.
Why Write Small CLs?
Quick Reviews: Very small CLs are much faster to review, thereby enabling much more frequent reviews without even the slightest possibility of missing essential details.
Thorough Reviews: Given fewer changes, reviewers are much more likely to spot potential issues or oversights.
Bug Prevention: Small changes are easier to reason about, so it is very unlikely that you will introduce bugs.
Less Work Loss: In case the very large CL is rejected, you can also waste a lot of work. The use of small CLs minimizes this possibility.
Easier Merges: It avoids large conflicts during merges and simplifies integrating smaller CLs
Easier Rollbacks: Rolling back small changes is more manageable than attempting to undo a massive one.
Better Design: It's much easier to focus on the small changes and ensure they are well-designed.
What is "Small"?
A CL should represent one self-contained change that is easy to understand, test, and review. There is no exact line for size, but typically 100 lines is a good target for most CLs. 1000 lines is usually too large.
Test Code: Always include related tests within the CL, not as a separate entity.
Implications: Small CLs should still be meaningful, not so fragmented that their implications are unclear.
When are Large CLs Alright?
File Deletions: Removing a file may be considered as one change, although it can be large.
Automatic CL: If a CL is generated automatically and requires little review, then it is acceptable to be larger.
Refactoring Large Structures: Large CLs may be required for complex refactoring that constitute a logical single change
Writing Small CLs Efficiently
Do Not Block: Use the waiting period for another set of changes or CLs.
Prepare in Advance: Decide beforehand how you would break up your work for this implementation.
File-based Split: Group files for logical purposes; keep the change to a specific protocol versus changing code in an unrelated part.
Stacking of Changes: In order, start building off of each CL such that you're constantly making tiny increments.
Split Strategies
Horizontal Splitting: Break code into independent layers that can be developed separately, such as separating API, service, and data model layers.
Vertical Splitting: Focus on independent full-stack features (e.g., implementing multiplication and division as separate features, despite some overlap).
Combination: Use a combination of horizontal and vertical splitting for more complex systems.
Refactoring and Test Code
Separate Refactoring: Move and rename classes in separate CLs from feature changes.
Test Code: Always include tests within the same CL as the functionality. If modifying test code independently, it can be in a separate CL.
Keep Builds Working: Ensure the system continues to function after every CL submission, even if changes are interdependent.
Split Functionality: If a CL seems too large, try to decompose it into smaller changes, including refactoring if possible.
Get Consent: If a large CL is unavoidable, discuss with your reviewers and get consent in advance.
Handling Reviewer Comments Effectively
Don't Take It Personally
Stay Calm: Review comments are to improve the codebase, not criticize you personally.
Constructive Feedback: Treat every comment as an opportunity to learn or improve the code.
Respond Professionally: Don't write angry responses. Take a step back if you need to cool down before responding.
Fix the Code
Clarify the Code: If some part of your code is misunderstood, so will future readers. Clarify by refactoring or comments.
Add Comments: Add comments to unclear but necessary parts that make sense, with your rationale.
Don't Be Defensive: Improving the code is more helpful than explaining it in the review tool.
Think Together
Understand the Request: Make sure you understand the comment. Ask for clarification if necessary.
Discuss: Disagree respectfully and collaboratively.
Elaborate on Your Rationale: Support your strategy with a rationale, tradeoffs, and facts. Share more information as needed.
Courteous and Respectful
Professionalism Reigned Supreme: Be respectful and courteous, even when you are disagreeing. Code reviews are for learning and collaboration.
Work Toward Common Ground: Aim for mutual understanding to work together on what's the best approach.
Consensus is Key: If you can't agree, have a detailed, respectful conversation about the pros and cons.
Use Standards for Disagreements: If consensus isn't reached, consult The Standard of Code Review for guidance.
Example of a good commit message with concise rule
git commit -m "feat(D2IQ-12345): Update BL 1 initialization for TF-A boot firmware"
Body of the commit message:
Added comprehensive guidelines to help developers write clear and concise
Git commit messages. The guidelines cover the importance of good commit
messages, the structure of a commit message, and best practices for
crafting meaningful commit descriptions.
Write your commit message in the imperative: "Fix bug" and not "Fixed
bug" or "Fixes bug." This convention matches up with commit messages
generated by commands like git merge and git revert.
A properly formed git commit subject line should always be able to
complete the following sentence: "If applied, this commit will
<your subject line here>."
Rules for a great git commit message style:
- Separate subject from body with a blank line.
- Do not end the subject line with a period.
- Capitalize the subject line and each paragraph.
- Use the imperative mood in the subject line.
- Wrap lines at 72 characters.
- Use the body to explain what and why you have done something. In
most cases, you can leave out details about how a change has been
made.
Information in commit messages:
- Describe why a change is being made.
- How does it address the issue?
- What effects does the patch have?
- Do not assume the reviewer understands what the original problem was.
- Do not assume the code is self-evident/self-documenting.
- Read the commit message to see if it hints at improved code structure.
- The first commit line is the most important.
- Describe any limitations of the current code.
- Do not include patch set-specific comments.
Key points include:
- Using capitalization and punctuation correctly.
- Writing in an imperative mood.
- Specifying the type of commit.
- Keeping the message length concise.
- Providing clear and direct content.
These guidelines aim to improve communication and collaboration within
engineering teams, future-proofing projects and aiding in better
troubleshooting and maintenance.
Ported from branch 'feature/commit-guidelines'
Commit ID: 8a7b9c1
Refer to the documentation for more details:
[Documentation URL](https://example.com/doc/commit-guidelines)
Change-Id: If0b179a97c0dca489edc0047da401bbb4ce09f39
Signed-off-by: [Your Name] <[email protected]>
Further Reading
Books for Reading
This is an important area. Wow of the thoughts that I have are as follows - What is the future of code review in the age of AI and Coding Co-pilots? How much of the review work can be done by Co-pilots? Is there an opportunity to create a specialized copilot for firmware code review? Can it check for functional safety ( this would be needed for automotive software)