Option 1: x = sanitize(readInput());
Option 2: x = readInput().replaceAll("'", "\\'");

Multiple benefits of your preferred option may apply. Choose the best answer.

Option 1: x = sanitize(readInput());
Option 2: x = readInput().replaceAll("'", "\\'");

A) Option 1 because it’s shorter

B) Option 1 because option 2 is just plain incorrect, even though it always works

C) Option 2 because it’s more clear what it does

D) Option 2 because it’s more efficient, avoiding the function call overhead

You might have had the reaction “How could something that always works be incorrect?” This is actually part of a very important concept which goes back at least to 1967: that of treating the guarantees of what a function must do as a different entity from what it actually does. One Googler told us that he considers understanding this as the dividing line between a junior and a senior engineer. This idea can be called “the implementation/interface distinction,” although that term is often used for a more elementary concept. There are other reasons to prefer Option 1, but it’s hard to beat “Option 2 is literally incorrect” for strength.

To explain this: There’s actually another instance of this idea which is much more widely taught, and we’ll start by drawing a connection there. In systems programming, the low-level C API used to write to the console is invoked like this: write(1,str,len). Here, write is a function that writes to a stream, and 1 is a constant that refers to stdout, the stream that by default writes to the console. Beginning programmers are taught to “not use magic numbers” and instead write write(stdout,str,len). The justification is that the definition stdout == 1 is a low-level implementation detail which is secret to another module. It should be possible to read and write this code without knowing that detail, and the code should still work even if that detail changes.

Another way to say that is that the code write(1,str,len) is incorrect as written, because it is not possible to know that it works without making assumptions about the rest of the system. The idea of the number 1 and the idea of the stream stdout are different, even though they are the same in implementation.

Back to the original question: the idea of a sanitized string and the idea of a string with all single quotes removed are different, even though they are the same in implementation. It is thus not possible to know that Option 2 actually sanitizes the string without knowing this detail. And this detail is far more likely to change than that stdout == 1, which actually is required by the POSIX standard.

At this point, the question of whether Option 2 is incorrect reduces to whether the exact format of sanitized strings should be considered secret to its module. But this is the case in nearly all contexts, for not doing so makes the program very hard to change, especially as the details of sanitization are referenced in ever-more places throughout the program.

This example comes from Jimmy’s Strange Loop Talk “You are a Program Synthesizer”, which spends significant time explaining this example and why programming languages for formal verification would actually give a compile error for Option 2. We explain the ideas more in the Advanced Software Design Web Course, as parts of the lessons on “the hidden layer of logic” and on secrets. But the concept behind this example is as old as programming.

Option 1 is not better in every way: Option 2 is clearer about what it does. It is a general fact that hiding information tends to make applications easier to change but harder to understand, a tension we term “The Information Hiding Dilemma.” But this is a case where the tradeoff swings very strongly towards hiding the information: the definition of sanitization is very easy to understand even with the indirection, details about sanitization can spread throughout the codebase in nasty subtle ways if it’s not hidden (as shown in the Strange Loop talk), and the problem description states this is a large application.

public class Rectangle {
    private int x;
    private int y;
    private int width;
    private int height;
    public Rectangle(int x, int y, int width, int height) {
        this.x = x;
        this.y = y;
        this.width = width;
        this.height = height;
    }
    // Getters omitted
    public int getArea() {
        return width * height;
    }
    // Compute perimeter of the rectangle
    public int getPerimeter() {
        return 2 * (width + height);
    }
}

Which of the following is mostly likely to be a design improvement to this class?

public class Rectangle {
    private int x;
    private int y;
    private int width;
    private int height;
    public Rectangle(int x, int y, int width, int height) {
        this.x = x;
        this.y = y;
        this.width = width;
        this.height = height;
    }
    // Getters omitted
    public int getArea() {
        return width * height;
    }
    // Compute perimeter of the rectangle
    public int getPerimeter() {
        return 2 * (width + height);
    }
}

A) The constructor should take two parameters, not four

B) The application should pass around arrays of integers instead of the Rectangle type to avoid the overhead of classes

C) Rectangle should be made to be an implementation of an IRectangle interface

D) The class should cache the area and perimeter instead of computing them anew each time

This question mostly rests on comparing one thing which is definitely a design improvement to the trained eye with several contenders whose benefits are more mixed or negative.

(A) is the correct answer. The current constructor API is flawed. For two reasons, it should instead take a Point and a Dimension, being called like newRectangle(point(x,y), dim(width,height)). The other answers are tempting but have far weaker support, and we shall discuss them below.

