Tag Archives: Code Health

Testing on the Toilet: Separation of Concerns? That’s a Wrap!

This article was adapted from a Google Testing on the Toilet (TotT) episode. You can download a printer-friendly version of this TotT episode and post it in your office.


By Stefan Kennedy


The following function decodes a byte array as an image using an API named SpeedyImg. What maintenance problems might arise due to referencing an API owned by a different team?

SpeedyImgImage decodeImage(List<SpeedyImgDecoder> decoders, byte[] data) {
  SpeedyImgOptions options = getDefaultConvertOptions();
  for (SpeedyImgDecoder decoder : decoders) {
    SpeedyImgResult decodeResult = decoder.decode(decoder.formatBytes(data));
    SpeedyImgImage image = decodeResult.getImage(options);
    if (validateGoodImage(image)) { return image; }
  }
  throw new RuntimeException();
}



Details about how to call the API are mixed with domain logic, which can make the code harder to understand. For example, the call to decoder.formatBytes() is required by the API, but how the bytes are formatted isn’t relevant to the domain logic.


Additionally, if this API is used in many places across a codebase, then all usages may need to change if the way the API is used changes. For example, if the return type of this function is changed to the more generic SpeedyImgResult type, usages of SpeedyImgImage would need to be updated.


To avoid these maintenance problems, create wrapper types to hide API details behind an abstraction:

Image decodeImage(List<ImageDecoder> decoders, byte[] data) {
  for (ImageDecoder decoder : decoders) {
    Image decodedImage = decoder.decode(data);
    if (validateGoodImage(decodedImage)) { return decodedImage; }
  }
  throw new RuntimeException();
}


Wrapping an external API follows the Separation of Concerns principle, since the logic for how the API is called is separated from the domain logic. This has many benefits, including:
  • If the way the API is used changes, encapsulating the API in a wrapper insulates how far those changes can propagate across your codebase.
  • You can modify the interface or the implementation of types you own, but you can’t for API types.
  • It is easier to switch or add another API, since they can still be represented by the introduced types (e.g. ImageDecoder/Image).
  • Readability can improve as you don’t need to sift through API code to understand core logic.

Not all external APIs need to be wrapped. For example, if an API would take a huge effort to separate or is simple enough that it doesn't pollute the codebase, it may be better not to introduce wrapper types (e.g. library types like List in Java or std::vector in C++). When in doubt, keep in mind that a wrapper should only be added if it will clearly improve the code (see the YAGNI principle).


“Separation of Concerns” in the context of external APIs is also described by Martin Fowler in his blog post, Refactoring code that accesses external services. 


Testing on the Toilet: Separation of Concerns? That’s a Wrap!

This article was adapted from a Google Testing on the Toilet (TotT) episode. You can download a printer-friendly version of this TotT episode and post it in your office.


By Stefan Kennedy


The following function decodes a byte array as an image using an API named SpeedyImg. What maintenance problems might arise due to referencing an API owned by a different team?

SpeedyImgImage decodeImage(List<SpeedyImgDecoder> decoders, byte[] data) {
  SpeedyImgOptions options = getDefaultConvertOptions();
  for (SpeedyImgDecoder decoder : decoders) {
    SpeedyImgResult decodeResult = decoder.decode(decoder.formatBytes(data));
    SpeedyImgImage image = decodeResult.getImage(options);
    if (validateGoodImage(image)) { return image; }
  }
  throw new RuntimeException();
}



Details about how to call the API are mixed with domain logic, which can make the code harder to understand. For example, the call to decoder.formatBytes() is required by the API, but how the bytes are formatted isn’t relevant to the domain logic.


Additionally, if this API is used in many places across a codebase, then all usages may need to change if the way the API is used changes. For example, if the return type of this function is changed to the more generic SpeedyImgResult type, usages of SpeedyImgImage would need to be updated.


To avoid these maintenance problems, create wrapper types to hide API details behind an abstraction:

Image decodeImage(List<ImageDecoder> decoders, byte[] data) {
  for (ImageDecoder decoder : decoders) {
    Image decodedImage = decoder.decode(data);
    if (validateGoodImage(decodedImage)) { return decodedImage; }
  }
  throw new RuntimeException();
}


Wrapping an external API follows the Separation of Concerns principle, since the logic for how the API is called is separated from the domain logic. This has many benefits, including:
  • If the way the API is used changes, encapsulating the API in a wrapper insulates how far those changes can propagate across your codebase.
  • You can modify the interface or the implementation of types you own, but you can’t for API types.
  • It is easier to switch or add another API, since they can still be represented by the introduced types (e.g. ImageDecoder/Image).
  • Readability can improve as you don’t need to sift through API code to understand core logic.

