Code Health Guardian: The Old-New Role of a Human Programmer in the AI Era

Kindle Highlights

Leave positive comments.

USE ASSERTIONS ONLY FOR CHECKING THINGS THAT SHOULD REALLY NEVER HAPPEN

“It’s programming if ‘clever’[17] is a compliment, but it’s software engineering if ‘clever’ is an accusation.”

The greatest limitation in writing software is our ability to understand the systems we are creating. [Ousterhout18]

For example, there is a widespread anti-pattern in Java of leaking HTTP request objects into an application layer from a Servlet

At Google, the expectation is that you “should do a code review shortly after it comes in. One business day is the maximum time it should take to respond.”

Yet the code by itself doesn’t explain why it’s doing what it is doing,[26] so comments answering the why question are orthogonal and complementary to the code itself, and in some cases, drastically improve readability.

There are two ways of constructing a software design: one way is to make it so simple that there are obviously no deficiencies and the other way is to make it so complicated that there are no obvious deficiencies. [Ashenhurst87]

There are enough edge cases inherent to the nature of software and the complexity of real-world systems for us to add even more of them to our codebases. If an edge case is avoidable, please do avoid it, especially in interfaces.

Of the five dynamics, one stands out ahead of the others. Our researchers found that the best teams created a climate of openness where team members admit to their errors and discuss them more often. In other words, they exhibited high levels of psychological safety.

The main problem with tests that use mocking frameworks is that they don’t truly enforce the contracts. Today, internal Google guidelines strongly recommend making tests as realistic as possible, that is, to use real objects in tests unless they’re slow or nondeterministic

One of my ex-managers seemed to be obsessed with asking during incident reviews whether any test that would have prevented the incident from happening was missing. But it’s a very good question to ask, given that automated tests are the primary mechanism of enforcing software correctness

The idea behind the Chesterton’s fence principle is rather simple—don’t remove a fence until you know why it was erected in the first place. It may have a purpose that’s not obvious or visible to us, so it’s important to stay intellectually humble and curious, and seek to understand before we act.

“[Minor] Your comment here” or “nit: Your comment here”—you don’t think it’s something serious, but you expect the author to fix it prior to merging the PR. “[Optional] Your comment here” or “FYI: Your comment here”—you think it might be better to do things differently, but it’s up to the author whether to change it or not.

It may be a synthetic example, but it illustrates perfectly the point that the “well-documented contract” requirement is not about excessive documentation. The interface is well-documented if documentation has all that is missing in code that may be necessary to use the module, and nothing else. In other words, documentation should not only be complete, but also concise.

Collect the requirements from the key stakeholders. Draft a design doc. Collaborate in the doc with the stakeholders (aim at closing all the major comment threads). Share the design doc with your team (same here: try to close all the major comment threads). [Optional] Go through a formal design review meeting. Change the design doc status to final and proceed with implementation.

Multiple unrelated changes in a PR => Split it into multiple PRs Change that is too large for a high-quality review => Break it down into a series of small changes[150] A substantial change that wasn’t discussed with the team => Discuss it (ask the author to write a one-pager?) It’s not a good time to add this functionality => Postpone it The change should be made by some other team instead => Hand it

Consider the following code: verify(quantity != null); verify(quantity > 0); verify(price != null); verify(price > 0); It’s not hard to imagine price=0 becoming a valid value in the future, which has nothing to do with the constraints on quantity. These checks should exist and evolve independently of each other. It’s important to remember that DRY is about knowledge duplication, not about code duplication.

when reviewing code, we’re faced with our errors more often than anywhere else.[148] Thus, freely discussing our mistakes, modeling being vulnerable, and showing genuine care for our colleagues during the review process is an unprecedented opportunity to gauge the level of psychological safety in the team. Don’t ever underestimate the importance of code reviews! They play a crucial role in shaping your team’s trust climate.

is extremely rare that the first draft of any code change is good enough. A healthy change is typically the result of multiple iterations of review and improvements. It’s a step-by-step process in which each subsequent step provides the necessary insights for the following one. Yet, it’s not about iterations only—a fresh perspective is another essential ingredient, and the easiest way to get it is to ask somebody else to take a

Aim to approve quickly. If there are no serious issues with the PR, consider giving an approval straightaway. You can still make comments explaining your minor concerns that need to be addressed by the author prior to merging the PR, but approve and trust them to do that. This way, they won’t need to wait for another round of review from your side. That’s a powerful trick that not only helps you deliver faster but also builds trust.

Here is the final list of the six rubrics to keep in mind when reviewing a PR: Test coverage: Are all the necessary tests there? Correctness: Does it look correct to you? Complexity: Is it the simplest change possible? Security: Are there any security issues? Documentation: Is there any relevant documentation, and is it (still) accurate enough? Consistency: Does the code follow your style guide, and does the solution fit well into the overall architecture? For

Documenting what a module expects (preconditions) and what it does (postconditions and invariants preserved) is at the very heart of modular design. Enforcing the contract internally is very much in the spirit of encapsulation—a module should encapsulate not only its contract’s implementation but its enforcement as well. And that’s all it takes to make interfaces trustworthy! If you practice these two rules consistently within your codebase, you’ll notice that over time, it builds trust in your interfaces among developers, serving as a solid foundation for keeping your codebase healthy.

WHY COLOR-CODED TABLES? I’ve first seen color-coded trade-off tables in internal Google documentation, comparing the pros and cons of different storage options: Spanner, BigTable, Firestore, and others. Their goal is to simplify trade-off analysis and the decision-making process by making it visual and avoiding duplication. The most popular alternative I’ve seen so far is a simple bullet list of pros and a separate list of cons for each of the options. But what has always confused me about such lists is the common DRY violations in them—the same argument would often appear (sometimes in different forms) in multiple lists. Thus, ever since I discovered color-coded trade-off tables, I strongly prefer them over the plain pros & cons lists. Making the summary of the trade-off analysis visual also simplifies both the decision-making process itself and communications around it.