The first reason is that it fails what’s called The Plain English Test. Suppose you were to describe what a Rectangle is in plain English. When we’ve asked people to describe what data defines a rectangle, almost none have answered 4 integers; they are far more likely to say that it consists of the top left corner and its dimension. Changing the constructor to instead take a point and dimension brings the code closer to the level of thought.

Secondly, the current API invites misuse. With 4 integers in a row, the constructor allows for 24 possible argument orderings, 23 of which are incorrect. While most of these, like newRectangle(height,y,x,width), would look strange to many programmers, the incorrect call newRectangle(width,height,x,y) could very easily pass code review. Having it take a Point and Dimension turns such a mistake into a type error. Another approach is to use keyword arguments in languages that have them, although the Plain English Test still gives weight to the two-argument constructor.

The Plain English Test is a consequence of the Embedded Design Principle, which is the focus of Unit 2 of the Advanced Software Design Course. The issue of argument ordering is an instance of the Representable/Valid Principle, the subject of Unit 3.

public void setProductDisplayType(String type) {
    if (type.equals("compact")) {
        this.maxDescriptionLength = 140;
        this.numSimilarProductsToShow = 2;
        this.numReviewsToShow = 1;
    } else if (type.equals("detailed")) {
        this.maxDescriptionLength = 500;
        this.numSimilarProductsToShow = 5;
        this.numReviewsToShow = 7;
    }
}

Try to think of the best refactoring yourself before looking at the answers. We are deliberately not showing you full detail in the answers below, for fear that showing you the possible solutions would make this question too easy. But if you can think up the best refactoring, then it should be clear which of the answers below is the best match.

public void setProductDisplayType(String type) {
    if (type.equals("compact")) {
        this.maxDescriptionLength = 140;
        this.numSimilarProductsToShow = 2;
        this.numReviewsToShow = 1;
    } else if (type.equals("detailed")) {
        this.maxDescriptionLength = 500;
        this.numSimilarProductsToShow = 5;
        this.numReviewsToShow = 7;
    }
}

A) The best refactored version does the equivalent of looking up a COMPACT_PRODUCT_WIDTH in a hardcoded global hash table

B) The best refactored version contains some form of the triple (140, 2, 1)

C) The best refactored version replaces the hardcoded strings with constants `COMPACT` and `DETAILED`, defined either as String constants or as an enum

D) The best refactored version uses a switch statement instead of an if-statement

Fundamentally, the function needs to output either the three numbers (140, 2, 1) or the three numbers (500, 5, 7). While there are many superficially-different variants, there really aren’t that many ways to do this.

Here is example code for the best refactoring, which corresponds to Answer B. There are minor variations, but the key idea is to pass around a structure containing all the values for a configuration. You can click on the expandable bullets below to look at the code for the other answers.

public static ProductDisplayType COMPACT = new ProductDisplayType(140, 2, 1);
public static ProductDisplayType DETAILED = new ProductDisplayType(500, 5, 6);
public void setProductDisplayType(ProductDisplayType type) {
      this.maxDescriptionLength = type.getMaxDescriptionLength();
      this.numSimilarProductsToShow = type.getNumSimilarProductsToShow();
      this.numReviewsToShow = type.getNumReviewsToShow();
}

public void setProductDisplayType(String type) {
  this.maxDescriptionLength = productDisplayTypeDescriptionLengths.get(type);
  this.numSimilarProductsToShow =  productDisplayTypeNumSimilarProducts.get(type);
  this.numReviewsToShow =  productDisplayTypeNumReviews.get(type);
}

enum ProductDisplayType { COMPACT, DETAILED }
public void setProductDisplayType(ProductDisplayType type) {
  if (type == ProductDisplayType.COMPACT) {
    this.maxDescriptionLength = 140;
    this.numSimilarProductsToShow = 2;
    this.numReviewsToShow = 1;
  } else if (type == ProductDisplayType.DETAILED) {
    this.maxDescriptionLength = 500;
    this.numSimilarProductsToShow = 5;
    this.numReviewsToShow = 7;
  }
}

public void setProductDisplayType(String type) {
  switch (type) {
    case "compact":
      this.maxDescriptionLength = 140;
      this.numSimilarProductsToShow = 2;
      this.numReviewsToShow = 1;
      break;
    case "detailed":
      this.maxDescriptionLength = 500;
      this.numSimilarProductsToShow = 5;
      this.numReviewsToShow = 7;
      break;
  }
}

