Clean Code, Refactoring, and Maintainability Questions

Direct answer. The leak is manual new with no matching delete on the exception path; fix it with RAII so ownership is expressed as an object's LIFETIME, and the destructor -- which C++ guarantees runs even during stack unwinding -- frees the memory automatically.

Before (leaks on the exception path)

struct Buffer {
    int* data;
    Buffer(int n) { data = new int[n]; }
    void destroy() { delete[] data; } // caller must remember to call this
};
void run(bool throwMidway) {
    Buffer* b = new Buffer(4);
    b->fill(4, 7);
    if (throwMidway) throw std::runtime_error("boom"); // b->destroy() below never runs -- LEAK
    b->destroy();
    delete b;
}

I instrumented this with a live-allocation counter and confirmed the leak directly: after run(true) throws and is caught, live_allocations == 1 -- the Buffer and its data array are never freed because the exception skipped the manual destroy()/delete calls.

After (RAII)

struct SafeBuffer {
    std::vector<int> data;   // owns its memory; freed automatically
    explicit SafeBuffer(int n) : data(n) {}
    ~SafeBuffer() { /* vector's own destructor frees the memory; ours runs too */ }
};
void run(bool throwMidway) {
    SafeBuffer b(4);          // no `new`, no matching `delete` to forget
    b.fill(4, 7);
    if (throwMidway) throw std::runtime_error("boom"); // stack unwinds; b's destructor still runs
}

Same instrumentation confirms live_allocations == 0 after the equivalent thrown-and-caught exception -- the destructor runs during stack unwinding, before the exception is caught further up.

Why this works

C++ guarantees that as an exception propagates up the call stack, every fully-constructed local object in each stack frame it passes through has its destructor invoked ('stack unwinding'). By making the buffer's LIFETIME be a normal stack-allocated object (or wrapped in std::vector/std::unique_ptr), cleanup becomes the language's responsibility, not something a human has to remember on every exit path (normal return, early return, AND every possible thrown exception).

Trade-offs and pitfalls

• RAII wrapping via std::vector here also gets you bounds-checked access with .at() if desired, but note operator[] is still unchecked, same as a raw pointer -- RAII fixes lifetime, not bounds safety, and the two are separate concerns.
• If a class holds a raw resource that has no existing RAII wrapper (a C library handle), wrap it yourself in a small class whose constructor acquires and destructor releases -- don't leave raw acquire/release calls scattered through business logic.
• Watch for RAII objects captured by reference/pointer and used after the owning scope exits (a dangling reference); RAII solves 'did I forget to free this' but not 'am I using this after its owner is gone.'

Direct answer. Good version-control hygiene means every commit and PR tells a clear, minimal, reviewable story: small commits with messages that explain WHY, PRs scoped to one reviewable change, and a branching approach the whole team actually follows consistently.

Commit granularity and messages

• Each commit should represent ONE logical change that could, in principle, be reverted independently without breaking something unrelated -- a commit that mixes a bug fix with an unrelated formatting sweep makes both harder to review and harder to revert cleanly later.
• Good message: Fix race condition in order total calculation (see INC-204)\n\nTwo concurrent requests could both read the stale total before either\nwrite committed. Wrap the read-modify-write in a transaction. -- explains WHY (the bug, the mechanism) not just WHAT (which is visible in the diff already).
• Bad message: fix bug -- tells a future reader (including git blame six months from now) nothing they couldn't already see from the diff itself, and gives zero context for WHY this change was needed.

PR size and branch/PR conventions

• Smaller PRs get reviewed faster and more thoroughly -- a reviewer can hold a 100-line diff in their head; a 2,000-line diff gets a rubber-stamp approval because nobody can meaningfully review it in one sitting.
• A consistent branch-naming convention (fix/order-total-race, feature/bulk-export) makes it easy to scan open branches and understand what's in flight without opening each one.
• Rebasing versus merging is a team-level convention choice (rebase keeps history linear and easier to bisect; merge preserves the exact chronological record) -- the specific choice matters less than the TEAM actually agreeing on and following one consistently, so history reads predictably regardless of who wrote it.

Handling large binary or generated files

