---
title: "Reading, Debugging and Software Design"
book: "Interviews III: Code"
subject: quant
language: en
chapter: 9
exercises: 0
source: https://one-course.com/books/quant/20/en/chapter/9-reading-debugging-and-software-design
license: CC-BY-NC-SA-4.0
credit: "One Quant Book, One Course (one-course.com)"
---

# Chapter 9 — Reading, Debugging and Software Design

Reading a short listing and saying what it really does, then naming the bug, the tool that would prove it and the smallest fix. The first questions are the ones you are expected to spot in seconds. The later ones ask how you would hunt a fault you cannot see, how you would test a piece of trading code, and how you would shape it so the same logic runs live and in a backtest. A few stems are in Java or C#, because banks ask them there.

**Question 9.1 ★ QD, Dev, LLE • any • junior.**

Built with g++ 11.4 on x86-64 Linux, what does [Listing 9.1](#lst-iv3-rd-unsigned) print? Is anything undefined here? Give the smallest fix for the comparison.

```cpp
int main() {
    std::vector<int> bids{10, 20};
    std::vector<int> asks{10, 20, 30};
    if (bids.size() - asks.size() > 0) {
        std::puts("more bids than asks");
    } else {
        std::puts("no more bids than asks");
    }
    std::printf("%zu\n", bids.size() - asks.size());
}
```

***Listing 9.1.** Comparing two sizes. code/interviews-3/09-reading-debugging-and-software-design/cpp/unsigned_diff.cpp*

**Answer to Question 9.1.**

It prints `more bids than asks`, then `18446744073709551615`. Nothing is undefined. The fix is to compare the sizes directly: `bids.size() > asks.size()`.

`size()` returns `std::size_t`, an unsigned type of 64 bits here. The standard defines unsigned arithmetic modulo $2^{64}$, so $2 - 3$ gives $2^{64} - 1$, which is the second line of output. An unsigned value is never negative, so `> 0` is true for every pair of sizes that differ, whichever is larger. Under `-Wall -Wextra -Werror` the listing compiles without a word, because `> 0` on an unsigned value is a sensible test in general.

If you do need the signed difference, C++20 has `std::ssize`. Subtracting two of those and testing `> 0` also gives the right branch.

**Classic wrong answer.** “Overflow is undefined behaviour, so anything can happen.” That is true of signed types. Unsigned arithmetic wraps by definition, so this program is perfectly defined and perfectly wrong.

**Follow-up.** *What does `for (std::size_t i = v.size() - 1; i >= 0; --i)` do?* It never ends, because `i >= 0` is always true: after 0 it wraps to the largest value and indexes far out of bounds. Here `-Wextra` does catch it, as `-Wtype-limits`. An empty `v` goes wrong even earlier, on the first iteration.

**Why it matters.** Quantity and level arithmetic is full of sizes. This bug passes every test where the first container happens to be the larger one.

**Question 9.2 ★ QR, QD, Dev, DS • any • junior.**

What does [Listing 9.2](#lst-iv3-rd-late) print, and why? Make it print `[10, 20, 30]`. Would rewriting the loop as a list comprehension help?

```python
def make_scalers():
    scalers = []
    for k in [1, 2, 3]:
        scalers.append(lambda x: k * x)
    return scalers


print([f(10) for f in make_scalers()])
```

***Listing 9.2.** Three scalers. code/interviews-3/09-reading-debugging-and-software-design/python/rd_late_binding.py*

**Answer to Question 9.2.**

It prints `[30, 30, 30]`. To get `[10, 20, 30]`, bind the value when the function is created: `lambda x, k=k: k * x`. A list comprehension does not help.

A closure in Python captures the variable, not its value. All three lambdas refer to the same `k` in the scope of `make_scalers`, and they look it up when called. By then the loop has finished and `k` is 3. Default arguments are evaluated when the function is defined, which is why `k=k` takes a snapshot. `functools.partial` does the same and leaves no extra parameter that a caller could pass by accident.

**Classic wrong answer.** “Use a comprehension, since it has its own scope.” It does, but only one scope for the whole comprehension. `[lambda x: k * x for k in [1, 2, 3]]` also gives `[30, 30, 30]`: the test runs it.

**Why it matters.** Building a list of callbacks or scalers in a loop is everyday research code, and the failure is silent: every instrument gets the last instrument’s parameters.

**Question 9.3 ★ QD, Dev, LLE • any • junior.**

[Listing 9.3](#lst-iv3-rd-erase) is meant to drop every zero quantity. Built with g++ 11.4, `-std=c++20 -O2`, it prints something. What, and why? What happens if the last element is the zero? Which tool shows the fault, and what is the fix?

```cpp
int main() {
    std::vector<long> qty{10, 0, 0, 20};
    for (auto it = qty.begin(); it != qty.end(); ++it) {
        if (*it == 0) {
            qty.erase(it);
        }
    }
    for (long q : qty) std::printf("%ld ", q);
    std::puts("");
}
```

***Listing 9.3.** Dropping zeros in a loop. code/interviews-3/09-reading-debugging-and-software-design/cpp/erase_loop.cpp*

**Answer to Question 9.3.**

On this compiler it prints `10 0 20`, one zero survives, and the program is undefined. With a zero in last place it runs off the end of the vector. Use `std::erase(qty, 0L)`.

`erase` invalidates every iterator at or after the erased element, so `it` may no longer be used, and `++it` is already undefined. What happened in the run: the elements slid left, so `it` pointed at the second zero, and `++it` stepped over it. That is why one zero survives.

Now make the input `{10, 0}`. The erase leaves `it` equal to `end()`, the `++it` moves past it, and the test `it != qty.end()` never becomes false. AddressSanitizer reports a heap-buffer-overflow on that input. The libstdc++ debug mode, `-D_GLIBCXX_DEBUG`, stops even the first input, with a message about a singular iterator.

C++20’s `std::erase` does the job in one pass. Before C++20 you would write the erase-remove idiom. The manual loop can be repaired with `it = qty.erase(it)` and no increment on that branch, but each erase shifts the tail, so it costs $O(n^2)$ where `std::erase` costs $O(n)$.

**Classic wrong answer.** “It removes all the zeros, since `erase` returns normally.” Reading the loop on `{10, 0, 0, 20}` step by step shows the skip at once. Do that aloud in the interview.

**Why it matters.** Removing while iterating is the classic reading question, and the follow-up about the last element is where candidates show whether they see the undefined behaviour or only the wrong output.

**Question 9.4 ★ QD, Dev, LLE • any • junior.**

What does [Listing 9.4](#lst-iv3-rd-mapindex) print? There are two faults in `allowed`. One is a wrong answer and one is a side effect. What is each, and how would you make the compiler catch the second?

```cpp
std::map<std::string, long> limits{{"AAA", 1000}};

bool allowed(const std::string& symbol, long qty) {
    return qty <= limits[symbol];
}

int main() {
    std::printf("%d\n", allowed("AAA", 500));
    std::printf("%d\n", allowed("BBB", 0));
    std::printf("%zu\n", limits.size());
}
```

***Listing 9.4.** A limit check. code/interviews-3/09-reading-debugging-and-software-design/cpp/map_index.cpp*

**Answer to Question 9.4.**

It prints `1`, `1`, `2`. The wrong answer: a symbol with no limit is allowed. The side effect: asking about `BBB` inserts it, so the map grows from one entry to two.

`std::map::operator[]` is defined as `try_emplace(x).first->second`: if the key is absent, it inserts a value-initialised entry, here 0, and returns a reference to it. So `limits["BBB"]` creates a limit of 0, and $0 \le 0$ passes. A risk check that says yes to an instrument it has never heard of should fail closed.

To make the compiler catch it, make the map `const`, or take it by `const&`. `operator[]` has no const overload, so the code stops compiling: the test does this. Then write the lookup with `find`:

`const auto it = limits.find(symbol);`

`return it != limits.end() && qty <= it->second;`

That prints `1`, `0`, `1`. `at` is the other choice, and it throws on a missing key.

**Follow-up.** *Why does the side effect matter if the wrong answer is fixed some other way?* Because the map is written during what looks like a read. Two threads calling `allowed` now race, even though neither thinks it is a writer. Memory also grows with every misspelt symbol.

**Why it matters.** This is the shortest example of a read that writes, and such a line appears in real risk checks.

**Question 9.5 ★ QD, Dev, LLE • any • junior.**

What does [Listing 9.5](#lst-iv3-rd-ctor) print, and why? Suppose `name` were pure virtual in `Strategy`, and the constructor called it through another member function. What then? How would you get the behaviour its author wanted?

```cpp
struct Strategy {
    Strategy() { std::printf("start %s\n", name()); }
    virtual ~Strategy() = default;
    virtual const char* name() const { return "base"; }
};

struct Momentum : Strategy {
    const char* name() const override { return "momentum"; }
};

int main() {
    Momentum m;
    std::printf("run %s\n", m.name());
}
```

***Listing 9.5.** A virtual call during construction. code/interviews-3/09-reading-debugging-and-software-design/cpp/ctor_virtual.cpp*

**Answer to Question 9.5.**

It prints `start base`, then `run momentum`. During `Strategy`’s constructor the object is a `Strategy`, so the call goes to `Strategy::name`.

The standard says that a virtual function called from a constructor resolves to the final overrider in the constructor’s own class, not in a more-derived one. The reason is sound: `Momentum`’s members do not exist yet, and an override might use them. Once construction ends, `m.name()` sees `Momentum` as expected.

If `name` is pure and the constructor calls it directly, the compiler can complain. Through a helper it cannot see the problem. That call is undefined, and at `-O0` the test run aborts with `pure virtual method called`.

To get what the author wanted, separate construction from starting. Either have a factory that builds the object and then calls `start()` on it, or pass the name up through the constructor: `Strategy(const char* name)`.

**Classic wrong answer.** “Virtual calls always go to the most-derived class.” Not during construction or destruction. The same holds in a destructor, where the derived part has already gone.

**Why it matters.** Logging or registering in a base-class constructor is a natural thing to write, and it is the usual way this bug appears: a strategy registers itself under the wrong name.

**Question 9.6 ★ QD, Dev, LLE • bank • junior.**

What three numbers does [Listing 9.6](#lst-iv3-rd-slicing) print? Name the effect, give the usual fix for the book, and say how to make the mistake fail to compile.

```cpp
struct Instrument {
    virtual ~Instrument() = default;
    virtual double value() const { return 0.0; }
};

struct Bond : Instrument {
    double price;
    explicit Bond(double p) : price(p) {}
    double value() const override { return price; }
};

int main() {
    std::vector<Instrument> book;
    book.push_back(Bond(99.5));
    Bond b(101.0);
    Instrument& ref = b;
    Instrument copy = b;
    std::printf("%.1f %.1f %.1f\n", book[0].value(), ref.value(), copy.value());
}
```

***Listing 9.6.** A book of instruments. code/interviews-3/09-reading-debugging-and-software-design/cpp/slicing.cpp*

**Answer to Question 9.6.**

It prints `0.0 101.0 0.0`. This is object slicing. Store pointers in the vector, usually `unique_ptr`, and delete the base class’s copy constructor to make the mistake fail to compile.

A vector of `Instrument` holds objects of exactly that type, each with room for an `Instrument` only. `push_back(Bond(99.5))` copies only the `Instrument` part, so the price is gone and the virtual table pointer is `Instrument`’s: `book[0].value()` is 0.0. `copy` is the same thing written out: `Instrument copy = b`. Only `ref` is still a `Bond`, so it gives 101.0.

With the vector of `unique_ptr`, the first number becomes 99.5. Add `Instrument(const Instrument&) = delete;`, together with a defaulted default constructor, and both slicing lines stop compiling. That is the usual rule for a class meant to be used through a base pointer: no public copying. If you do need copies, add a virtual `clone`.

**Follow-up.** *Is there a way to keep the instruments side by side in memory?* Yes, if the set of types is closed: `std::vector<std::variant<Bond, Swap>>` stores each value in place with no slicing and no extra allocation. The cost is that adding a type means changing that line.

**Why it matters.** A bank’s pricing code is full of instrument hierarchies, and a slice gives a plausible number with no error at all.

**Question 9.7 ★ QD, Dev, LLE • crypto, market maker • junior.**

With rustc 1.97 on x86-64, what does [Listing 9.7](#lst-iv3-rd-usize) do under `cargo run`? And under `cargo run --release`? Is either of them unsafe in Rust’s sense? Fix `imbalance` in two ways, depending on what the caller wants.

```rust
fn imbalance(bids: &[u32], asks: &[u32]) -> usize {
    bids.len() - asks.len()
}

fn main() {
    let bids = vec![10, 20];
    let asks = vec![10, 20, 30];
    println!("{}", imbalance(&bids, &asks));
}
```

***Listing 9.7.** An imbalance. code/interviews-3/09-reading-debugging-and-software-design/rust/src/bin/imbalance.rs*

**Answer to Question 9.7.**

Under `cargo run` it panics, and the message is `attempt to subtract with overflow`. With `--release` it prints $2^{64} - 1$ in full. Neither is unsafe in Rust’s sense. Use `abs_diff` for the size of the gap, or a signed result if the sign matters.

`len()` returns `usize`, and $2 - 3$ does not fit. The Rust Reference calls arithmetic overflow a programmer’s error, but it is not undefined behaviour. With debug assertions on, the compiler must insert a check that panics. In a release build the default is to wrap, as the test confirms. The program stays memory safe either way: the wrong number comes out, or the process stops, and no invariant of the language is broken. You can keep the checks in release builds with `overflow-checks = true` in the Cargo profile, and the test confirms that an optimised build then panics too.

The fix depends on the caller. `bids.len().abs_diff(asks.len())` gives 1 here if only the size of the gap matters. `checked_sub` returns `None` if a negative result means “this should not happen”. `a as i64 - b as i64` gives $-1$ if the caller wants the sign.

**Classic wrong answer.** “Rust catches overflow.” Only with the checks on. The same code ships a different behaviour from what you tested, so the release run in the test matters.

**Why it matters.** Crypto and market-making firms using Rust ask this to check that you know what safe does and does not promise.

**Question 9.8 ★ QR, QT, QD, Dev, DS • any • junior.**

What two lines does [Listing 9.8](#lst-iv3-rd-floatkeys) print on CPython? A colleague stores price levels in a dict keyed by float, and fills disappear now and then. What do you tell them?

```python
levels = {}
levels[0.1 + 0.2] = 500
print(0.3 in levels, levels.get(0.3, 0))
print(round(2.675, 2), round(0.5), round(1.5), round(2.5))
```

***Listing 9.8.** Floats as keys, and rounding. code/interviews-3/09-reading-debugging-and-software-design/python/rd_float_keys.py*

**Answer to Question 9.8.**

It prints `False 0`, then `2.67 0 2 2`. Tell your colleague to key the levels by an integer number of ticks.

$0.1 + 0.2$ is `0.30000000000000004` in binary floating point, and the literal `0.3` is a different double. Dict lookup uses exact equality, so the entry exists under one key and the lookup asks for another, and the fill appears to vanish. Two prices that print the same can differ in the last bit if they came by different arithmetic.

The second line shows two more traps in `round`. `2.675` is stored slightly below that value, as `2.67499999…`, so it rounds down, and the Python documentation says so itself. Halves go to the even neighbour: `round(0.5)` is 0, `round(1.5)` and `round(2.5)` are both 2. Rounding to the tick’s decimals does give one canonical double, so `round(0.1 + 0.2, 2) == 0.3`, but it works only while the error stays far below half a tick, and each writer must remember to do it. Integer ticks need no such care.

They solve it completely: at a tick of 0.001, the keys 100 and 200 add to exactly 300. `Decimal("0.1") + Decimal("0.2")` also equals `Decimal("0.3")`, which suits reports and money, at a cost in speed.

**Classic wrong answer.** “Compare with a tolerance.” You cannot look up a dict with a tolerance, and tolerance is not transitive, so you cannot build a consistent key from it either.

**Why it matters.** Prices as floats are the most common design fault in candidates’ order books, and many firms check for it in the first minute.

**Question 9.9 ★ QD, Dev • bank • junior.**

On OpenJDK 11 with default settings, what does [Listing 9.9](#lst-iv3-rd-boxes) print? Which of those results does the Java language promise, and which belong to this JVM?

```java
public class Boxes {
    public static void main(String[] args) {
        Integer a = 127, b = 127;
        Integer c = 128, d = 128;
        String s = "AAA", t = "AAA", u = new String("AAA");
        System.out.println((a == b) + " " + (c == d) + " " + c.equals(d));
        System.out.println((s == t) + " " + (s == u) + " " + s.equals(u));
    }
}
```

***Listing 9.9.** Comparing boxes and strings. code/interviews-3/09-reading-debugging-and-software-design/java/Boxes.java*

**Answer to Question 9.9.**

It prints `true false true` and `true false true`. The language promises every result here except `c == d`: only that false belongs to this JVM.

`==` on references asks whether they are the same object. The Java Language Specification says that boxing an integer between $-128$ and 127 twice gives the same object, so `a == b` is always true. Beyond that range nothing is promised either way. The specification adds that less memory-limited implementations might cache more, up to $\pm$32K for `int`. OpenJDK 11 with default settings gives two objects for 128, so `c == d` is false.

For strings, the specification says literals are interned, so `s == t` is true. `new String("AAA")` is a new object by definition, so `s == u` is false, while `equals` compares contents.

The rule is short: use `equals` on objects, and `==` only on primitives. This bug is nasty because it passes every test with small quantities.

**Classic wrong answer.** “`==` on `Integer` compares values.” It would, if one side were a primitive `int`, because the other side is then unboxed. With two `Integer` variables it compares references.

**Why it matters.** Bank systems are full of boxed values in collections. A check like `qty == limit` works in every small test case and breaks at 128.

**Question 9.10 ★★ QD, Dev, LLE • any • junior, experienced.**

[Listing 9.10](#lst-iv3-rd-dtor) compiles without a warning under g++ 11.4, `-std=c++20 -O2 -Wall -Wextra -Werror`. What does it print? Is the program correct? What would AddressSanitizer say? Would the answer change with `std::make_shared`?

```cpp
struct Order {
    virtual long qty() const { return 0; }
    ~Order() { std::puts("~Order"); }
};

struct LimitOrder : Order {
    std::string tag = "a tag too long for the small-string buffer";
    long qty() const override { return 100; }
    ~LimitOrder() { std::puts("~LimitOrder"); }
};

int main() {
    std::unique_ptr<Order> p = std::make_unique<LimitOrder>();
    std::printf("%ld\n", p->qty());
}
```

***Listing 9.10.** An order behind a base pointer. code/interviews-3/09-reading-debugging-and-software-design/cpp/no_virtual_dtor.cpp*

**Answer to Question 9.10.**

It prints `100`, then `~Order`, and `~LimitOrder` never runs. The program is undefined: deleting a `LimitOrder` through an `Order*` needs a virtual destructor. AddressSanitizer stops it. With `std::make_shared<LimitOrder>` it is defined, and both destructors run.

The standard says that in a single-object delete, if the static type differs from the dynamic type, the static type must have a virtual destructor, or the behaviour is undefined. `qty` is virtual, so the class is polymorphic, but the destructor is not. In this run, `tag` is never destroyed and its heap buffer is lost.

ASan reports `new-delete-type-mismatch`: the allocated type has 40 bytes and the deallocated one 8. With that check turned off, it reports the leak of the string instead. The test shows both.

Why was there no warning? g++ has `-Wdelete-non-virtual-dtor`, which `-Wall` turns on, but the `delete` expression here is inside `unique_ptr`, in a system header, where warnings are suppressed. Write `delete p` on a raw pointer and the compile fails under the same flags, as the test confirms. So do not trust the silence.

The fix is one word: `virtual ~Order()`. It then prints `~LimitOrder`, then `~Order`, and ASan is silent.

**Follow-up.** *Why does `std::make_shared<LimitOrder>` work without the virtual destructor?* The shared pointer’s control block records how to destroy the object it was created with, the real `LimitOrder`, so the deleter does not go through `Order`. The test prints both destructor messages, and ASan is silent. It is guaranteed, but only for this pointer type: add the virtual destructor anyway, since the next person uses `unique_ptr`.

**Why it matters.** This is the classic C++ interview question, and the twist is that the usual smart pointer hides the warning.

**Question 9.11 ★★ QD, Dev, LLE • any • junior, experienced.**

[Listing 9.11](#lst-iv3-rd-exleak) prints the expected total, and it has run for months. What is wrong with it? How much does it lose, and on which path? Which tool shows it, and what is the fix? Your colleague offers a `catch`, a `delete` and a rethrow inside `handle`. Do you accept that?

```cpp
struct Order { long qty; std::string tag; };

void validate(const Order& o) {
    if (o.qty <= 0) throw std::invalid_argument("bad qty");
}

long handle(long qty) {
    Order* o = new Order{qty, "tag"};
    validate(*o);
    const long q = o->qty;
    delete o;
    return q;
}

int main() {
    long total = 0;
    for (long q : {100L, -1L, 200L}) {
        try {
            total += handle(q);
        } catch (const std::exception& e) {
            std::puts(e.what());
        }
    }
    std::printf("%ld\n", total);
}
```

***Listing 9.11.** Handling an order. code/interviews-3/09-reading-debugging-and-software-design/cpp/exception_leak.cpp*

**Answer to Question 9.11.**

It leaks one `Order` every time `validate` throws. Here that is one object of 40 bytes, plus nothing else, because the short tag fits in the string’s own buffer. LeakSanitizer finds it. Use `std::make_unique`. Do not accept the `catch`.

The path is the `-1` order. `new` succeeds, `validate` throws, the stack unwinds, and the raw pointer `o` is destroyed without touching what it points to. So `delete` never runs. The total is still right, 300, because the leak has nothing to do with the arithmetic, which is why it could run for months: a few bytes per rejected order.

ASan’s leak check reports `Direct leak of 40 byte(s) in 1 object(s)`, allocated in `handle`. With `auto o = std::make_unique<Order>(…)` and no `delete`, the destructor runs during unwinding. The test confirms it: same output, nothing in the report.

The colleague’s version does work for one pointer. But it duplicates the cleanup on two paths. It gets worse with each new resource, because a second allocation needs nested handlers. And the next person to add an early `return` forgets it. This is why the rule is that every resource has an owning object whose destructor releases it. Here, in fact, you need no heap at all: `const Order o{qty, "tag"};` is the simplest correct code.

**Classic wrong answer.** “It is only 40 bytes.” It is 40 bytes each time, and it grows with the rate of rejects, which spike exactly when the market or a client goes wrong.

**Why it matters.** Reading for the path that throws, not the path that runs, is the habit this question tests.

**Question 9.12 ★★ QD, Dev, LLE • any • junior, experienced.**

[Listing 9.12](#lst-iv3-rd-temprange) is compiled as C++20 with g++ 11.4. Is it defined? Now make `levels_` public and loop over `latest().levels_`. Is that defined? What did C++23 change? Give a fix for C++20 and a way to make the compiler reject the mistake.

```cpp
class Snapshot {
public:
    Snapshot() : levels_{100, 200, 300} {}
    const std::vector<long>& levels() const { return levels_; }

private:
    std::vector<long> levels_;
};

Snapshot latest() { return Snapshot{}; }

int main() {
    long total = 0;
    for (long q : latest().levels()) total += q;
    std::printf("%ld\n", total);
}
```

***Listing 9.12.** Looping over the latest snapshot. code/interviews-3/09-reading-debugging-and-software-design/cpp/temp_range.cpp*

**Answer to Question 9.12.**

The listing is undefined in C++20, and ASan reports `stack-use-after-scope`. The version with the public member is defined and gives 600. C++23 makes the first version defined too.

A range-for is defined as `auto&& range = *initializer*;` followed by a loop. Binding a reference to a temporary extends the life of that temporary, but only if the reference binds to it, or to a subobject of it. Here it binds to whatever `levels()` returns. That function returns a reference, and the compiler cannot see that it points into the temporary `Snapshot`. So the `Snapshot` dies at the end of the initialiser, before the loop starts, and the loop walks a destroyed vector.

`latest().levels_` names a subobject of the temporary directly, so the extension applies to the whole `Snapshot`. The test runs it clean under ASan. So the same-looking line is safe through a data member and unsafe through a getter.

C++23 added a rule to `[class.temporary]`: a temporary created in a range-for’s initialiser lives as long as the range reference. g++ 11.4 in C++20 mode does not do this, as the ASan report shows.

For C++20, keep the object in a named variable. Write `const Snapshot snap = latest();` first, then loop over `snap.levels()`. To make the compiler reject it, add reference qualifiers: `levels() const&` plus a deleted `levels() const&&`. The test shows the listing then fails to compile.

**Classic wrong answer.** “Temporaries live to the end of the statement, and the loop is one statement.” The temporary lives to the end of the full expression that initialises the range, which ends before the body runs.

**Why it matters.** A getter on a function’s return value inside a loop is a shape every C++ code base has. It often seems to work, because the freed memory is still there.

**Question 9.13 ★★ QD, Dev, LLE • any • junior, experienced.**

Two threads run [Listing 9.13](#lst-iv3-rd-guard). It compiles with g++ 11.4 under `-Wall -Wextra -Werror`, and the author expects 200000. What is wrong? Show the fix. What happens if you write the same line with parentheses instead of braces?

```cpp
std::mutex m;
long total = 0;

void add(long n) {
    std::lock_guard<std::mutex>{m};
    total += n;
}

int main() {
    auto work = [] { for (int i = 0; i < 100000; ++i) add(1); };
    std::thread a(work), b(work);
    a.join();
    b.join();
    std::printf("%ld\n", total);
}
```

***Listing 9.13.** A counter behind a mutex. code/interviews-3/09-reading-debugging-and-software-design/cpp/guard_temp.cpp*

**Answer to Question 9.13.**

The line `std::lock_guard<std::mutex>{m};` creates a temporary, which locks and unlocks at the semicolon. So `total += n` runs unprotected and there is a data race. Give the guard a name: `std::lock_guard<std::mutex> lock{m};`.

The object has no name, so its life ends with its full expression, before the next line. The compiler sees a legal expression with a constructor and destructor that have side effects, and gives no warning, even under `-Wall -Wextra -Werror`. ThreadSanitizer reports `data race` at once on `total`. Without it, the total may or may not come out right, depending on how the threads interleave. With the name added, TSan is silent and the program prints 200000.

With parentheses, `std::lock_guard<std::mutex>(m);` parses as a declaration of a variable called `m` of type `lock_guard`, with no initialiser. The extra parentheses around a declarator are allowed. Since `lock_guard` has no default constructor, it does not compile here, by luck: the test confirms it. For a type with a default constructor it would compile and hide the global `m`.

**Follow-up.** *Does `std::scoped_lock` make this safer?* Not by itself. `std::scoped_lock lock;` with no mutex at all compiles, since the template takes any number of mutexes including none, and it locks nothing. TSan still reports the race on that version. Name the guard, and name the mutex in it.

**Why it matters.** This is a one-token bug that no compiler warning catches, and spotting a nameless guard is a fair test of having really read lock code.

**Question 9.14 ★★ QD, Dev, LLE • any • junior, experienced.**

[Listing 9.14](#lst-iv3-rd-lockorder) prints `1000 1000` every time, because the threads never overlap. What is the bug in `transfer`? Would ThreadSanitizer report it on this run? Fix it. Then: what does your fix do on a transfer from an account to itself?

```cpp
struct Account { std::mutex m; long balance = 1000; };

void transfer(Account& from, Account& to, long n) {
    std::lock_guard a(from.m);
    std::lock_guard b(to.m);
    from.balance -= n;
    to.balance += n;
}

int main() {
    Account x, y;
    std::thread t1([&] { transfer(x, y, 1); });
    t1.join();
    std::thread t2([&] { transfer(y, x, 1); });
    t2.join();
    std::printf("%ld %ld\n", x.balance, y.balance);
}
```

***Listing 9.14.** A transfer between accounts. code/interviews-3/09-reading-debugging-and-software-design/cpp/lock_order.cpp*

**Answer to Question 9.14.**

`transfer(x, y)` locks `x` then `y`, and `transfer(y, x)` locks `y` then `x`. If two such calls overlap, each can hold one mutex and wait for the other for ever. TSan does report it on this run, as `lock-order-inversion`. Use `std::scoped_lock lock(from.m, to.m);`.

This run cannot deadlock, because the first thread is joined before the second starts. That is the point of the question: you cannot catch this with a test that happens to pass. TSan does not need the deadlock to occur. It builds a graph of which mutex was held when another was acquired, and reports a cycle. Here it prints `Cycle in lock order graph`, on a run that printed the right answer.

`std::scoped_lock` with two mutexes uses `std::lock`, which the standard says does not result in deadlock. The usual technique is to lock one and try the others, backing off on failure. Another fix is a fixed global order, say by address or by account id. That also lets you see the rule in the code, and it costs no retries.

The self-transfer is the trap. With `from` and `to` the same account, `scoped_lock` tries to lock one mutex twice, and a `std::mutex` locked twice by one thread is undefined. In the test it hung until the timeout. So add a guard at the top: `if (&from == &to) return;`, or the equivalent rule for your domain.

**Classic wrong answer.** “Always lock in the order of the arguments.” That is the bug. The order must be a property of the mutexes, not of the call.

**Why it matters.** Two locks taken in opposite orders on different code paths is the classic deadlock. A good answer mentions the tool, the fix, and the edge case the fix creates.

**Question 9.15 ★★ QD, Dev, LLE • any • junior, experienced.**

Find the bug in `pop` in [Listing 9.15](#lst-iv3-rd-cvif) and say exactly how it goes wrong with these two consumers. How would you make the failure loud? Fix it. Would changing `notify_all` to `notify_one` also fix it?

```cpp
std::mutex m;
std::condition_variable cv;
std::deque<int> q;

int pop() {
    std::unique_lock lock(m);
    if (q.empty()) cv.wait(lock);
    const int v = q.front();
    q.pop_front();
    return v;
}

void push(int v) {
    {
        std::lock_guard lock(m);
        q.push_back(v);
    }
    cv.notify_all();
}

int main() {
    std::thread a([] { std::printf("a %d\n", pop()); });
    std::thread b([] { std::printf("b %d\n", pop()); });
    std::this_thread::sleep_for(std::chrono::milliseconds(200));
    push(7);
    std::this_thread::sleep_for(std::chrono::milliseconds(200));
    push(8);
    a.join();
    b.join();
}
```

***Listing 9.15.** A queue with two consumers. code/interviews-3/09-reading-debugging-and-software-design/cpp/cv_if.cpp*

**Answer to Question 9.15.**

The bug is the `if`: after `wait` returns, nothing checks that the queue still has an item. Use the predicate form, `cv.wait(lock, [] { return !q.empty(); });`. `notify_one` alone would not fix it.

Here is how it fails. Both consumers wait. `push(7)` calls `notify_all`, so both wake and compete for the mutex. The first takes 7. The second then gets the mutex, returns from `wait`, and goes straight to `q.front()` on an empty deque. That call is undefined. With no checks on, nothing stops the read, nor the `pop_front` after it, and this may pass unnoticed for a long while.

To make it loud, build with `-D_GLIBCXX_ASSERTIONS`, which turns on the library’s precondition checks. The test run aborts with SIGABRT at `front`. With the predicate the same build prints both values, and TSan is silent.

`notify_one` wakes one thread, which looks enough. It still leaves two ways to fail. First, `wait` is allowed to return with no notify at all: the standard’s wording includes “or spuriously”. Second, between the notify and the woken thread getting the mutex, a third thread calling `pop` can take the item, because it finds the queue non-empty and does not wait.

The rule is short: a condition variable never tells you the condition is true, only that it is worth checking again. So always wait in a loop or use the predicate form, which is that loop.

**Classic wrong answer.** “`if` is fine because `push` always adds an item before notifying.” That is true, but the item may be gone by the time this thread holds the lock.

**Why it matters.** This appears in nearly every queue that candidates write at a whiteboard. Interviewers look for the loop, and then ask why.

**Question 9.16 ★★ QD, Dev, LLE • any • junior, experienced.**

What does [Listing 9.16](#lst-iv3-rd-constmove) print? Its author was sure there would be no copies. Why is there one? Then say why a move constructor without `noexcept` can also bring copies back, inside `std::vector`.

```cpp
struct Tracked {
    inline static int copies = 0, moves = 0;
    std::string text;
    explicit Tracked(std::string t) : text(std::move(t)) {}
    Tracked(const Tracked& o) : text(o.text) { ++copies; }
    Tracked(Tracked&& o) noexcept : text(std::move(o.text)) { ++moves; }
};

class Holder {
public:
    explicit Holder(const Tracked item) : item_(std::move(item)) {}

private:
    Tracked item_;
};

int main() {
    Tracked t("abc");
    Holder h(std::move(t));
    std::printf("copies %d, moves %d\n", Tracked::copies, Tracked::moves);
}
```

***Listing 9.16.** Moving into a member. code/interviews-3/09-reading-debugging-and-software-design/cpp/const_move.cpp*

**Answer to Question 9.16.**

It prints `copies 1, moves 1`. The parameter is `const Tracked`, and `std::move` of a const object gives a const rvalue, which binds to the copy constructor. Drop the `const` and it prints `copies 0, moves 2`.

`std::move` moves nothing. It is a cast to an rvalue reference, and it keeps `const`. Overload resolution then has two candidates: `Tracked(Tracked&&)` needs a non-const argument, so it does not fit, and `Tracked(const Tracked&)` does. So the compiler quietly copies, with no warning. The one move is the outer one, from `t` into the parameter. The one copy is from the parameter into `item_`.

The `const` on a by-value parameter looks harmless and even virtuous, since it does not change the function’s signature. For a parameter you intend to move from, it is exactly wrong. The same goes for a `const` local variable that you later `std::move`.

In `std::vector`, growth has to give the strong exception guarantee: if something throws while relocating, the old buffer must still be intact. A move that throws would have destroyed the old elements. So the vector uses `std::move_if_noexcept`: it moves only if the move constructor is `noexcept`, or if the type cannot be copied. The test shows it: two `emplace_back` calls print `copies 0, moves 1` with `noexcept`, and `copies 1, moves 0` without it.

**Classic wrong answer.** “`std::move` forces a move.” It only makes one possible. Counting with a tracked type, as here, is how you check.

**Why it matters.** Unnoticed copies of large messages in a hot path come from exactly these two lines. They give no compile error and no test failure, only a slower program.

**Question 9.17 ★★ QD, Dev, LLE • market maker, hedge fund • junior, experienced.**

[Listing 9.17](#lst-iv3-rd-callback) prints `1` then `done` when built normally. Is it correct? What would AddressSanitizer report, and at which line? Design the fix. What does it need from the class, so that no one can break it by accident?

```cpp
class Feed {
public:
    void subscribe(std::function<void(long)> f) { subs_.push_back(std::move(f)); }
    void publish(long v) { for (auto& f : subs_) f(v); }

private:
    std::vector<std::function<void(long)>> subs_;
};

class Strategy {
public:
    explicit Strategy(Feed& feed) { feed.subscribe([this](long v) { total_ += v; }); }
    long total() const { return total_; }

private:
    long total_ = 0;
};

int main() {
    Feed feed;
    auto s = std::make_unique<Strategy>(feed);
    feed.publish(1);
    std::printf("%ld\n", s->total());
    s.reset();
    feed.publish(2);
    std::puts("done");
}
```

***Listing 9.17.** A strategy subscribes to a feed. code/interviews-3/09-reading-debugging-and-software-design/cpp/callback_this.cpp*

**Answer to Question 9.17.**

It is not correct: the second `publish` calls a lambda holding a dangling `this`, and it writes to freed memory. ASan reports `heap-use-after-free` at `total_ += v`. The fix gives the subscription an owner, so the destructor unsubscribes.

The feed stores the `std::function`, which stores the raw `this`. The feed knows nothing about the strategy’s lifetime. After `s.reset()`, the strategy is gone, but its lambda is still in `subs_`. Built normally, the write lands on freed memory and nothing shows, so you see `1`, `done`. In production the same write lands in whatever was allocated there next, and the symptom appears somewhere else entirely.

The fix in [Listing 9.41](#lst-iv3-rd-callbackfixed) makes `subscribe` return a handle, stored in the strategy, and the destructor gives it back. A list keeps iterators valid while other subscribers come and go. It runs clean under ASan.

There is a second bug to prevent. The default copy constructor would copy `slot_` but leave the lambda pointing at the original object, and both destructors would erase the same slot. So the class must delete copying, as here. The test shows that copying then does not compile.

Another common design uses `std::shared_ptr` for the strategy and captures a `std::weak_ptr` in the lambda. The feed locks it on every call, and an expired one is skipped. That is simpler with several threads, and it costs an atomic operation per call.

```cpp
class Feed {
public:
    using Slot = std::list<std::function<void(long)>>::iterator;
    Slot subscribe(std::function<void(long)> f) {
        subs_.push_back(std::move(f));
        return std::prev(subs_.end());
    }
    void unsubscribe(Slot s) { subs_.erase(s); }
    void publish(long v) { for (auto& f : subs_) f(v); }

private:
    std::list<std::function<void(long)>> subs_;
};

class Strategy {
public:
    explicit Strategy(Feed& feed)
        : feed_(feed), slot_(feed.subscribe([this](long v) { total_ += v; })) {}
    ~Strategy() { feed_.unsubscribe(slot_); }
    Strategy(const Strategy&) = delete;             // [this] is inside the feed: never copy
    Strategy& operator=(const Strategy&) = delete;
    long total() const { return total_; }

private:
    Feed& feed_;
    Feed::Slot slot_;
    long total_ = 0;
};

int main() {
    Feed feed;
    auto s = std::make_unique<Strategy>(feed);
    feed.publish(1);
    std::printf("%ld\n", s->total());
    s.reset();
    feed.publish(2);
    std::puts("done");
}
```

***Listing 9.41.** The subscription has an owner. code/interviews-3/09-reading-debugging-and-software-design/cpp/callback_fixed.cpp*

**Follow-up.** *What if a callback unsubscribes itself during `publish`?* This version then erases the element the range-for is standing on, which is undefined. The usual answer is to mark it dead and remove it after the loop, or to loop over a copy.

**Why it matters.** Callbacks are how feeds reach strategies. Getting the order of destruction right is daily work in a trading system.

**Question 9.18 ★★ QD, Dev, DS • any • junior, experienced.**

What does the last line of [Listing 9.18](#lst-iv3-rd-hash) print on CPython, and why? What should this class hash on? What happens if you simply delete its `__hash__`?

```python
class Order:
    def __init__(self, oid, qty):
        self.oid, self.qty = oid, qty

    def __eq__(self, other):
        return (self.oid, self.qty) == (other.oid, other.qty)

    def __hash__(self):
        return hash((self.oid, self.qty))


book = set()
o = Order(1, 100)
book.add(o)
o.qty = 50                # a partial fill
print(o in book, len(book), any(x is o for x in book))
```

***Listing 9.18.** Orders in a set. code/interviews-3/09-reading-debugging-and-software-design/python/rd_mutable_hash.py*

**Answer to Question 9.18.**

It prints `False 1 True`: the order is in the set, and `in` cannot find it. Hash on `oid` alone, the part that never changes, and have `__eq__` agree. If you delete `__hash__`, `book.add(o)` raises a `TypeError`.

A set stores each item in a slot chosen from its hash at the time of insertion. Changing `qty` changes the hash that `__hash__` now computes, so the lookup looks in a different slot. It finds nothing, and it never compares. The item is still there, as the last two values show: `len` is 1, and a scan by identity finds it. You now have an order that you can neither find nor remove.

The Python data model warns against this directly: a class that defines mutable objects and implements `__eq__` should not implement `__hash__`, since a key’s hash value must be immutable. The language enforces half of this. A class that defines `__eq__` without `__hash__` has its hash set to `None`, so instances are unhashable. The test shows that error.

With `hash(self.oid)` and equality on `oid`, the line prints `True 1 True`. But note what that changes: two orders with the same id are now equal whatever their quantity. That is what you want for identity in a book. It is not what you want if you are comparing two snapshots. A cleaner design separates them: a dict from id to order keeps the identity in the key and leaves the order free to change.

**Classic wrong answer.** “Sets compare with `==`, so it will find it.” They compare only within a slot chosen by the hash, so the comparison never happens.

**Why it matters.** Hashing a mutable field is an easy mistake with a nasty symptom: items that are there and yet are not.

**Question 9.19 ★★ QR, QD, Dev, DS • any • junior, experienced.**

[Listing 9.19](#lst-iv3-rd-cache) is a pricer that caches its results. What do the two lines print? A long-running process creates one `Pricer` per curve, and its memory keeps growing. Why? Give a fix that keeps the cache.

```python
import gc
import weakref
from functools import lru_cache


class Pricer:
    def __init__(self, curve):
        self.curve = curve

    @lru_cache(maxsize=None)
    def price(self, strike):
        return self.curve[0] * strike


p = Pricer([1.5] * 1_000_000)
print(p.price(100))
ref = weakref.ref(p)
del p
gc.collect()
print(ref() is None)
```

***Listing 9.19.** A cached pricer. code/interviews-3/09-reading-debugging-and-software-design/python/rd_method_cache.py*

**Answer to Question 9.19.**

It prints `150.0` and `False`: after `del p` and a full collection, the pricer is still alive. The cache keeps it alive, along with its million-element curve. Give each instance its own cache.

`@lru_cache` on a method wraps the function once, at class level. Its key contains all the arguments, and the first of them is `self`. So the class-level cache holds a strong reference to every instance that ever called it. The Python FAQ says the same of `lru_cache` on methods: “it creates a reference to the instance”. With `maxsize=None` it never evicts, so each new `Pricer` and its curve stay until the process ends. That is the memory growth. Linters flag this pattern too: ruff’s rule B019 reports this very line.

To keep the caching, create the cache in the constructor:

`self.price = lru_cache(maxsize=None)(self._price)`

The cache then belongs to the instance. There is now a reference cycle (instance to cache to bound method to instance), so the object is freed by the cycle collector rather than at once. The test prints `True` after `gc.collect()`. Bounding `maxsize` is sensible too: with the class-level form it limits the damage but does not cure it, because a few big instances can stay pinned.

**Follow-up.** *Is there another bug waiting in this class?* Yes. The cache key does not include `self.curve`’s contents. If someone updates the curve in place, the cache returns stale prices. A cache should be keyed on everything the result depends on, or the input should be immutable.

**Why it matters.** Caching in a pricer is natural, and this is the commonest real memory leak in Python research and pricing services.

**Question 9.20 ★★ QR, QT, QD, Dev, DS • any • junior, experienced.**

[Listing 9.20](#lst-iv3-rd-dst) measures one New York day on Python 3.10. What do the two lines print? Which one is the length of that day? Name a calculation at a trading firm that would silently go wrong with the first.

```python
from datetime import datetime, timezone
from zoneinfo import ZoneInfo

ny = ZoneInfo("America/New_York")
start = datetime(2026, 3, 8, 0, 0, tzinfo=ny)
end = datetime(2026, 3, 9, 0, 0, tzinfo=ny)
print(end - start)
print(end.astimezone(timezone.utc) - start.astimezone(timezone.utc))
```

***Listing 9.20.** The length of a day. code/interviews-3/09-reading-debugging-and-software-design/python/rd_dst.py*

**Answer to Question 9.20.**

It prints `1 day, 0:00:00`, then `23:00:00`. The second is true: on 8 March 2026 New York moved its clocks forward, so that day had 23 hours.

The Python documentation states the rule: if both datetimes are aware and have the same `tzinfo` attribute, the `tzinfo` attributes are ignored and no time zone adjustment is done. Here both use the same `ny` object, so the subtraction works on wall-clock fields, midnight to midnight, and gives one day. Converted to UTC, the two carry different offsets: the test shows $-5$ hours just before the change and $-4$ after. The subtraction then gives the elapsed time. The same trap runs the other way on 1 November 2026, a day of 25 hours.

Anything that divides by elapsed time goes wrong silently with the first form. Volume per hour, or a time-weighted average spread for that day, is off by about 4%, as 24 hours is used for a 23-hour day. A timeout or a session schedule computed by adding hours to a local time can be off by an hour. An annualisation or a time-to-expiry computed on local timestamps can pick up the same error.

The rule at trading firms: store and compute in UTC, or in integer nanoseconds since the epoch, and convert to local time only to display, or to apply exchange session rules, which are themselves defined in local time.

**Classic wrong answer.** “`zoneinfo` handles daylight saving.” It does give the right offsets, but the subtraction ignores them when both sides have the same `tzinfo`, as here.

**Why it matters.** Two days a year, this is an hour of wrong data, an hour early or late at the open. It usually surfaces first in production, because few tests use those dates.

**Question 9.21 ★★ QD, Dev, LLE • crypto, market maker • junior, experienced.**

What does [Listing 9.21](#lst-iv3-rd-rccycle) print, with rustc 1.97? Which messages do you never see, and why? Is that a bug by Rust’s own rules? Fix it with one change of type, and say what the fixed version prints.

```rust
use std::cell::RefCell;
use std::rc::Rc;

struct Node {
    name: &'static str,
    next: RefCell<Option<Rc<Node>>>,
}

impl Drop for Node {
    fn drop(&mut self) {
        println!("drop {}", self.name);
    }
}

fn main() {
    let a = Rc::new(Node { name: "a", next: RefCell::new(None) });
    let b = Rc::new(Node { name: "b", next: RefCell::new(Some(a.clone())) });
    *a.next.borrow_mut() = Some(b.clone());
    println!("{} {}", Rc::strong_count(&a), Rc::strong_count(&b));
}
```

***Listing 9.21.** Two nodes. code/interviews-3/09-reading-debugging-and-software-design/rust/src/bin/rc_cycle.rs*

**Answer to Question 9.21.**

It prints `2 2` and nothing more. You never see `drop a` or `drop b`, because the two nodes own each other and neither count reaches zero. By Rust’s rules that is not a safety bug. Change `Rc` to `Weak` in the `next` field.

Each node starts with one strong reference from its local variable, and the other node adds one, hence `2 2`. At the end of `main`, the two local variables go away and each count drops to 1. Nothing further can drop them, since each is kept alive by the other. So the memory is never freed and the destructors never run.

The standard library says so plainly: a cycle between `Rc` pointers will never be deallocated, and `Weak` is there to break cycles. The Rust Reference lists leaks of memory under behaviour that is not considered unsafe, along with deadlocks. So never rely on `Drop` to release a limit or close a position.

With `RefCell<Weak<Node>>` and `Rc::downgrade`, the fixed version prints `1 1`, then `a` (the upgrade from `b` still succeeds), then `drop b`, `drop a`. A `Weak` does not keep the value alive, so `upgrade` returns an `Option`, and you must handle `None`.

**Classic wrong answer.** “Rust prevents memory leaks.” It prevents use-after-free and data races in safe code. A leak is memory safe.

**Follow-up.** *Which pointer should be weak in a tree?* The one from child to parent. A parent owns its children, and a child only needs to reach its parent while the parent exists.

**Why it matters.** Graph-shaped state, such as orders pointing at their levels and levels at orders, tempts people into cycles in Rust as in any language with reference counts.

**Question 9.22 ★★ QD, Dev, LLE • crypto, market maker • experienced.**

[Listing 9.22](#lst-iv3-rd-guardmatch) compiles with no warning under rustc 1.97. What happens when you run it, and why? Does the 2024 edition change this? Fix it with one extra line.

```rust
use std::sync::Mutex;

fn main() {
    let queue = Mutex::new(vec![1, 2, 3]);
    match queue.lock().unwrap().pop() {
        Some(x) => {
            queue.lock().unwrap().push(x * 2);
            println!("requeued {x}");
        }
        None => println!("empty"),
    }
    println!("done");
}
```

***Listing 9.22.** Requeueing an item. code/interviews-3/09-reading-debugging-and-software-design/rust/src/bin/guard_match.rs*

**Answer to Question 9.22.**

On rustc 1.97 it hangs for ever. The guard from the first `lock()` lives until the end of the `match`, and the arm tries to lock the same mutex again. The 2024 edition does not change this. Add a `let` first: `let item = queue.lock().unwrap().pop();`, then `match item {`.

`queue.lock().unwrap()` creates a temporary `MutexGuard`, and `pop()` borrows from it only briefly. You might expect the guard to go once `pop` returns. But the Rust Reference notes that the scrutinee of a `match` is not a temporary scope, so temporaries in the scrutinee can be dropped after the `match` expression. The guard is still alive through the arms. The second `lock()` on the same thread then waits for a lock that it holds itself. The documentation of `Mutex::lock` says this call might panic. Here it hung, and the test sees the timeout, so do not rely on the panic.

The 2024 edition changes two scopes, and neither is this one. Temporaries in an `if let` scrutinee now drop before the `else` branch, but not before the body, and tail expression temporaries drop earlier. The test builds this `match` and the `if let` form under edition 2024, and both still hang, since the second lock is in the body.

With the `let`, the statement ends at its semicolon, the guard drops, and the program prints `requeued 3`, then `done`.

**Classic wrong answer.** “The borrow checker prevents deadlocks.” It prevents data races. The Rust Reference lists deadlocks as not considered unsafe.

**Why it matters.** Nothing in the text shows the guard, let alone how long it lives. Reading for that scope is the skill, and it is the one that finds this deadlock before production does.

**Question 9.23 ★★ QD, Dev • bank • junior, experienced.**

On OpenJDK 11, what does [Listing 9.23](#lst-iv3-rd-equalsonly) print? Why is the second line surprising given the first? State the rule that was broken and the fix. Then give another way to break a key, even with both methods written.

```java
import java.util.HashMap;
import java.util.Map;

public class EqualsOnly {
    static final class Key {
        final String symbol;

        Key(String symbol) { this.symbol = symbol; }

        @Override public boolean equals(Object o) {
            return o instanceof Key && ((Key) o).symbol.equals(symbol);
        }
    }

    public static void main(String[] args) {
        Map<Key, Long> limits = new HashMap<>();
        limits.put(new Key("AAA"), 1000L);
        System.out.println(new Key("AAA").equals(new Key("AAA")));
        System.out.println(limits.get(new Key("AAA")));
    }
}
```

***Listing 9.23.** A key class. code/interviews-3/09-reading-debugging-and-software-design/java/EqualsOnly.java*

**Answer to Question 9.23.**

It prints `true`, then `null`. `Key` overrides `equals` but not `hashCode`, so two equal keys have different hash codes, and the map does not find the entry. Add `hashCode`, which returns `symbol.hashCode()`.

The `Object` documentation states the contract: if two objects are equal according to `equals`, calling `hashCode` on each must produce the same integer result, so it is generally necessary to override `hashCode` whenever `equals` is overridden. The default `hashCode` is not based on the fields, so equal keys almost always have different hashes. `HashMap.get` compares the stored hash first, and calls `equals` only if they match, so even a key in the same bucket is not found. So the lookup returns `null` without ever calling your `equals`. With the override the program prints `true` and `1000`.

The compiler says nothing, because both methods are optional. A `null` from `get` also looks like “no limit set”. That is the dangerous part. In a limit check, unboxing the `null` into a `long` gives a `NullPointerException` at best. At worst it reads as “no limit, so allow”.

Another way to break it: keep both methods, but use a mutable field in them, and change the field after inserting. The hash now differs from the stored one, and the same lookup returns `null`. This is why key classes should be immutable. Here `symbol` is `final`, and the class is `final` too, so a subclass cannot change what equality means. The test runs this mutable variant too.

**Classic wrong answer.** “It prints 1000, because the keys are equal.” `HashMap` never reaches the equality check.

**Why it matters.** This is the first question on a Java interview at a bank, because every position and limit map depends on it.

**Question 9.24 ★★ QD, Dev • bank • junior, experienced.**

On OpenJDK 11, [Listing 9.24](#lst-iv3-rd-lostmerge) should print 400000. It does not. The map is a `ConcurrentHashMap`, so what went wrong? Make the fix one line. What would you use if the counter were updated from very many threads and read rarely?

```java
import java.util.concurrent.ConcurrentHashMap;

public class LostMerge {
    static final ConcurrentHashMap<String, Long> position = new ConcurrentHashMap<>();

    static void add(String symbol, long qty) {
        Long old = position.get(symbol);
        position.put(symbol, (old == null ? 0L : old) + qty);
    }

    public static void main(String[] args) throws InterruptedException {
        Thread[] pool = new Thread[4];
        for (int i = 0; i < pool.length; i++) {
            pool[i] = new Thread(() -> {
                for (int j = 0; j < 100000; j++) add("AAA", 1);
            });
            pool[i].start();
        }
        for (Thread t : pool) t.join();
        System.out.println(position.get("AAA"));
    }
}
```

***Listing 9.24.** Adding to a position. code/interviews-3/09-reading-debugging-and-software-design/java/LostMerge.java*

**Answer to Question 9.24.**

It prints a number far below 400000, about 130000 in two runs here with four threads, and it changes from run to run. The map is thread-safe call by call, but `get` followed by `put` is two calls, so updates get lost between them. Replace both lines by `position.merge(symbol, qty, Long::sum);`.

Each operation of `ConcurrentHashMap` is safe, and the map’s internals are never corrupted. But thread A can read 5, thread B can read 5, both compute 6, and both write 6. One increment is gone. This is a check-then-act race, and no choice of collection saves you from it while the logic stays spread over two calls.

`merge` does the read, the computation and the write as one step on that key. The documentation states that the entire method invocation is performed atomically, and that update operations by other threads may be blocked meanwhile. So keep the function short, and do not touch the map from inside it. The test prints exactly 400000 with this line.

`compute` does the same with a more general function, and `putIfAbsent` covers the first-time case. Here, `Long::sum` is all that is needed.

For a counter hit by many threads and read rarely, use `LongAdder`. It keeps one or more variables that grow in number under contention, and adds them up on read. Its Javadoc calls it usually preferable to `AtomicLong` for such sums, at the expense of more space. `map.computeIfAbsent(symbol, s -> new LongAdder()).add(qty)` is the usual form.

**Classic wrong answer.** “Make the map a `Hashtable` or wrap it in `synchronizedMap`.” Those lock each call, so the race between the two calls is still there.

**Why it matters.** Position keeping in a concurrent service is exactly this code. A number that drifts low is one of the worst kinds of bug, since nothing throws.

**Question 9.25 ★★ QD, Dev • bank • junior, experienced.**

[Listing 9.25](#lst-iv3-rd-eventleak) is built in Release mode on .NET 8. The `Strategy` that `Make` creates goes out of scope, and nothing in `Main` refers to it. What does the program print after the collection, and why? Why would many short-lived strategies be a problem here? Give the usual fix.

```csharp
using System;
using System.Runtime.CompilerServices;

class Feed {
    public event Action<long>? Tick;
    public void Publish(long v) => Tick?.Invoke(v);
}

class Strategy {
    long total;
    public Strategy(Feed feed) { feed.Tick += OnTick; }
    void OnTick(long v) => total += v;
    public long Total => total;
}

static class Program {
    [MethodImpl(MethodImplOptions.NoInlining)]
    static WeakReference Make(Feed feed) => new WeakReference(new Strategy(feed));

    static void Main() {
        var feed = new Feed();
        WeakReference w = Make(feed);
        GC.Collect();
        GC.WaitForPendingFinalizers();
        GC.Collect();
        Console.WriteLine(w.IsAlive);
        GC.KeepAlive(feed);
    }
}
```

***Listing 9.25.** A subscriber and a garbage collection. code/interviews-3/09-reading-debugging-and-software-design/csharp/EventLeak/Program.cs*

**Answer to Question 9.25.**

It prints `True`: the strategy survives the collection. The event in `feed` holds a delegate that holds the strategy. Many short-lived strategies would therefore pile up, each still receiving ticks. Unsubscribe with `-=`, normally in `Dispose`.

`feed.Tick += OnTick` creates a delegate whose target is the `Strategy` instance, and adds it to the feed’s list. Garbage collection follows references, so a live `feed` keeps the delegate alive, which keeps the strategy alive. Microsoft’s programming guide says this in nearly those words: as long as the publishing object holds that reference, garbage collection will not delete your subscriber object, so you should unsubscribe before you dispose of the subscriber.

The listing is built with care to show this honestly. `NoInlining` on `Make` and the Release build mean that no local variable keeps the strategy alive by accident. `GC.KeepAlive(feed)` keeps the feed alive through the check. So the only path left is the event.

Short-lived subscribers on a long-lived publisher are the bad shape: an instrument’s view subscribing to a global feed, say, for the length of a screen. Memory grows, and so does CPU, because each dead strategy still runs `OnTick` on every tick, doing work for nothing. That second symptom is often the one you notice first.

With `IDisposable` and `feed.Tick -= OnTick` in `Dispose`, and `using var s` in `Make`, the program prints `False`.

**Classic wrong answer.** “C# has a garbage collector, so it cannot leak.” The collector frees what is unreachable. This is reachable, which is a leak in everything but name.

**Follow-up.** *Does the strategy holding a reference to the feed cause the same problem?* No. That reference runs the other way: a strategy that is otherwise unreachable can be collected along with its reference. Only publisher to subscriber keeps a subscriber alive.

**Why it matters.** Long-running desktop and pricing applications at banks often hit this one.

**Question 9.26 ★★ QR, QD, Dev, DS • any • junior, experienced.**

All three tests in [Listing 9.26](#lst-iv3-rd-weaktests) pass under pytest. How many of them could ever fail? What is wrong with `vwap`, and with each test? Rewrite the tests so they catch the bug.

```python
import pytest


def vwap(trades):
    return sum(price for price, _ in trades) / len(trades)


def test_two_trades():
    assert vwap([(100.0, 50), (102.0, 50)]) == pytest.approx(101.0)


def test_empty():
    try:
        vwap([])
    except ZeroDivisionError:
        pass


def test_one_trade():
    assert (vwap([(100.0, 7)]) == 99.0, "a single trade is its own average")
```

***Listing 9.26.** A function and its tests. code/interviews-3/09-reading-debugging-and-software-design/python/rd_vwap_weak.py*

**Answer to Question 9.26.**

Only the first can ever fail on a wrong number, and it cannot catch the bug. `vwap` ignores the quantities and takes a simple mean. Test with unequal quantities, and use `pytest.raises` and a bare `assert`.

Go through them. `test_two_trades` uses equal quantities, 50 and 50, and for those the simple and weighted means agree: 101 either way. `test_empty` has no `assert` and swallows the exception it hopes for, so it passes if the call raises that and passes if it returns. `test_one_trade` asserts a two-element tuple. A non-empty tuple is always true, so it can never fail, even though it compares against a wrong value, 99. Python itself warns at compile time: `assertion is always true`. The test captures that warning. pytest happily counts `3 passed`.

The right function is the sum of $p \times q$ divided by the sum of $q$. Rewrite:

`assert vwap([(100.0, 10), (102.0, 30)]) == 101.5`

On the current function that gives 101, and it fails. Hand-compute your expected value, and do not paste it from a run.

`with pytest.raises(ValueError):` then `vwap([])`, which fails if nothing raises. Then `assert vwap([(100.0, 7)]) == 100.0`.

Add properties, which catch whole families of bugs: the result lies between the lowest and highest price, and multiplying every quantity by 3 changes nothing. The test runs both on 200 random lists.

To check the tests themselves, break the code on purpose and see that a test fails. This is the idea behind mutation testing, and here it would have exposed all three at once.

**Classic wrong answer.** “Coverage is 100%, so it is tested.” Every line runs. Nothing is checked.

**Why it matters.** Firms read your tests as closely as your code. Tests that cannot fail are worse than none, because they give confidence.

**Question 9.27 ★★ QD, Dev, LLE • market maker, hedge fund • junior, experienced.**

[Listing 9.27](#lst-iv3-rd-swapped) sends 101 lots at a price of 500, and the compiler has nothing to say. Design the types so that this mistake does not compile. A colleague fears the wrappers cost time in the hot path. With g++ 11.4, `-std=c++20 -O2`, on x86-64 Linux, what do you expect in the machine code?

```cpp
void send(long price, long qty) {
    std::printf("%ld at %ld\n", qty, price);
}

int main() {
    long price = 101, qty = 500;
    send(qty, price);
}
```

***Listing 9.27.** An order with two longs. code/interviews-3/09-reading-debugging-and-software-design/cpp/swapped_args.cpp*

**Answer to Question 9.27.**

Give each quantity its own type: `struct Price { long ticks; };` and `struct Qty { long lots; };`. `send(qty, price)` then does not compile. In the machine code, expect no difference at all from plain `long`.

[Listing 9.42](#lst-iv3-rd-strong) is the result. It compiles at the same flags, prints the right thing, and swapping the arguments is now an error: the test confirms it. For the cost, it compares two functions:

`long cost_raw(long, long)` and `long cost_typed(Price, Qty)`.

Under g++ 11.4 at `-O2` on x86-64 Linux, both compile to the same instructions, `movq`, `imulq`, `ret` (plus an `endbr64` on Ubuntu’s default flags). The test compares them line by line. The reason lies in the System V calling convention: a small struct of one integer goes in a register exactly as the integer does. A wrapper has no run-time existence, and all of it happens in the type checker.

Add only the operations that make sense. Price minus price is a price difference, and quantity times price is a notional. Price plus price has no meaning, so leave it out, and the compiler now stops that too. Conversions from `long` should be explicit, or the protection goes away.

There is one hole in this listing. These are aggregates, so `send({500}, {101})` still compiles and still swaps. If that worries you, add an `explicit` constructor.

```cpp
struct Price { long ticks; };
struct Qty { long lots; };

void send(Price price, Qty qty) {
    std::printf("%ld at %ld\n", qty.lots, price.ticks);
}

long cost_raw(long price, long qty) { return price * qty; }
long cost_typed(Price price, Qty qty) { return price.ticks * qty.lots; }

int main() {
    Price price{101};
    Qty qty{500};
    send(price, qty);
    std::printf("%ld %ld\n", cost_raw(101, 500), cost_typed(price, qty));
}
```

***Listing 9.42.** The same order with types. code/interviews-3/09-reading-debugging-and-software-design/cpp/strong_types.cpp*

**Classic wrong answer.** “Comment the parameters, or use named variables.” The listing does name its variables, and that is how it fails.

**Why it matters.** A swapped price and quantity is a fat-finger bug made by a programmer. Type wrappers are one of the very few tools that remove a class of bug at no run-time cost.

**Question 9.28 ★★ QD, Dev, QR • any • junior, experienced.**

A test started failing somewhere in the last 1024 commits. Each run, build included, takes 6 minutes. How many runs does a bisect need, and how long does it take? Now the test is flaky: on a commit with the bug it fails only 1 run in 4, and on a commit without the bug it never fails. How many runs per commit do you need for the whole bisect to find the right commit with probability at least 99%? How long is that, in the worst case?

**Answer to Question 9.28.**

The plain bisect needs 10 runs, $2^{10} = 1024$, so 60 minutes. With the flaky test you need 24 runs per commit, and the worst case is 24 hours.

The `git bisect` command is a binary search: each test halves the range, so 1024 commits take 10 steps. That assumes one good commit and one later bad commit, with one change from good to bad between them.

With the flaky test, a failure is conclusive, since a commit without the bug never fails. A pass is not. A bad commit passes $r$ runs in a row with probability $(3/4)^r$. In that case the search takes a bad commit for a good one, and goes the wrong way for good. On the worst path, all 10 commits tested are bad, and you need them all to be judged correctly:

$$
\left(1 - (3/4)^r\right)^{10} \ge 0.99.
$$

Taking the tenth root, $(3/4)^r \le 1 - 0.99^{1/10} \approx 0.001$, so $r \ge \ln(0.001) / \ln(0.75) \approx 24$. Indeed $r = 24$ gives 0.9900 and $r = 23$ gives 0.987. So each bad commit needs a miss rate of about $1/1000$.

For the time, the worst case is 10 commits at 24 runs each: 240 runs, or 24 hours. Every good commit costs all 24 runs, because it has to pass them all. A bad commit stops at its first failure, after about 4 runs on average. A typical path meets 5 of each, so $5 \times 24 + 5 \times 4 = 140$ runs, or 14 hours. A simulation of 4000 searches in the test finds the right commit more than 99% of the time.

In practice, first make the test less flaky, or run it on many machines at once. With 24 runs in parallel, each step takes 6 minutes and the whole search an hour.

**Classic wrong answer.** “Run it three or four times, since it fails one time in four.” Four runs still miss with probability 32%, and over ten steps the search is more likely to be wrong than right.

**Why it matters.** Finding when a rare fault came in is a routine task at a firm, and the arithmetic decides whether it is an afternoon’s work or a waste of a week.

**Question 9.29 ★★ QD, Dev • bank • junior, experienced.**

You are reviewing [Listing 9.28](#lst-iv3-rd-readonly). A book rebuilt from history must not take new orders, so its author made it a subclass that raises. The code runs. What do you say in the review, and what shape would you ask for instead? Is there a case where you would accept it?

```python
class Book:
    def __init__(self):
        self.orders = {}

    def add(self, oid, qty):
        self.orders[oid] = qty

    def total(self):
        return sum(self.orders.values())


class ReplayBook(Book):
    """A book rebuilt from history. Nothing may be added to it."""

    def add(self, oid, qty):
        raise NotImplementedError("read-only")


def top_up(book, oid, qty):
    book.add(oid, qty)
    return book.total()
```

***Listing 9.28.** A read-only book. code/interviews-3/09-reading-debugging-and-software-design/python/rd_read_only.py*

**Answer to Question 9.29.**

I would ask for a change. `ReplayBook` breaks what `Book` promises, since any code handed a `Book` may call `add`, and here that raises. Split the interface: a read-only view with `total`, and a mutable `Book` built on top of it.

The test for inheritance is whether the subclass can stand in everywhere the base is used. This is the Liskov substitution principle, and `top_up` shows the failure: with a `Book` it returns a total, and with a `ReplayBook` it raises. Nothing in the types warns the author of `top_up`, so the failure shows up at run time, probably in the path nobody tests, and only for replayed books.

The direction of the hierarchy is wrong too. Read-only is the smaller interface, so it should be the base. `ReadableBook` has `total`; `Book` adds `add`. `ReplayBook` is then a `ReadableBook`, and `top_up` declares that it needs a `Book`. With a static checker or a C++ compiler, handing it a replayed book is an error at the point of the mistake. In Python you would state this with `typing.Protocol` and let a type checker enforce it.

Often you should go further and drop inheritance. Replay needs a loader that fills a book and then hands out a read-only view. Composition gives you that with no new class of books at all, and the same `Book` implementation stays the only one to test.

There is a case for accepting the subclass: a short-lived script, one caller, under a deadline. Say why, and leave a note. I would also accept it if the base class’s contract already declares the operation optional, and callers are written to expect that. But then that contract is the design to question.

**Classic wrong answer.** “It is fine, because `NotImplementedError` tells you exactly what went wrong.” It tells you at run time, on the replay path. A type would tell you before anything runs.

**Why it matters.** Interviewers want to hear a reason and a trade-off, not the name of a principle. A design answer that says when you would not bother is more convincing.

**Question 9.30 ★★★ QD, Dev, LLE • any • experienced.**

[Listing 9.29](#lst-iv3-rd-uninit) prints 200 in every unit test and sometimes prints nonsense in production. Built with g++ 11.4, `-O2 -Wall -Wextra -Werror`, it gives no warning. What is the bug, and why does the test pass? How would you make the failure happen every time, on the spot, with no new tools and no recompile? What would it print then? Give the one-token fix.

```cpp
struct Stats { long count; long sum; };

Stats* make_stats() { return new Stats; }

long average(const long* xs, int n) {
    Stats* s = make_stats();
    for (int i = 0; i < n; ++i) {
        s->count += 1;
        s->sum += xs[i];
    }
    const long out = s->sum / s->count;
    delete s;
    return out;
}

int main() {
    const long xs[] = {100, 200, 300};
    std::printf("%ld\n", average(xs, 3));
}
```

***Listing 9.29.** An average with a statistics block. code/interviews-3/09-reading-debugging-and-software-design/cpp/uninit.cpp*

**Answer to Question 9.30.**

`new Stats` leaves `count` and `sum` uninitialised, and reading them is undefined. Tests pass because fresh heap memory happens to be zero. Run with `MALLOC_PERTURB_=165`: it then prints 1. Write `new Stats{}`.

`Stats` has no constructor, so `new Stats` does default initialisation, which for a plain struct does nothing. `new Stats{}` does value initialisation, which zeroes it. The two differ by two characters, and neither gets a warning under `-Wall -Wextra -Werror`, since the allocation and the use are in different functions.

Why does the test pass? In a short-lived process, the allocator hands out memory fresh from the operating system, and that is zeroed. In a long-lived process, the allocator reuses blocks that were freed, so you see old contents, which is the nonsense in production. So the unit test and production really do have different initial conditions.

glibc gives you a way to expose this with no recompile. The man page for `mallopt` describes `M_PERTURB`: bytes of allocated memory are initialised to the complement of the value’s low byte. Setting the environment variable `MALLOC_PERTURB_=165` fills allocations that come from the main allocator with `0x5a`, and the program then prints 1: both fields start at the same huge value, and adding 600 and 3 hardly changes the ratio. With `new Stats{}`, the same run prints 200. On glibc 2.35, a block reused from the per-thread cache keeps what it had, so to catch every one also set `GLIBC_TUNABLES=glibc.malloc.tcache_count=0`. The check needs no rebuild, so it can run in every test job.

Other tools exist. g++ 11.4 rejects `-fsanitize=memory`, as the test confirms. Clang’s MemorySanitizer is the sanitizer built for this, and it needs a rebuild under Clang. Valgrind’s Memcheck reports the use of uninitialised values too. Neither was run for this answer.

**Classic wrong answer.** “Set the fields to zero in a constructor everywhere.” That works, but default member initialisers, `long count = 0;`, are shorter and cannot be forgotten in a new constructor.

**Why it matters.** “It works on my machine and in tests” is what this bug looks like, and a developer who can force it to reproduce in one command is who you want to have around.

**Question 9.31 ★★★ QD, Dev, LLE • market maker, hedge fund • experienced.**

The resident memory of a trading process grows by a few hundred megabytes a day. You run it under AddressSanitizer with leak checking on, and at exit it reports nothing. [Listing 9.30](#lst-iv3-rd-growth) is the shape of the code. Why is the tool silent? There are three different things that can make memory grow. Name them, and describe how you tell which one you have, in order.

```cpp
struct Order { long id; char pad[120]; };

std::map<long, Order> done;              // finished orders, kept "for the audit"

void on_fill(long id) { done.emplace(id, Order{id, {}}); }

int main() {
    for (long id = 0; id < 100000; ++id) on_fill(id);
    std::printf("%zu\n", done.size());
}
```

***Listing 9.30.** Orders kept for the audit. code/interviews-3/09-reading-debugging-and-software-design/cpp/growth.cpp*

**Answer to Question 9.31.**

The memory is still reachable: `done` is a global that points to every order. LeakSanitizer reports only memory with no live pointer left at exit, so it says nothing. The three causes are a true leak, unbounded growth of live data, and allocator retention or fragmentation.

The test shows the difference. The listing, run under ASan with 100000 orders, reports nothing. Replace the map insert by a bare `new Order` whose pointer is thrown away, and it reports the leak at once. The tool answers one exact question, “is anything unreachable?”, and here it is correct. The fault lies in the design: this is what people call a logical leak.

Tell them apart in this order. First, record the allocator’s own view over time alongside the resident size. glibc’s `mallinfo2` reports allocated space and free space. Add `uordblks` and `hblkhd` for the allocated bytes, since large blocks are mapped directly and counted apart.

If allocated bytes grow with resident memory, you have real growth in use, a leak or a grow-only structure. A heap profiler then gives you the allocation sites, ranked by bytes still live. A site that keeps climbing is your suspect, and you read what holds it. If LeakSanitizer is silent, it is reachable, so look for maps, caches, queues and logs with no bound.

If allocated bytes are flat and resident memory grows, there are two cases. Either free space inside the allocator is growing, or the memory never came through `malloc`: direct mappings, thread stacks and libraries with their own allocators do not appear at all, so check `/proc/PID/smaps`. In the first case the allocator is holding freed pages it cannot return, usually because of a mix of sizes that fragments its arenas, or many threads with separate arenas. Look at the free-space figure, and test with another allocator or a bounded arena.

For this code: each entry costs 136 bytes of payload (8 for the key and 128 for the order), plus the tree node’s overhead. Two million fills a day is at least 272 MB a day, which matches the growth. Write the audit to disk, keep only the session’s records in memory, and add a metric for the map’s size.

**Classic wrong answer.** “ASan says no leaks, so it must be fragmentation.” That skips the likeliest cause. Reachable growth is much more common.

**Why it matters.** Trading processes run for days with no restart, and a growth problem is a routine investigation. Interviewers want an ordered method, not a list of tools.

**Question 9.32 ★★★ QD, Dev, LLE • any • experienced.**

[Listing 9.31](#lst-iv3-rd-dcl) looks careful: it checks, locks and checks again. Compiled with g++ 11.4 for x86-64, is it correct? Exactly what can go wrong, and does the strong memory model of x86-64 save it? Give the simplest correct version, then one that keeps the double check. How does the same pattern get fixed in Java?

```cpp
struct Config { long limit = 1000; };

Config* instance = nullptr;
std::mutex m;

Config* get() {
    if (instance == nullptr) {
        std::lock_guard lock(m);
        if (instance == nullptr) instance = new Config;
    }
    return instance;
}

int main() {
    std::vector<std::thread> pool;
    for (int i = 0; i < 4; ++i) pool.emplace_back([] { (void) get()->limit; });
    for (auto& t : pool) t.join();
    std::printf("%ld\n", get()->limit);
}
```

***Listing 9.31.** Double-checked locking. code/interviews-3/09-reading-debugging-and-software-design/cpp/dcl.cpp*

**Answer to Question 9.32.**

It is not correct. The first read of `instance` is unsynchronised against the write in another thread, so this is a data race and undefined behaviour. x86-64 does not save it. The simple correct version is a function-local static. Keeping the double check needs `std::atomic` with acquire and release.

The classic failure: thread A runs `new Config`, which is three steps, allocate, construct, assign. Nothing stops the assignment from becoming visible to thread B before the construction, so B sees a non-null pointer and reads a `limit` that is not yet set. B never takes the lock, so the lock cannot help.

The strong hardware memory model of x86-64 keeps stores in order, but the language’s rules bind the compiler, and it is entitled to reorder a non-atomic write, or to assume no other thread is looking. And the program is undefined regardless of what the machine does. TSan reports `data race` on this listing in the test.

The simplest fix, also from the test:

`Config* get() { static Config instance; return &instance; }`

The standard guarantees this: if control enters the declaration concurrently while the variable is being initialised, the other threads wait for it to finish. g++ does it with a guard variable that is checked on every call and `__cxa_guard_acquire` on the slow path, so it is itself a double check, written for you. TSan is silent.

To keep the pattern, use `std::atomic<Config*>`. The fast path does an acquire load. The slow path holds the mutex, reloads with relaxed order, constructs, and does a release store. Then a thread that sees the pointer also sees the object, and TSan is silent on this version too. `std::call_once` is the library form.

In Java, the same pattern works if the field is `volatile`: the Java Language Specification says that a write to a volatile field happens-before every subsequent read of it. Without `volatile` it is broken in the same way. That rests on the specification, since no test here can force the reordering on a JVM.

**Classic wrong answer.** “x86 has a strong memory model, so it is fine in practice.” The compiler is what reorders here, and a passing test proves little about a race.

**Why it matters.** This pattern still appears in old C++ and Java code, and it separates people who remember the recipe from those who can say what goes wrong.

**Question 9.33 ★★★ QD, Dev, LLE • any • experienced.**

A program is three files. [Listing 9.32](#lst-iv3-rd-rates) and [Listing 9.33](#lst-iv3-rd-limits) each define one global, and [Listing 9.34](#lst-iv3-rd-simain) prints `limit`, with `BASE_RATE` unset. A colleague changes the order of the object files in the link line and the risk limit becomes zero. Explain that. What exactly prints in each order on g++ 11.4 and GNU ld? Give a fix that works in any order, and a keyword that turns the mistake into a compile error.

```cpp
long compute_base() {
    const char* s = std::getenv("BASE_RATE");
    return s ? std::atol(s) : 100;
}

long base_rate = compute_base();
```

***Listing 9.32.** The rates file. code/interviews-3/09-reading-debugging-and-software-design/cpp/static_rates.cpp*

```cpp
extern long base_rate;

long limit = base_rate * 10;
```

***Listing 9.33.** The limits file. code/interviews-3/09-reading-debugging-and-software-design/cpp/static_limits.cpp*

```cpp
extern long limit;

int main() { std::printf("%ld\n", limit); }
```

***Listing 9.34.** The main file. code/interviews-3/09-reading-debugging-and-software-design/cpp/static_main.cpp*

**Answer to Question 9.33.**

With the object files in the order main, rates, limits, it prints 1000. With main, limits, rates, it prints 0. This is the static initialisation order problem. Use a function-local static, and mark a global `constinit` to make it a compile error.

Both globals need a call or a read at start-up, so they have dynamic initialisation. Within one file, the order is the order of definition. Across files the standard leaves it indeterminately sequenced, so the order is up to the implementation, and with g++ and GNU ld here it followed the link line.

Before any dynamic initialisation runs, all globals are zero-initialised. So when `limits` runs first, it reads `base_rate` as 0, and computes $0 \times 10$. Nothing is uninitialised, no sanitizer complains, and the risk limit is simply zero. Later `base_rate` becomes 100, as it should, and the wrong value stays in `limit`. It can get worse: a change in a build script far from either file can flip it.

The fix turns the global into a function:

`long& base_rate() { static long v = compute_base(); return v; }`

The first caller triggers the initialisation, whichever file it is in, and the test prints 1000 for both link orders.

`constinit` is the other tool, new in C++20. It requires that a variable be initialised at compile time, and the standard makes the program ill-formed if it has dynamic initialisation. `constinit long limit = base_rate * 10;` fails to compile, which is what you want: it turns a silent ordering bet into an error. It does not fix anything by itself. It tells you which variables to rewrite.

The most robust design has no such globals. Read the configuration once in `main`, build the objects in order, and pass them where they are needed. That makes the order a line of code that you can read.

**Classic wrong answer.** “Put `limits.o` after `rates.o` and leave a comment.” It works on this toolchain, and the standard promises nothing. A static library or a new linker can change it.

**Follow-up.** *Is the function-local version safe with threads?* Yes, the initialisation is thread-safe since C++11. There is one remaining catch: destruction order at exit, if another global’s destructor uses it after it has gone. Here it is a `long`, so this does not arise.

**Why it matters.** Limits and configuration held in globals are real, and a zero limit found in production is a very bad day.

**Question 9.34 ★★★ LLE, QD, Dev • market maker, hedge fund • experienced.**

[Listing 9.35](#lst-iv3-rd-dispatch) shows five ways to call `score`. Compiled with g++ 11.4 and its standard library, `-std=c++20 -O2`, on x86-64: which of the five functions contain an indirect call or jump? Which is the surprise? Beyond speed, what decides between them when you design a strategy engine?

```cpp
struct Signal {
    virtual ~Signal() = default;
    virtual long score(long x) const = 0;
};
struct Momentum final : Signal {
    long score(long x) const override { return 2 * x; }
};
struct Carry final : Signal {
    long score(long x) const override { return x + 1; }
};
using Any = std::variant<Momentum, Carry>;

long by_virtual(const Signal& s, long x) { return s.score(x); }
long by_final(const Momentum& s, long x) { return s.score(x); }
long by_variant(const Any& a, long x) {
    return std::visit([x](const auto& s) { return s.score(x); }, a);
}
long by_switch(const Any& a, long x) {
    if (const auto* m = std::get_if<Momentum>(&a)) return m->score(x);
    return std::get<Carry>(a).score(x);
}
template <class S> long by_template(const S& s, long x) { return s.score(x); }
template long by_template<Momentum>(const Momentum&, long);
```

***Listing 9.35.** Five kinds of dispatch. code/interviews-3/09-reading-debugging-and-software-design/cpp/dispatch.cpp*

**Answer to Question 9.34.**

`by_virtual` and `by_variant` contain an indirect call or jump. `by_final` and `by_template` inline to two lines (plus an `endbr64` on Ubuntu’s default flags), and `by_switch` is compares and branches, with no indirect call. The surprise is `by_variant`: many people assume `std::visit` avoids an indirect call.

`by_virtual` loads the virtual table and ends in `jmp *`: the compiler cannot know the type. `by_final` takes a `Momentum`, which is `final`, so there is no override to look for, and it inlines. `by_template` knows the type at compile time, and the test finds identical code to `by_final`.

On libstdc++ 11, `std::visit` builds a table of function pointers indexed by the alternative, then uses `call *(%rdx,%rax,8)`. The test reads this out of the assembly. So in this version it is a virtual call in other clothes, plus a check for the valueless state. `by_switch` tests the index with `std::get_if`, and everything inlines into two compares and two branches, with a cold path that throws. A later library may compile `std::visit` differently, so read your own compiler’s output, as this test does, before believing either claim.

For speed, the instruction itself is rarely the main cost. The larger loss is that the compiler cannot inline through an indirect call or optimise across it. Measure it in place.

Other things usually decide the design:

Who can add a type? If strategies arrive from other teams and plugins, the set is open, and virtual is the right tool. Adding a type then touches no existing code. If the set is closed and known, a variant lets the compiler check that every operation handles every type.

Where does it live? A variant is as big as its largest alternative, stored in place with no allocation. That suits a vector of messages. Virtual objects mostly need the heap, and pointer chasing costs more than the call.

Templates give the best code when the type is fixed at build time. But they spread through headers, slow the build and cannot be chosen by a configuration file at run time.

**Classic wrong answer.** “Variants are always faster than virtual calls.” As this compiler shows, it is not even a different kind of call.

**Why it matters.** Low-latency interviews ask this to see whether you reach for the assembly, and whether you can argue the design beyond the nanoseconds.

**Question 9.35 ★★★ QD, Dev, QT • market maker, hedge fund, crypto • experienced.**

[Listing 9.36](#lst-iv3-rd-replay) models what a position keeper does across a reconnect, when the gateway resends the last fill. What does it print, and what should it print? Redesign `on_fill` so a resend is harmless. What must your design do when a fill is missing rather than repeated? How do you test it?

```python
class Position:
    def __init__(self):
        self.qty = 0

    def on_fill(self, fill):
        self.qty += fill["qty"] if fill["side"] == "B" else -fill["qty"]


fills = [{"seq": 1, "side": "B", "qty": 100}, {"seq": 2, "side": "S", "qty": 30}]
p = Position()
for f in fills:
    p.on_fill(f)
for f in fills[-1:]:      # the connection dropped; on reconnect the last fill is resent
    p.on_fill(f)
print(p.qty)
```

***Listing 9.36.** A position across a reconnect. code/interviews-3/09-reading-debugging-and-software-design/python/rd_replay.py*

**Answer to Question 9.35.**

It prints 40. It should print 70, since the resend is the same fill. Make `on_fill` idempotent by remembering the highest sequence number applied per session, and skip anything at or below it. A gap must stop the keeper.

The fault is that `on_fill` treats every message as a new event. On any connection that resends after a reconnect, delivery is at least once, not exactly once, so the receiver must make repeats harmless. [Listing 9.43](#lst-iv3-rd-replayfixed) has the shape.

```python
class Position:
    def __init__(self):
        self.qty = 0
        self.last_seq = {}        # session -> the highest sequence number applied

    def on_fill(self, session, fill):
        if fill["seq"] <= self.last_seq.get(session, 0):
            return                # already applied: a resend changes nothing
        if fill["seq"] != self.last_seq.get(session, 0) + 1:
            raise RuntimeError("gap: stop and recover before going on")
        self.qty += fill["qty"] if fill["side"] == "B" else -fill["qty"]
        self.last_seq[session] = fill["seq"]
```

***Listing 9.43.** A keeper that ignores repeats and stops on gaps. code/interviews-3/09-reading-debugging-and-software-design/python/rd_replay_fixed.py*

Three decisions matter here. First, the sequence is per session, so the state is a map, and the test confirms that two sessions do not interfere. Second, a gap raises an error. Silently applying fill 4 after fill 2 hides a missing fill, and the position is then wrong in a way nobody notices. A trading system should stop quoting and ask for the missing range. Third, the check and the update go together, so the state moves only when a fill is applied.

A high-water mark needs the fills to arrive in order on each session, and it stores one number. If order is not guaranteed, or sequence numbers restart, key on a unique execution id instead and remember the ids you have seen. That set grows, so it needs trimming, say at the end of the session. Whichever you use, the mark has to survive a restart along with the position. Save both together, in one atomic write, or rebuild both from the log. If you save the position without the mark, the first resend after a restart counts twice again.

For the tests, take a random fill sequence and deliver it with random resends of the last few messages between new ones. The final position must equal the plain sum of the fills, whatever the resend pattern. The test runs that 200 times. Add a test for a gap, and a test that restarts in the middle.

**Classic wrong answer.** “Reconcile with the exchange at the end of the day.” You should, but a position that is wrong for hours can breach a limit in the meantime, in either direction.

**Why it matters.** Reconnects happen daily, and a double-counted fill means you trade on a wrong position. This is one of the most common real faults in a position keeper.

**Question 9.36 ★★★ QD, Dev, QR, QT • hedge fund, market maker • experienced.**

You want the strategy in [Listing 9.37](#lst-iv3-rd-strategy) to run unchanged in live trading and in a backtest. You replay five ticks, 2 seconds apart in market time, as fast as the machine can, all above the level. How many orders does it send? How many should it send? Redesign it. What else, beyond the clock, stops a backtest from repeating itself exactly?

```python
import time


class Throttled:
    """Sends at most one order per second, on every tick above a level."""

    def __init__(self, gateway, level):
        self.gateway, self.level = gateway, level
        self.last = 0.0

    def on_tick(self, price):
        now = time.time()
        if price > self.level and now - self.last >= 1.0:
            self.last = now
            self.gateway.send(price)
```

***Listing 9.37.** A throttled strategy. code/interviews-3/09-reading-debugging-and-software-design/python/rd_strategy.py*

**Answer to Question 9.36.**

It sends one order. It should send five, since the ticks are 2 seconds apart in market time. The strategy reads the machine’s clock. Inject the clock and the gateway, so the strategy never finds out which world it is in.

`time.time()` is hidden input. During a replay, all five ticks arrive within microseconds of wall-clock time, so the 1-second throttle blocks the other four. Live, the same code sends all five. A backtest of this would be wrong in a way that is hard to see, because it runs fine and gives numbers. It would also change with the speed of the machine.

[Listing 9.44](#lst-iv3-rd-strategyfixed) has the same logic, except that it asks `self.clock.now()`, and it sends through whatever gateway it is given. In a backtest, the replay loop sets the simulated clock to each event’s timestamp before delivering the event. Live, the clock wraps the system clock and the gateway sends real orders. The test replays the same ticks and gets all five orders, twice, identically.

```python
class Throttled:
    """The same logic: the time and the way out are given to it."""

    def __init__(self, clock, gateway, level):
        self.clock, self.gateway, self.level = clock, gateway, level
        self.last = float("-inf")

    def on_tick(self, price):
        now = self.clock.now()
        if price > self.level and now - self.last >= 1.0:
            self.last = now
            self.gateway.send(price)


class SimClock:
    """In a backtest the replay loop moves this clock to each event's timestamp."""

    def __init__(self):
        self.t = 0.0

    def now(self):
        return self.t
```

***Listing 9.44.** The clock and the gateway are given to it. code/interviews-3/09-reading-debugging-and-software-design/python/rd_strategy_fixed.py*

Note the fixed version’s start value too: `self.last` is minus infinity. In the original it was 0.0, which works only because `time.time()` is a large number. A simulated clock starting at 0 would have blocked the first order, and that bug appears only with the new design.

Other hidden inputs break repetition: random numbers without a saved seed; iteration over a Python `set`, whose order for strings changes between runs; threads, whose interleaving changes the order of events; reading the current date, or a file that changes, or a price from a live service. Order of events with equal timestamps needs a stable rule too.

The test of the whole design is simple: run the same input twice and compare every output, byte for byte. Make that a standing test. In C++, the injection can be a template parameter, so the live path pays nothing for it.

**Classic wrong answer.** “Sleep in the replay so that time passes correctly.” Then a year of data takes a year, and it is still not deterministic.

**Why it matters.** “The same code in research and production” is a goal at nearly every firm. Time is where it usually fails first.

**Question 9.37 ★★★ QD, Dev, QR • market maker, hedge fund • experienced.**

The two tests in [Listing 9.38](#lst-iv3-rd-propbook) pass, and `Book` still has a bug. Without reading for it, write a random test that finds it: what does it generate, what does it check after each step, and how does it give you a short failing case rather than a list of thirty operations? What does the short case come out as? What is the bug?

```python
class Book:
    def __init__(self):
        self.levels = {}          # price -> total quantity
        self.orders = {}          # id -> (price, qty)

    def add(self, oid, price, qty):
        self.orders[oid] = (price, qty)
        self.levels[price] = self.levels.get(price, 0) + qty

    def cancel(self, oid):
        price, qty = self.orders.pop(oid)
        self.levels[price] -= qty

    def best(self):
        return max(self.levels) if self.levels else None


def test_add_then_best():
    b = Book()
    b.add(1, 100, 5)
    b.add(2, 101, 5)
    assert b.best() == 101


def test_cancel_lower():
    b = Book()
    b.add(1, 100, 5)
    b.add(2, 101, 5)
    b.cancel(1)
    assert b.best() == 101
```

***Listing 9.38.** A small book and its unit tests. code/interviews-3/09-reading-debugging-and-software-design/python/rd_prop_book.py*

**Answer to Question 9.37.**

Generate random adds and cancels, keep a plain model of the live orders beside the book, check invariants after every step, and shrink a failure by deleting operations. It comes down to two operations: add order 5 at price 100, then cancel order 5. The bug: `cancel` leaves a level at quantity 0, so `best` returns a price nobody is quoting.

[Listing 9.45](#lst-iv3-rd-propcheck) is all of it, in plain Python with `random`. It uses no library. Each run draws 30 operations over a small range of ids and prices, because small ranges make collisions likely, and collisions are where bugs live. The model is a dict of live orders. It is simple enough to be obviously right, and that is the point. After each step, two facts must hold: the best price equals the highest price among live orders, and the sum over the levels equals the sum over the live orders.

```python

def check(make_book, runs=500, steps=30, seed=1):
    """Random operations against the plain truth. Returns a short failing list, or None."""
    rng = random.Random(seed)
    for _ in range(runs):
        ops = [(rng.choice("ac"), rng.randint(1, 5), rng.randint(99, 101), rng.randint(1, 9))
               for _ in range(steps)]
        if failure(make_book, ops):
            return shrink(make_book, ops)
    return None


def failure(make_book, ops):
    """Run the operations and check every invariant after each one. True if any fails."""
    book, live = make_book(), {}
    for kind, oid, price, qty in ops:
        if kind == "a" and oid not in live:
            book.add(oid, price, qty)
            live[oid] = (price, qty)
        elif kind == "c" and oid in live:
            book.cancel(oid)
            del live[oid]
        if book.best() != max((p for p, _ in live.values()), default=None):
            return True
        if sum(book.levels.values()) != sum(q for _, q in live.values()):
            return True
    return False


def shrink(make_book, ops):
    """Drop operations one at a time for as long as the failure survives."""
    i = 0
    while i < len(ops):
        shorter = ops[:i] + ops[i + 1:]
        if failure(make_book, shorter):
            ops = shorter
        else:
            i += 1
    return ops
```

***Listing 9.45.** A random test and a simple shrinker. code/interviews-3/09-reading-debugging-and-software-design/python/rd_prop_check.py*

Shrinking is simple too: try removing each operation in turn, and keep the removal if the failure survives. That turned 30 operations into the 2 quoted above, which you can read in one glance. Fixing it means deleting the level when it reaches zero. After that, 1000 runs find nothing. The seed is fixed, so a failure can always be replayed.

The generator skips adds of a live id, so it says nothing about that case, and `Book` mishandles it: `add(1, 100, 5)`, `add(1, 101, 5)` leaves 5 lots stranded at 100. Either reject a duplicate id, or let the test generate one and specify the result.

Why did the unit tests miss it? `test_cancel_lower` cancels the lower of two prices, and only the best is checked, so an empty lower level does not show. Its author tested what they had thought of. A random test has no such preference.

The hard part is choosing the invariants, not generating the inputs. Good ones: compare with a slow model, check that a round trip restores the state, and check conservation of quantity. A library such as Hypothesis does the generation and shrinking better for you, and a firm that already has it should use it. In an interview, showing that you can write the 30-line version counts for more.

**Classic wrong answer.** “Add a test for cancelling the best.” That catches this bug, and it is good to keep as a regression test. It does nothing for the next one you did not think of.

**Why it matters.** Order books have many states and few simple inputs, which is the ideal case for random testing. A candidate who proposes it unprompted stands out.

**Question 9.38 ★★★ Dev, QD, LLE • market maker, hedge fund, bank • experienced.**

A binary message on your internal bus is read by six services and written by two. You need to add a field and retire another, and you cannot stop everything at once. How do you roll this out, in order? Which rules must the format follow, and which tests do you add? You also keep years of recorded messages: what does that change?

**Answer to Question 9.38.**

Add the field as optional with a default, and upgrade every reader first, then the writers. Retire the old field in the reverse order: stop all readers from needing it, stop the writers from sending it, and then mark its number as never to be reused. Keep decoders for old recordings for as long as you keep the recordings.

Order matters because you cannot flip everything at once, so at each step every reader must handle every message then on the bus. Rollout for adding:

- Change the schema, and give the new field a default that means “not known”.
- Deploy all six readers. They accept old and new messages, and use the default when the field is absent.
- Deploy the writers, which start filling the field.
- Only then write logic that relies on the field being there.

For removal, run it backwards: readers stop using the field, then writers stop sending it, then the schema marks it retired.

The format must let an old reader skip what it does not know, and a new reader find out what is absent. A tagged format gives you this directly. The Protobuf documentation states that old binaries simply ignore a new field when parsing, and that removing a field is safe as long as its number is not used again. That is the rule that matters: reusing a number makes decoding ambiguous, and its best-practice guide says to reserve deleted numbers and never to add a required field.

Formats with a fixed layout, common on latency-sensitive paths, need more care. A field can only be added at the end of a block, because anywhere else it moves the offset of every later field. The message header then needs a version, or a length so that readers can skip what they do not understand. The field’s meaning and units are part of the contract too. Changing cents to ticks under the same field is the worst case, since every decoder keeps working and every number is wrong. Add a new field for that instead.

Tests to add. Keep a set of recorded messages from each past version in the repository, and decode them in every build. Have the build compare the schema with the previous one, and fail on an incompatible change. During the rollout, run old and new decoders side by side on live traffic and compare their outputs.

Recorded data changes the time horizon. A replay in five years must still read today’s messages. So schemas are never deleted, the version travels with the data, and a change that seems safe for live traffic can still break research.

**Classic wrong answer.** “Bump a version and deploy everything over the weekend.” Recorded data still has the old version, a rollback brings it back, and six teams are rarely ready at the same moment.

**Why it matters.** Firms live with their formats for years. Knowing the order of the rollout separates people who have done this from people who have only read about it.

**Question 9.39 ★★★★ QD, Dev, LLE • market maker, hedge fund • experienced.**

[Listing 9.39](#lst-iv3-rd-bookbugs) is one side of an order book, written for a take-home test. It is used for the bid side and the ask side. You have ten minutes to review it. There are at least three bugs. For each, give the smallest sequence of calls that shows it, and say what you would run to see it. Which of them does AddressSanitizer miss? Then say what you would change in the design.

```cpp
struct Order { long id; long qty; };

class Side {                                 // used for both the bid side and the ask side
public:
    void add(long price, long id, long qty) {
        auto& level = levels_[price];
        level.push_back({id, qty});
        index_[id] = &level.back();
    }
    bool cancel(long id) {
        auto it = index_.find(id);
        if (it == index_.end()) return false;
        it->second->qty = 0;
        index_.erase(it);
        return true;
    }
    long take(long qty) {                    // best price first, oldest first
        long filled = 0;
        for (auto& [price, level] : levels_) {
            for (auto& o : level) {
                const long n = std::min(o.qty, qty - filled);
                o.qty -= n;
                filled += n;
                if (n > 0 && o.qty == 0) index_.erase(o.id);
            }
            if (spent(level)) levels_.erase(price);
            if (filled == qty) break;
        }
        return filled;
    }
    bool empty() const { return levels_.empty(); }
    long best() const { return levels_.begin()->first; }

private:
    static bool spent(const std::vector<Order>& level) {
        return std::ranges::all_of(level, [](const Order& o) { return o.qty == 0; });
    }
    std::map<long, std::vector<Order>> levels_;
    std::unordered_map<long, Order*> index_;
};
```

***Listing 9.39.** One side of a book. code/interviews-3/09-reading-debugging-and-software-design/cpp/book_bugs.hpp*

**Answer to Question 9.39.**

There are four bugs: a dangling pointer after a second order at a level, an erase inside a range-for, the wrong end of the book for bids, and a cancel that leaves its level behind. ASan catches only the first. The second is a memory bug that it misses, and the last two are logic that no memory tool can see.

**Dangling pointer.** `index_` stores `&level.back()`, a pointer into a `std::vector`, and the next `push_back` at that level may reallocate. Sequence: `add(100, 1, 5)`, `add(100, 2, 5)`, `cancel(1)`. ASan reports `heap-use-after-free` in `cancel`.

**Erase inside the loop.** `take` erases the level from `levels_` inside the range-for that walks `levels_`, so the loop’s iterator is invalid when it steps. Sequence: `add(100, 1, 5)`, `add(101, 2, 5)`, `take(7)`. Here ASan says nothing: the step is in `_Rb_tree_increment`, compiled inside the uninstrumented `libstdc++.so`, and the freed node still holds good pointers. It prints the right fill. `-D_GLIBCXX_DEBUG` stops it, with a message about a singular iterator. So one tool is never enough.

**Wrong side.** The map orders prices upward, so `begin()` is the lowest. For asks that is the best. For bids it is the worst. Sequence on a bid side: `add(100, 1, 5)`, `add(101, 2, 5)`, `take(3)`. It fills at 100 and not at the bid of 101. No tool finds this: only a test with an expected answer does. The comment on the class is the clue.

**Stale level.** `cancel` sets the quantity to zero and leaves the entry. After `add(100, 1, 5)`, `cancel(1)`, `best()` returns 100 with nothing there. A `take` erases each spent level it visits, so empty entries pile up only deeper in the book, where no `take` reaches, and there they use memory for good.

[Listing 9.46](#lst-iv3-rd-bookfixed) fixes all four. A `std::list` per level keeps iterators stable. Each index entry stores its price and iterator, so a cancel is $O(1)$ plus the level lookup. The side is a template parameter, so a bid side uses `std::greater`. `take` advances with the iterator that `erase` returns. The test checks it under ASan on 30 random scripts for each side, against a slow Python model.

```cpp
template <class Better>                      // std::less for asks, std::greater for bids
class Side {
public:
    void add(long price, long id, long qty) {
        auto& level = levels_[price];
        level.push_back({id, qty});
        index_[id] = {price, std::prev(level.end())};   // list iterators survive growth
    }
    bool cancel(long id) {
        auto it = index_.find(id);
        if (it == index_.end()) return false;
        auto level = levels_.find(it->second.price);
        level->second.erase(it->second.where);
        if (level->second.empty()) levels_.erase(level);
        index_.erase(it);
        return true;
    }
    long take(long qty) {
        long filled = 0;
        for (auto it = levels_.begin(); it != levels_.end() && filled < qty;) {
            auto& level = it->second;
            while (!level.empty() && filled < qty) {
                Order& o = level.front();
                const long n = std::min(o.qty, qty - filled);
                o.qty -= n;
                filled += n;
                if (o.qty == 0) { index_.erase(o.id); level.pop_front(); }
            }
            it = level.empty() ? levels_.erase(it) : std::next(it);
        }
        return filled;
    }
    bool empty() const { return levels_.empty(); }
    long best() const { return levels_.begin()->first; }

private:
    struct Where { long price; std::list<Order>::iterator where; };
    std::map<long, std::list<Order>, Better> levels_;
    std::unordered_map<long, Where> index_;
};
```

***Listing 9.46.** The corrected side. code/interviews-3/09-reading-debugging-and-software-design/cpp/book_fixed.hpp*

On design, I would say four more things. A `long` for both price and quantity allows them to be swapped, so use types. There is no check on duplicate ids or non-positive quantities. A list allocates on every order, so a hot path would use a pool or an intrusive list. And a real book needs an interface for a fill’s details, not only how much was filled.

**Classic wrong answer.** “It passes under ASan, so the memory handling is right.” The second bug passes under ASan on every input you try. Each tool sees only what it instruments.

**Why it matters.** This is the take-home review in its purest form. Interviewers count bugs, but they also watch whether you can show each one with a short sequence.

**Question 9.40 ★★★★ QD, Dev, QR, DS • any • junior, experienced.**

A candidate’s take-home answer is [Listing 9.40](#lst-iv3-rd-review): it reads a CSV of trades with columns `symbol`, `price` and `qty`, and reports the volume-weighted average price per symbol. It gives the right numbers on the sample file. You are reviewing it. List what is wrong, most serious first, and for each say what input shows it. Then sketch the version you would accept and the tests that go with it.

```python
import csv

totals = {}


def load(path, symbols=[]):
    f = open(path)
    for row in csv.DictReader(f):
        if row["symbol"] in symbols or not symbols:
            key = row["symbol"]
            if key not in totals:
                totals[key] = [0.0, 0]
            totals[key][0] += float(row["price"]) * int(row["qty"])
            totals[key][1] += int(row["qty"])
    return totals


def vwap(symbol):
    notional, qty = totals[symbol]
    return notional / qty


def report(path):
    for symbol in load(path):
        print(symbol, round(vwap(symbol), 2))
```

***Listing 9.40.** A take-home answer. code/interviews-3/09-reading-debugging-and-software-design/python/rd_review.py*

**Answer to Question 9.40.**

Most serious first: a hidden global that mixes files, one bad row killing the whole run, a crash on zero quantity, floats for money, and a file never closed. The mutable default looks bad, but it is harmless here. Accept a pure function over parsed trades, plus a thin file wrapper.

**Global state.** `totals` survives between calls. Load day 1 (10 at 100 and 30 at 102, VWAP 101.5), then day 2 (10 at 200), and `vwap("AAA")` gives 121.2, mixing the two days. No error appears. This is the worst fault, because the number looks plausible. It also means the function cannot be tested without clearing a global first.

**No handling of bad rows.** One row with `price` equal to `oops` raises `ValueError`, and the file is lost at that row. So a single bad print loses a day. Worse, the earlier rows are already in `totals`, so a caller who catches the error sees partial totals. Decide on a policy: count and report the bad rows.

**Zero quantity.** Rows of `+10` and `-10`, as in a correction, give `ZeroDivisionError` in `vwap`.

**Floats.** `sum([0.1] * 10)` is not 1.0, so small errors build up over millions of rows, and then `round` adds half-to-even behaviour in the report. Prices arrive as exact text, so keep them exact, as `Fraction`, `Decimal` or integer ticks.

**The file is never closed.** `open` has no `with`. CPython closes the file when the object is collected, and raises a `ResourceWarning`, as the test confirms. Nothing in the language promises when that happens. The file handle is a resource, so say when it ends.

**Mutable default.** `symbols=[]` is never changed, so it does no harm here, and saying so shows you read it rather than matched a pattern. Still, ask for `None` or a tuple. `in` on a list is also linear, so a set would be better. The way it is written, an empty list means “everything”, which is a surprising rule to leave undocumented.

**Structure.** `load` opens, parses, filters and accumulates, and `report` prints. Nothing can be used alone.

[Listing 9.47](#lst-iv3-rd-reviewfixed) separates these: `parse` turns rows into trades and collects errors, `vwaps` is a pure function, and `report` only handles the file. The tests cover both days giving their own answers, bad rows counted with their row numbers, whether a word for a price, a short row or a price of `1/0`, `None` for zero quantity, ten trades at 0.1 averaging exactly 0.1, and, on random trades, that shuffling the input and calling a second time change nothing.

```python
import csv
from collections.abc import Iterable, Iterator
from fractions import Fraction
from typing import NamedTuple


class Trade(NamedTuple):
    symbol: str
    price: Fraction
    qty: int


def parse(rows: Iterable[dict], errors: list) -> Iterator[Trade]:
    for n, row in enumerate(rows, 1):
        try:
            yield Trade(row["symbol"], Fraction(row["price"]), int(row["qty"]))
        except (KeyError, ValueError, TypeError, ZeroDivisionError) as e:
            errors.append((n, repr(e)))


def vwaps(trades: Iterable[Trade]) -> dict:
    """A pure function: same trades in, same numbers out, and no hidden state."""
    totals = {}
    for t in trades:
        notional, qty = totals.get(t.symbol, (Fraction(0), 0))
        totals[t.symbol] = (notional + t.price * t.qty, qty + t.qty)
    return {s: (n / q if q else None) for s, (n, q) in totals.items()}


def report(path: str) -> tuple[dict, list]:
    errors = []
    with open(path, newline="") as f:
        return vwaps(parse(csv.DictReader(f), errors)), errors
```

***Listing 9.47.** The version to accept. code/interviews-3/09-reading-debugging-and-software-design/python/rd_review_fixed.py*

The fixed version keeps one running pair per symbol, so memory stays small. It does not check for duplicate trades, and in an interview I would ask what the file guarantees before adding that. A NUL byte still stops the `csv` reader itself on Python 3.10: treat that as a corrupt file, not a bad row.

**Classic wrong answer.** “Leading with the mutable default.” It is the first thing a pattern-matcher sees and the least harmful thing here. Order your review by what can cost money.

**Why it matters.** Reviewing is a large part of the job, and a take-home review tests judgement: what you put first, and whether you can show each problem with an input.