Not all external APIs need to be wrapped. For example, if an API would take a huge effort to separate or is simple enough that it doesn't pollute the codebase, it may be better not to introduce wrapper types (e.g. library types like List in Java or std::vector in C++). When in doubt, keep in mind that a wrapper should only be added if it will clearly improve the code (see the YAGNI principle).


“Separation of Concerns” in the context of external APIs is also described by Martin Fowler in his blog post, Refactoring code that accesses external services. 


Code Health: Respectful Reviews == Useful Reviews

This is another post in our Code Health series. A version of this post originally appeared in Google bathrooms worldwide as a Google Testing on the Toilet episode. You can download a printer-friendly version to display in your office.

By Liz Kammer (Google), Maggie Hodges (UX research consultant), and Ambar Murillo (Google)

While code review is recognized as a valuable tool for improving the quality of software projects, code review comments that are perceived as being unclear or harsh can have unfavorable consequences: slow reviews, blocked dependent code reviews, negative emotions, or negative perceptions of other contributors or colleagues.

Consider these tips to resolve code review comments respectfully.

As a Reviewer or Author:
  • DO: Assume competence. An author’s implementation or a reviewer’s recommendation may be due to the other party having different context than you. Start by asking questions to gain understanding.
  • DO: Provide rationale or context, such as a best practices document, a style guide, or a design document. This can help others understand your decision or provide mentorship.
  • DO: Consider how comments may be interpreted. Be mindful of the differing ways hyperbole, jokes, and emojis may be perceived.
    Author Don’t:
    I prefer short names so I’d rather
    not change this. Unless you make
    me? :)
    		 
    Author Do:
    Best practice suggests omitting
    obvious/generic terms. I’m not
    sure how to reconcile that
    advice with this request.
  • DON’T: Criticize the person. Instead, discuss the code. Even the perception that a comment is about a person (e.g., due to using “you” or “your”) distracts from the goal of improving the code.
    Reviewer Don’t:
    Why are you using this approach?
    You’re adding unnecessary
    complexity.
    		 
    Reviewer Do:
    This concurrency model appears to
    be adding complexity to the
    system without any visible
    performance benefit.
  • DON’T: Use harsh language. Code review comments with a negative tone are less likely to be useful. For example, prior research found very negative comments were considered useful by authors 57% of the time, while more-neutral comments were useful 79% of the time.  

As a Reviewer:
  • DO: Provide specific and actionable feedback. If you don’t have specific advice, sometimes it’s helpful to ask for clarification on why the author made a decision. 
    Reviewer Don’t:
    I don’t understand this.
    		 
    Reviewer Do:
    If this is an optimization, can you
    please add comments?
  • DO: Clearly mark nitpicks and optional comments by using prefixes such as ‘Nit’ or ‘Optional’. This allows the author to better gauge the reviewer’s expectations.

As an Author:
  • DO: Clarify code or reply to the reviewer’s comment in response to feedback. Failing to do so can signal a lack of receptiveness to implementing improvements to the code.
    Author Don’t:
    That makes sense in some cases but
    not here.
    		 
    Author Do:
    I added a comment about why
    it’s implemented that way.
  • DO: When disagreeing with feedback, explain the advantage of your approach. In cases where you can’t reach consensus, follow Google’s guidance for resolving conflicts in code review.

Code Health: Make Interfaces Hard to Misuse

This is another post in our Code Health series. A version of this post originally appeared in Google bathrooms worldwide as a Google Testing on the Toilet episode. You can download a printer-friendly version to display in your office.

By Marek Kiszkis

We all try to avoid errors in our code. But what about errors created by callers of your code? A good interface design can make it easy for callers to do the right thing, and hard for callers to do the wrong thing. Don't push the responsibility of maintaining invariants required by your class on to its callers.
Can you see the issues that can arise with this code?
class Vector {
  explicit Vector(int num_slots);  // Creates an empty vector with `num_slots` slots.
  int RemainingSlots() const;  // Returns the number of currently remaining slots.
  void AddSlots(int num_slots);  // Adds `num_slots` more slots to the vector.
  // Adds a new element at the end of the vector. Caller must ensure that RemainingSlots()  
  // returns at least 1 before calling this, otherwise caller should call AddSlots().
  void Insert(int value);
}

If the caller forgets to call AddSlots(), undefined behavior might be triggered when Insert() is called. The interface pushes complexity onto the caller, exposing the caller to implementation details.