Looking at the code for the other solutions, it’s easy to explain why Answer B is better in nearly every way.

First, the disadvantages of the others: Option D is the easiest to eliminate because it is nearly identical to the original, as a switch statement and an if-else statement are both variants of the idea of a conditional. The switch statement will be marginally easier to extend if new options are added other than compact and detailed, but otherwise, it only has the disadvantages of being slightly longer.

The enum option, Answer C, is similar but marginally better. Fundamentally, the function before could take in one of two values, “compact” or “detailed”, and then conditionally branched over them. With this change, it is the exact same, except the values are now called COMPACT and DETAILED instead. The interface is more type-safe, however.

Answer A makes the code straight-line instead of conditional, at least superficially. It also makes it much easier to do the possible future work of making the product display type settings configurable and extensible. However, in a sense, it doesn’t actually eliminate the conditional, because if you were to describe what it does, you would still need to say “It sets the configuration to these values for ‘compact,’ and these other values for ‘detailed.’”

Answer B dominates. It has all the type-safety of Answer B, more brevity than Answer A, and shares the configurability-and-extensibility benefit of Answer A. And it is competitive with the other answers in how easy it is to see the range of possible values for the three variables being set; whether it’s better or worse depends on extra details such as code placement.

However, its greatest advantage is that it is the only solution that truly eliminates the conditional, making the function both shorter and simpler.

There are still a few marginal improvements that can be made on top of this. For instance, if you did this example in a different language that has keyword arguments, it may be beneficial to use them in the ProductDisplayType constructor. But introducing this data structure is the key ingredient in improving this code.

There are tools and techniques from programming language theory that let you look at code like this and come up with the main ways for changing it, so that you can pick the best refactoring without depending on creativity. We teach this focused subset of PL theory in Unit 5 of the Advanced Software Design Course, “Algebraic Refactoring.” There’s a similar example to Answer A in Jimmy’s Strange Loop talk that uses PL theory to explain why it’s still conditional, and this is covered more in one of the first exercises of our Advanced Software Design Course. Also check out this article where we give a detailed discussion of how to truly remove conditionals from code.

public class Post {
    private int id;
    private String content;
    private String author;
    private List<Comment> comments;
    public Post(int id, String content, String author, List<Comment> comments) {
        this.id = id;
        this.content = content;
        this.author = author;
        this.comments = new ArrayList<>(comments);
    }
    public int getId() {  return id; }
    public String getContent() {  return content;  }
    public void setContent(String content) {  this.content = content; }
    public String getAuthor() { return author; }
    public List<Comment> getComments() {
      return Collections.unmodifiableList(new ArrayList<>(comments));
    }
    public void addComment(Comment c) { comments.add(c); }
}

Which of the following proposed changes is most likely to improve the maintainability of this code and the rest of the system? Note that this code may contain other flaws not listed among these choices.

public class Post {
    private int id;
    private String content;
    private String author;
    private List<Comment> comments;
    public Post(int id, String content, String author, List<Comment> comments) {
        this.id = id;
        this.title = title;
        this.content = content;
        this.comments = new ArrayList<>(comments);
    }
    public int getId() {  return id; }
    public String getContent() {  return content;  }
    public void setContent(String content) {  this.content = content; }
    public String getAuthor() { return author; }
    public List<Comment> getComments() {
      return Collections.unmodifiableList(new ArrayList<>(comments));
    }
    public void addComment(Comment c) { comments.add(c); }
    }
}

A) The id of a Post should be a hash of its content and author

B) The Post class should not contain a list of Comments

C) The comments list should not be copied

D) The code should not assume that all Posts have authors

This example comes from Daniel Jackson’s book The Essence of Software, endnote 81 (first edition). The rule behind it is only mostly-objective, but that is still enough to make answer B unambiguously the best of the four answers.

That is: Although posts may contain comments, the Post class should have no references to the Comment class, because doing so hinders reusability.

We determine this using a powerful-but-underappreciated rule called the Parnas Subset Criteria, first stated in 1979. This rule requires us to ask: are there useful subsets of the program which contain posts but not comments? The answer to this question is almost certainly yes. Because of that, the Parnas Subset Criteria dictates that the Post class is not allowed to use the Comment class, for that would hinder reusability.