• Committing large binaries (design assets, model weights, video fixtures) directly into a normal git history bloats every future clone and slows down operations like git log and git blame for the whole team, forever, even after the file is deleted, since git history keeps every version.
• Use Git LFS (or an equivalent large-file extension) for binaries that genuinely need to be version-controlled alongside code, so the main repository only stores a lightweight pointer.
• For anything that can be REGENERATED from source (build output, compiled assets, lockfile-derived artifacts), keep it out of version control entirely via .gitignore rather than committing it and then fighting merge conflicts on a file nobody hand-edits.
• If large files were already committed by mistake, history-rewriting tools (git filter-repo) can remove them retroactively, but that rewrites shared history and needs the same coordination caution as any other history rewrite on a branch others have pulled.

Why this matters for maintainability specifically

Git history is a maintainability tool in its own right: git blame and git log are often the FASTEST way to understand why a confusing piece of code exists, but only if commit messages actually explain the why -- a history of 'fix bug', 'wip', 'more fixes' gives future maintainers nothing to work with when they're trying to understand a decision made months or years ago.

Trade-offs and pitfalls

• Enforcing small PRs can pressure people to under-scope a change that's genuinely indivisible (e.g., a schema migration that must ship atomically with the code that depends on it) -- the goal is REVIEWABLE size, not an arbitrary line-count limit that ignores what a change actually requires.
• Rewriting history (interactive rebase, squashing) before merging to clean up a messy WIP trail is generally good practice, but doing it on a SHARED branch others have already pulled causes real pain -- keep history rewrites scoped to your own not-yet-shared branch.

Direct answer. Design the public surface around what the CALLER needs to accomplish, not around your internal implementation details -- keep the surface small, make illegal states hard to represent, and design for evolution (adding capability later without breaking existing callers) from the start.

Worked example: a camera-capture SDK

// A surface shaped around the CALLER's goal, not the implementation
interface CameraCapture {
    fun requestPermission(callback: (Granted) -> Unit)
    fun capturePhoto(options: CaptureOptions = CaptureOptions.default()): CaptureResult
    fun release()
}

Compare this to exposing internal details directly (raw camera device handles, platform-specific capture session objects) -- the caller shouldn't need to understand HOW capture works internally, only WHAT they can ask the SDK to do.

What makes an API hard to misuse

• Make invalid states unrepresentable: rather than a boolean isPermissionGranted the caller must remember to check before calling capturePhoto, the method itself can enforce the check and return a typed result (PermissionDenied | Success(photo)) that FORCES the caller to handle both cases at compile time rather than trusting them to remember.
• Sensible, safe defaults: CaptureOptions.default() lets the common case be a one-line call, while power users can override specific options -- this avoids forcing every caller through a long parameter list for the 90% case.
• Explicit lifecycle: release() makes resource cleanup an obvious, discoverable part of the contract rather than an implicit expectation buried in documentation.

Designing for safe evolution

• Prefer adding a NEW optional parameter with a default (non-breaking) over changing an existing parameter's meaning.
• Return a structured result object rather than a bare value, so adding a new field later (e.g., photo metadata) doesn't require a breaking signature change.
• Version the SDK's public surface explicitly (semantic versioning) so consumers can tell, from the version number alone, whether an upgrade might require code changes.

Trade-offs and pitfalls

• Over-abstracting the surface (hiding EVERY implementation detail behind layers of indirection) can make the SDK harder to debug when something does go wrong, since the caller has no visibility into what's actually happening underneath -- expose enough (logging hooks, diagnostic callbacks) that failures are debuggable without breaking the abstraction.
• A 'safe defaults' design can hide important decisions from callers who actually need to know about them (e.g., silently defaulting to a lower photo resolution) -- make defaults sensible for the COMMON case, but ensure anything with real consequences is visible in documentation even if not required in every call.

Direct answer. Comment when the code can't express WHY (a business rule, a workaround, a non-obvious trade-off); refactor instead of commenting when the comment only restates WHAT the code already says -- a comment that duplicates the code is guaranteed to drift out of sync with it eventually.

The trivial case

# increment i by 1
i += 1

This comment is pure noise: it tells you nothing i += 1 doesn't already say faster to read. Delete it; if i needs a better name to convey intent (e.g., retry_count += 1), fix the name instead of commenting around it.