Since maintaining the slots is not relevant to the caller-visible behaviors of the class, don't expose them in the interface; make it impossible to trigger undefined behavior by adding slots as needed in Insert().
@Test public void class Vector {
  explicit Vector(int num_slots);
  // Adds a new element at the end of the vector. If necessary,
  // allocates new slots to ensure that there is enough storage
  // for the new value.
  void Insert(int value);
}


Contracts enforced by the compiler are usually better than contracts enforced by runtime checks, or worse, documentation-only contracts that rely on callers to do the right thing.
Here are other examples that could signal that an interface is easy to misuse:
  • Requiring callers to call an initialization function (alternative: expose factory methods that return your object fully initialized).
  • Requiring callers to perform custom cleanup (alternative: use language-specific constructs that ensure automated cleanup when your object goes out of scope).
  • Allowing code paths that create objects without required parameters (e.g. a user without an ID).
  • Allowing parameters for which only some values are valid, especially if it is possible to use a more appropriate type (e.g. prefer Duration timeout instead of int timeout_in_millis).
It is not always practical to have a foolproof interface. In certain cases, relying on static analysis or documentation is necessary since some requirements are impossible to express in an interface (e.g. that a callback function needs to be thread-safe).

Don’t enforce what you don’t need to enforce - avoid code that is too defensive. For example, extensive validation of function parameters can increase complexity and reduce performance.

Code Health: Understanding Code In Review


This is another post in our Code Health series. A version of this post originally appeared in Google bathrooms worldwide as a Google Testing on the Toilet episode. You can download a printer-friendly version to display in your office.

By Max Kanat-Alexander


It's easy to assume that a developer who sends you some code for review is smarter than you'll ever be, and that's why you don't understand their code.

But in reality, if code is hard to understand, it's probably too complex. If you're familiar with the programming language being used, reading healthy code should be almost as easy as reading a book in your native language.

Pretend a developer sends you this block of Python to be reviewed:
def IsOkay(n):
  f = False
  for i in range(2, n):
    if n % i == 0:
      f = True
  return not f

Don't spend more than a few seconds trying to understand it. Simply add a code review comment saying, "It's hard for me to understand this piece of code," or be more specific, and say, "Please use more descriptive names here."

The developer then clarifies the code and sends it to you for review again:
def IsPrime(n):
  for divisor in range(2, n / 2):
    if n % divisor == 0:
      return False

  return True

Now we can read it pretty easily, which is a benefit in itself.

Often, just asking a developer to clarify a piece of code will result in fundamental improvements. In this case, the developer noticed possible performance improvements since the code was easier to read—the function now returns earlier when the number isn't prime, and the loop only goes to n/2 instead of n.

However, now that we can easily understand this code, we can see many problems with it. For example, it has strange behavior with 0 and 1, and there are other problems, too. But most importantly, it is now apparent that this entire function should be removed and be replaced with a preexisting function for detecting if a number is prime. Clarifying the code helped both the developer and reviewer.

In summary, don't waste time reviewing code that is hard to understand, just ask for it to be clarified. In fact, such review comments are one of the most useful and important tools a code reviewer has!

Code Health: Obsessed With Primitives?


This is another post in our Code Health series. A version of this post originally appeared in Google bathrooms worldwide as a Google Testing on the Toilet episode. You can download a printer-friendly version to display in your office.

By Marc Eaddy

Programming languages provide basic types, such as integers, strings, and maps, which are useful in many contexts. For example, a string can be used to hold everything from a person's name to a web page URL. However, code that relies too heavily on basic types instead of custom abstractions can be hard to understand and maintain.