There are multiple possible fixes. One option is to keep the post/comment relation in a relational database, or its in-memory imitation, having an external Post->List<Comment> map. Another is to instead have Post contain a collection of PostAttachment’s, an abstract type whose implementations include Comment's. A third is to instead put a reference to Post’s within Comment’s instead of the reverse. Each of these has their own tradeoffs. Even without considering reusability or future changes, these possible fixes also simplify the code that deals with posts, for this need not be concerned with comments except to the minimal extent that they are attached to posts.

The Parnas Subset Criteria are one of many topics covered in Unit 6 of the Advanced Software Design Course, “Future-Proofing Code.”

This function is used to simultaneously compare an unordered set of objects (files, messages, etc) against a checksum. It first combines the digests of each object by taking their XOR, and then compares the result to the target checksum, also using XOR. Note that, while there are many disadvantages to using XOR as a digest, this example is adapted from a real codebase where those were deemed not important.

  1  public boolean compareDigestsToTarget(int[] digests, int target) {
  2    int combinedDigest = 0;
  3  
  4    for (int curDigest : digests) {
  5      combinedDigest = combinedDigest  ^ curDigest;
  6    }
  7  
  8    int difference = combinedDigest ^ target;  
  9    return difference == 0;
10  }

Choose the best of the following proposed code improvements. There may be other useful improvements not named.

  1  public boolean compareDigestsToTarget(int[] digests, int target) {
  2    int combinedDigest = 0;
  3  
  4    for (int curDigest : digests) {
  5      combinedDigest = combinedDigest  ^ curDigest;
  6    }
  7  
  8    int difference = combinedDigest ^ target;  
  9    return difference == 0;
10  }

A) The `target` parameter should be merged into the digests[], so that line 8 can be removed

B) The code should be optimized to have one less loop iteration, initializing combinedDigest to digest[0]

C) The two uses of the XOR (^) operator should be extracted into a function

D) The two uses of the XOR (^) operator should be extracted into two different functions

In our Advanced Software Design Course, we challenge students to come up with examples of two code snippets with identical implementations that nonetheless should not be merged, because their specifications are incompatible. One student, whose company builds messaging infrastructure, submitted this real example from their codebase:

/* returns a ClusterDigest object containing the differences (i.e. bitwise xor)
 * between two digests for the same cluster segment */
compare(other: ClusterDigest): ClusterDigest {
  return new ClusterDigest(this.digest ^ other.digest);
}
 
/* Merge two digests for different cluster segments (using bitwise xor) to produce a
 * new digest for the union of the two cluster segments */
merge(other: ClusterDigest): ClusterDigest {
  return new ClusterDigest(this.digest ^ other.digest);
}

This question is based directly on that example, featuring a (somewhat contrived) function that uses both operations.

This challenge rests on two questions. First, should the uses of XOR be extracted into a function at all? Second, if so, should it be the same function?

To understand why they should be extracted to a function, we employ the Embedded Design Principle. It works like this: The high-level intention of the code on line 5 is to combine two digests. It is a low level detail that the method of combining them is through the XOR. Likewise, the high-level intention of the code on line 8 is to take the difference of two digests, with its implementation via XOR a low-level detail. Compared to writing combineDigests(combinedDigest,curDigest) or digestDifference(combinedDigest,target), writing lines 5 and 8 requires more thought and knowledge. It requires more thinking to read these lines and reconstruct the high-level intention. And, as written, these lines must change if either the implementation of differencing or combining digests changes. The Embedded Design Principle states the programmer should do less thinking to write this code by using a function instead.

The above discussion also strongly implies that the extracted functions for line 5 and 8 should be different because they have different high-level intentions. Elaborating on this more: it should be possible to give a spec to the combineDigests function “returns a digest combining the two arguments” without stating what it means to combine them, and likewise for digestDifference. It follows that the two functions may evolve differently, i.e.: you may change what it means to combine two digests without changing what it means to difference them, and in that case the functions will need to be split. Doing so requires inspecting every usage of this combined function to determine whether that usage is for a combine or difference operation.

In short, because these two functions have different descriptions of what they do, they should remain separate even though their implementations are identical. Case in point: what would you call the combined function? It is hard to give a name more descriptive than xorDigests, at which point the function adds nothing over just using the XOR operation inline.

The Embedded Design Principle is the subject of Unit 2 of the Advanced Software Design Course. The lens of specifications is used all throughout the course, but is introduced in Unit 1, “The Hidden Layer of Logic.” The idea that two functions with identical implementations but different specs should not be deduplicated is presented in The Design of Software is a Thing Apart.