A comment that legitimately belongs (explains WHY)

# Stripe requires idempotency keys to be reused for retries within 24h,
# otherwise it treats a retry as a new charge. See INC-4021.
idempotency_key = order_id  # intentionally NOT time-based

No amount of renaming makes 'why we chose this specific value, tied to an external API's undocumented-until-we-got-burned behavior' obvious from the code alone -- this is exactly the kind of context a comment should preserve, ideally with a link to the incident/ticket for anyone who wants the full story.

A comment that's a smell (explains WHAT, redundant with the code)

# loop through all users
for user in users:

The code already says this as clearly as English could; the comment adds a second thing that has to be kept in sync every time the loop changes, for zero reader benefit.

A simple test to apply

Ask: 'if I deleted this comment, would a competent reader lose information, or just lose a restatement?' If deleting it loses nothing, delete it. If deleting it loses the REASON something non-obvious is true, keep it (and consider whether the reason belongs in a commit message / ticket link too, for permanence).

Trade-offs and pitfalls

• Comments that explain why are still at risk of going stale if the underlying reason changes (the external API behavior gets fixed) but nobody removes the now-obsolete comment -- treat comments as code that also needs maintenance, not a write-once artifact.
• Don't over-correct into a 'no comments ever' culture; some domains (financial regulations, security-sensitive code, deliberately non-obvious performance tricks) genuinely need WHY documented, and a codebase that bans comments entirely just pushes that knowledge into people's heads (or nowhere), which is worse.
• A comment that says 'TODO: fix this properly' with no ticket link or date is close to noise too -- if it's worth flagging, it's worth tracking somewhere more durable than an inline string that nobody searches for.

Direct answer. Dependency injection gives you compile-time (or construction-time) visibility into what a class depends on and lets a test substitute a fake trivially; the service locator hides dependencies inside a global lookup, so a reader (and a test) can't tell what a class actually needs without reading its full body.

Dependency injection

class OrderService:
    def __init__(self, payment_client, notifier):
        self.payment_client = payment_client
        self.notifier = notifier

Every dependency is visible in the constructor signature. A test constructs OrderService(fake_payment_client, fake_notifier) with zero global state to set up or tear down, and the signature itself documents the class's needs.

Service locator

class OrderService:
    def charge(self, amount):
        payment_client = ServiceLocator.get("payment_client")  # dependency is hidden
        payment_client.charge(amount)

A reader has to open the METHOD BODY (not just the constructor) to discover this class needs a payment client at all, and a test has to configure the global locator before running, then remember to reset it afterward or risk leaking state into the next test.

What you gain and lose with each

• Testability: DI wins clearly -- dependencies are explicit and swappable per-test with no shared global state. Service locator tests are more fragile (order-dependent, need setup/teardown discipline) because the locator itself is global mutable state.
• Discoverability: DI wins -- you can tell a class's dependencies from its constructor without reading every method. Service locator dependencies are invisible until you trace every call site that hits the locator.
• Runtime cost: essentially a wash for typical DI (constructor injection is just normal object construction); a poorly implemented service locator that does string-keyed lookups on every call adds a small runtime cost DI avoids, though a well-cached locator narrows this gap.
• Convenience for deep call chains: service locator can look more convenient when a dependency is needed 10 layers deep and you don't want to thread it through every intermediate constructor ('parameter drilling') -- but that's usually a signal the layering itself needs rethinking, not a case for hiding the dependency.

Default recommendation

Default to constructor-based DI for new code; it makes the dependency graph an explicit, reviewable part of the design and keeps tests simple and isolated. Reach for a locator (or a DI container that resolves dependencies FOR you, which is a more disciplined middle ground) mainly in frameworks/plugin systems where the set of implementations is genuinely dynamic and not known at construction time.

Trade-offs and pitfalls

• A DI container (Spring, Guice, etc.) can itself become a soft service locator if code reaches into the container directly at arbitrary points rather than only at composition roots -- the discipline that matters is WHERE resolution happens, not just which mechanism you use.
• Excessive constructor parameters from over-injecting is itself a smell (see the parameter-object survivor) -- if a class needs eight injected dependencies, that's often a sign it has too many responsibilities.