HTA Reviewer: Bad Patterns & Anti-patterns

This document captures “Bad Ideas” and anti-patterns that should be flagged during review.

1. “Test Mode” in Production

Pattern: Adding a boolean flag like inside_test_ or test_mode_ to a production class to alter its behavior for unittests. HTA's Take: "I‘m not happy about adding flags that put the code into ‘test mode’. It’s a bad pattern

  • as far as possible, we should be testing the actual code, and adaptations should be in the test fixture." Fix: Use dependency injection (e.g., inject a factory or a mock handler) or specialize the test fixture.

2. Command-line Flags in Unittests

Pattern: Using ABSL_FLAG to control unittest behavior or to trigger specific test cases. HTA's Take: “I still don't like the use of ABSL_FLAG in unittest binaries... testing should be automatic and done in CQ.” Fix: If it‘s a performance test, move it to a dedicated benchmark binary. If it’s an integration test, use INSTANTIATE_TEST_SUITE_P to cover different configurations automatically.

3. Mixing Input and Output Concepts

Pattern: A configuration struct or class where some members are inputs (provided by the user) and others are internal outputs (calculated during setup), but they are all mixed together. HTA's Take: “Here you are not separating the concepts that are input to the configuration setup from the concepts that are its output. Thus, this becomes unreadable.” Fix: Separate input configuration from the resulting state. Make the state class immutable (const) after construction.

4. Opaque Logic / Magic Numbers

Pattern: Using hex values or bitwise operations without descriptive constants or comments. HTA's Take: “what‘s 0x10 here? ... it’s more obvious to use !((udpOpts & PacketSocketFactory::OPT_DTLS) && (udpOopts & PacketSocketFactory::OPT_DTLS_INSECURE)) instead.” Fix: Replace magic numbers with named constants or clear boolean expressions.

5. Weak Justification for Relands

Pattern: Relanding a reverted CL without clearly explaining why the previous issue is resolved. HTA's Take: “Relands should always tell why it's now safe to land them. ... When relanding, always make sure to say in the description why you think it will pass this time.” Fix: Add a detailed explanation in the commit message regarding the fix for the original failure (e.g., “Reason for reland: Moved problematic functions to .cc file”).

6. “Impure” Default Flips

Pattern: Mixing logic changes or test adaptations with the flipping of a default flag in the same CL. HTA's Take: “I'd be happier if those changes were landed first, and the default-flip were in a pure CL that did only that.” Fix: Split the CL. Land the supporting changes (tests, internal fixes) first, then flip the default in a dedicated, minimal CL.