Primitive obsession is the overuse of basic ("primitive") types to represent higher-level concepts. For example, this code uses basic types to represent shapes:
vector<pair<int, int>> polygon = ...
pair<pair<int, int>, pair<int, int>> bounding_box = GetBoundingBox(polygon);
int area = (bounding_box.second.first  - bounding_box.first.first) *
           (bounding_box.second.second - bounding_box.first.seco
pair is not the right level of abstraction because its generically-named first and second fields are used to represent X and Y in one case and lower-left (er, upper-left?) and upper-right (er, lower-right?) in the other. Worse, basic types don't encapsulate domain-specific code such as computing the bounding box and area.

Replacing basic types with higher-level abstractions results in clearer and better encapsulated code:
Polygon polygon = ...
int area = polygon.GetBoundingBox().GetArea();
Here are some other examples of primitive obsession:
  • Related maps, lists, vectors, etc. that can be easily combined into a single collection by consolidating the values into a custom higher-level abstraction.
    map<UserId, string> id_to_name;
    map<UserId, int> id_to_age;
    		 
    map<UserId, Person> id_to_person;
  • A vector or map with magic indices/keys, e.g. string values at indices/keys 0, 1, and 2 hold name, address, and phone #, respectively. Instead, consolidate these values into a higher-level abstraction.
    person_data[kName] = "Foo";
    		 
    person.SetName("Foo");
  • A string that holds complex or structured text (e.g. a date). Instead, use a higher-level abstraction (e.g. Date) that provides self-documenting accessors (e.g. GetMonth) and guarantees correctness.
    string date = "01-02-03";
    		 
    Date date(Month::Feb, Day(1), Year(2003));
  • An integer or floating point number that stores a time value, e.g. seconds. Instead, use a structured timestamp or duration type.
    int timeout_secs = 5;
    		 
    Duration timeout = Seconds(5);
It's possible for any type—from a lowly int to a sophisticated red-black tree—to be too primitive for the job. If you see code that uses a lot of basic types that would be clearer or better encapsulated by using a higher-level abstraction, refactor it or politely remind the author to keep it classy!

Code Health: IdentifierNamingPostForWorldWideWebBlog


This is another post in our Code Health series. A version of this post originally appeared in Google bathrooms worldwide as a Google Testing on the Toilet episode. You can download a printer-friendly version to display in your office.

By Chris Lewis and Bob Nystrom


It's easy to get carried away creating long identifiers. Longer names often make things more readable. But names that are too long can decrease readability. There are many examples of variable names longer than 60 characters on GitHub and elsewhere. In 58 characters, we managed this haiku for you to consider:

Name variables
Using these simple guidelines
Beautiful source code

Names should be two things: clear (know what it refers to) and precise (know what it does not refer to). Here are some guidelines to help:

• Omit words that are obvious given a variable's type declaration.
// Bad, the type tells us what these variables are:
String nameString; List<datetime> holidayDateList;
// Better:
String name; List<datetime> holidays;

• Omit irrelevant details.
// Overly specific names are hard to read:
Monster finalBattleMostDangerousBossMonster; Payments nonTypicalMonthlyPayments;
// Better, if there's no other monsters or payments that need disambiguation:
Monster boss; Payments payments;


• Omit words that are clear from the surrounding context.
// Bad, repeating the context:
class AnnualHolidaySale {int annualSaleRebate; boolean promoteHolidaySale() {...}}
// Better:
class AnnualHolidaySale {int rebate; boolean promote() {...}}

• Omit words that could apply to any identifier.
You know the usual suspects: data, state, amount, number, value, manager, engine, object, entity, instance, helper, util, broker, metadata, process, handle, context. Cut them out.

There are some exceptions to these rules; use your judgment. Names that are too long are still better than names that are too short. However, following these guidelines, your code will remain unambiguous and be much easier to read. Readers, including "future you,” will appreciate how clear your code is!

Code Health: Providing Context with Commit Messages and Bug Reports

This is another post in our Code Health series. A version of this post originally appeared in Google bathrooms worldwide as a Google Testing on the Toilet episode. You can download a printer-friendly version to display in your office.

By Chris Lewis

You are caught in a trap. Mechanisms whirl around you, but they make no sense. You desperately search the room and find the builder's original plans! The description of the work order that implemented the trap reads, "Miscellaneous fixes." Oh dear.

Reading other engineers' code can sometimes feel like an archaeology expedition, full of weird and wonderful statements that are hard to decipher. Code is always written with a purpose, but sometimes that purpose is not clear in the code itself. You can address this knowledge gap by documenting the context that explains why a change was needed. Code comments provide context, but comments alone sometimes can’t provide enough.

There are two key ways to indicate context:
Commit Messages

  • A commit message is one of the easiest, most discoverable means of providing context. When you encounter lines of code that may be unclear, checking the commit message which introduced the code is a great way to gain more insight into what the code is meant to do.
  • Write the first line of the commit message so it stands alone, as tools like GitHub will display this line in commit listing pages. Stand-alone first lines allow you to skim through code history much faster, quickly building up your understanding of how a source file evolved over time. Example:   
    Add Frobber to the list of available widgets.

    This allows consumers to easily discover the new Frobber widget and
    add it to their application.
Bug Reports
  • You can use a bug report to track the entire story of a bug/feature/refactoring, adding context such as the original problem description, the design discussions between the team, and the commits that are used to solve the problem. This lets you easily see all related commits in one place, and allows others to easily keep track of the status of a particular problem.
  • Most commits should reference a bug report. Standalone commits (e.g. one-time cleanups or other small unplanned changes) don't need their own bug report, though, since they often contain all their context within the description and the source changes.
Informative commit messages and bug reports go hand-in-hand, providing context from different perspectives. Keep in mind that such context can be useful even to yourself, providing an easy reminder about the work you did last week, last quarter, or even last year. Future you will thank past you!

Code Health: Eliminate YAGNI Smells

This is another post in our Code Health series. A version of this post originally appeared in Google bathrooms worldwide as a Google Testing on the Toilet episode. You can download a printer-friendly version to display in your office.

By Marc Eaddy

The majority of software development costs are due to maintenance. One way to reduce maintenance costs is to implement something only when you actually need it, a.k.a. the “You Aren't Gonna Need It” (YAGNI) design principle. How do you spot unnecessary code? Follow your nose!

A code smell is a code pattern that usually indicates a design flaw. For example, creating a base class or interface with only one subclass may indicate a speculation that more subclasses will be needed in the future. Instead, practice incremental development and design: don't add the second subclass until it is actually needed.

The following C++ code has many YAGNI smells:
class Mammal { ...
  virtual Status Sleep(bool hibernate) = 0;
};
class Human : public Mammal { ...
  virtual Status Sleep(bool hibernate) {
    age += hibernate ? kSevenMonths : kSevenHours;
    return OK;
  }
};

Maintainers are burdened with understanding, documenting, and testing both classes when only one is really needed. Code must handle the case when hibernate is true, even though all callers pass false, as well as the case when Sleep returns an error, even though that never happens. This results in unnecessary code that never executes. Eliminating those smells simplifies the code:

class Human { ...
  void Sleep() { age += kSevenHours; }
};

Here are some other YAGNI smells:
  • Code that has never been executed other than by tests (a.k.a. code that is dead on arrival)
  • Classes designed to be subclassed (have virtual methods and/or protected members) that are not actually subclassed
  • Public or protected methods or fields that could be private
  • Parameters, variables, or flags that always have the same value
Thankfully, YAGNI smells, and code smells in general, are often easy to spot by looking for simple patterns and are easy to eliminate using simple refactorings.

Are you thinking of adding code that won't be used today? Trust me, you aren't gonna need it!


Code Health: Too Many Comments on Your Code Reviews?

This is another post in our Code Health series. A version of this post originally appeared in Google bathrooms worldwide as a Google Testing on the Toilet episode. You can download a printer-friendly version to display in your office.

By Tom O'Neill


Code reviews can slow down an individual code change, but they’re also an opportunity to improve your code and learn from another intelligent, experienced engineer. How can you get the most out of them?
Aim to get most of your changes approved in the first round of review, with only minor comments. If your code reviews frequently require multiple rounds of comments, these tips can save you time.

Spend your reviewers’ time wisely—it’s a limited resource. If they’re catching issues that you could easily have caught yourself, you’re lowering the overall productivity of your team.
Before you send out the code review:
  • Re-evaluate your code: Don’t just send the review out as soon as the tests pass. Step back and try to rethink the whole thing—can the design be cleaned up? Especially if it’s late in the day, see if a better approach occurs to you the next morning. Although this step might slow down an individual code change, it will result long-term in greater average throughput.
  • Consider an informal design discussion: If there’s something you’re not sure about, pair program, talk face-to-face, or send an early diff and ask for a “pre-review” of the overall design.
  • Self-review the change: Try to look at the code as critically as possible from the standpoint of someone who doesn’t know anything about it. Your code review tool can give you a radically different view of your code than the IDE. This can easily save you a round trip.
  • Make the diff easy to understand: Multiple changes at once make the code harder to review. When you self-review, look for simple changes that reduce the size of the diff. For example, save significant refactoring or formatting changes for another code review.
  • Don’t hide important info in the submit message: Put it in the code as well. Someone reading the code later is unlikely to look at the submit message.
When you’re addressing code review comments:
  • Re-evaluate your code after addressing non-trivial comments: Take a step back and really look at the code with fresh eyes. Once you’ve made one set of changes, you can often find additional improvements that are enabled or suggested by those changes. Just as with any refactoring, it may take several steps to reach the best design.
  • Understand why the reviewer made each comment: If you don’t understand the reasoning behind a comment, don’t just make the change—seek out the reviewer and learn something new.
  • Answer the reviewer’s questions in the code: Don’t just reply—make the code easier to understand (e.g., improve a variable name, change a boolean to an enum) or add a comment. Someone else is going to have the same question later on.