diff --git a/dot_claude/skills/clean-code-craftsman/SKILL.md b/dot_claude/skills/clean-code-craftsman/SKILL.md new file mode 100644 index 0000000..feeff78 --- /dev/null +++ b/dot_claude/skills/clean-code-craftsman/SKILL.md @@ -0,0 +1,1205 @@ +--- +name: clean-code-craftsman +description: | + Enforces high-quality software engineering practices when writing code, based on + principles from Robert C. Martin (Clean Code, Clean Architecture), John Ousterhout + (A Philosophy of Software Design), Kent Beck (Simple Design, TDD), Martin Fowler + (Refactoring), and The Pragmatic Programmer. Triggers: (1) Before writing any + non-trivial code, (2) during code reviews, (3) when refactoring, (4) when the user + asks for "clean" or "production-quality" code. Covers naming, function design, SOLID, + deep modules, error handling, testing, and code structure. Primary languages: Python + and TypeScript. +author: Claude Code +version: 1.1.0 +date: 2026-02-07 +--- + +# Clean Code Craftsman + +You write code as a craftsman. Every function, every name, every abstraction is a +deliberate act of communication to the next developer who reads it — including +future-you. Quality is not a phase; it is woven into every keystroke. + +## When This Skill Activates + +Apply these principles **every time you write or modify code**. This is not a checklist +you run at the end — it is how you think while coding. + +--- + +## 1. Naming Is Design + +> "You should name a variable using the same care with which you name a first-born child." +> — Robert C. Martin + +Names are the primary vehicle of intent in code. A good name eliminates the need for +a comment. A bad name is a tiny lie that compounds into confusion. + +### Rules + +- **Reveal intent**: The name should answer why it exists, what it does, and how it is + used. If a name requires a comment, the name is wrong. +- **Make meaningful distinctions**: `accountList` vs `accounts` vs `accountGroup` — each + must mean something different, or collapse them into one. +- **Use domain vocabulary**: Name things using the language of the problem domain + (Ubiquitous Language). If the business calls it a "claim", don't call it a "request". +- **Verb phrases for functions**: `fetch_listings`, `calculate_route`, `validateAddress`. + Functions do things; their names should say what. +- **Noun phrases for classes and variables**: `ListingRepository`, `transit_route`, + `priceRange`. +- **Avoid encodings**: No Hungarian notation, no type prefixes (`strName`, `iCount`), + no member prefixes (`m_`). Modern editors and type systems make these redundant. +- **Length proportional to scope**: Loop counters can be `i`. Module-level constants + should be `MAX_RETRY_ATTEMPTS`. The wider the scope, the more descriptive the name. +- **Searchable names**: Avoid single-letter names and magic numbers outside the + tightest scopes. `RESULTS_PER_PAGE = 25` beats a bare `25`. +- **Don't be cute**: `whack()` instead of `kill()`? No. Clarity over cleverness. +- **One word per concept**: Pick `fetch`, `retrieve`, or `get` and stick with it + throughout the codebase. Don't use all three to mean the same thing. + +### Python + +```python +# BAD: What does this do? What's d? What's 7? +def calc(d: list[dict]) -> list[dict]: + return [x for x in d if x["s"] >= 7] + +# GOOD: Every name tells you exactly what's happening +MIN_LISTING_SCORE = 7 + +def filter_high_scoring_listings(listings: list[Listing]) -> list[Listing]: + return [listing for listing in listings if listing.score >= MIN_LISTING_SCORE] +``` + +### TypeScript + +```typescript +// BAD: Cryptic, forces reader into the implementation +const proc = (d: any[], f: boolean) => d.filter(x => f ? x.a : x.b); + +// GOOD: Self-documenting +function filterListingsByAvailability( + listings: Listing[], + status: AvailabilityStatus, +): Listing[] { + return listings.filter((listing) => listing.availability === status); +} +``` + +### Smell Test + +If you read a name and need to look at the implementation to understand what it does, +rename it. + +--- + +## 2. Functions + +> "The first rule of functions is that they should be small. The second rule is that +> they should be smaller than that." — Robert C. Martin + +> "Functions should do one thing. They should do it well. They should do it only." +> — Robert C. Martin + +### Rules + +- **Small**: A function should fit on a screen. If you're scrolling, it's too long. + Aim for 5-20 lines. This is not a hard rule — some algorithms are inherently longer — + but it is the default. +- **One level of abstraction**: A function should not mix high-level orchestration + with low-level details. Extract the low-level work into well-named helper functions. +- **Single Responsibility**: A function does one thing. If you describe what a function + does and use the word "and", it does two things. Split it. +- **Few arguments**: Zero is ideal. One is good. Two is acceptable. Three requires + justification. More than three — introduce a parameter object or rethink the design. +- **No side effects**: A function named `check_password` should not also initialize + a session. If it must do both, name it `check_password_and_init_session` — then realize + that name is a smell and refactor. +- **Command-Query Separation**: Functions should either do something (command) or + answer something (query), not both. +- **Guard clauses for preconditions**: Handle edge cases at the top and return early. + This keeps the main logic at the lowest indentation level. +- **Don't Repeat Yourself (DRY)**: If you see the same 3+ lines in two places, + extract a function. But don't create premature abstractions for coincidental + similarity — wait until you see the pattern three times (Rule of Three). +- **No flag arguments**: A boolean parameter means the function does two things. + Write two functions instead. + +### Python + +```python +# BAD: Too long, mixed abstraction levels, flag argument, multiple responsibilities +def process_listing(listing: dict, send_email: bool = False) -> None: + if listing.get("price") and listing.get("address"): + conn = sqlite3.connect("data/wrongmove.db") + cursor = conn.cursor() + cursor.execute( + "INSERT INTO listings (price, address, sqm) VALUES (?, ?, ?)", + (listing["price"], listing["address"], listing.get("sqm", 0)), + ) + conn.commit() + conn.close() + if send_email: + import smtplib + server = smtplib.SMTP("smtp.gmail.com", 587) + server.starttls() + # ... 20 more lines of email sending + server.quit() + +# GOOD: Each function does one thing, clear names, no flags +def save_listing(listing: Listing, repository: ListingRepository) -> None: + repository.insert(listing) + +def notify_new_listing(listing: Listing, notifier: EmailNotifier) -> None: + notifier.send_new_listing_alert(listing) + +def process_new_listing( + listing: Listing, + repository: ListingRepository, + notifier: EmailNotifier, +) -> None: + save_listing(listing, repository) + notify_new_listing(listing, notifier) +``` + +```python +# BAD: Deeply nested, no guard clauses +def calculate_price_per_sqm(listing: Listing) -> float | None: + if listing is not None: + if listing.price is not None: + if listing.square_meters is not None: + if listing.square_meters > 0: + return listing.price / listing.square_meters + else: + return None + else: + return None + else: + return None + else: + return None + +# GOOD: Guard clauses, flat structure, happy path at lowest indentation +def calculate_price_per_sqm(listing: Listing) -> float | None: + if listing.price is None or listing.square_meters is None: + return None + if listing.square_meters <= 0: + return None + return listing.price / listing.square_meters +``` + +### TypeScript + +```typescript +// BAD: Flag argument, mixed concerns +async function getListings(includeArchived: boolean): Promise { + // ... +} + +// GOOD: Two functions, each does one thing +async function getActiveListings(): Promise { + return listingRepository.findByStatus("active"); +} + +async function getArchivedListings(): Promise { + return listingRepository.findByStatus("archived"); +} +``` + +```typescript +// BAD: Too many parameters +function createListing( + price: number, + address: string, + bedrooms: number, + bathrooms: number, + sqm: number, + type: string, + district: string, +): Listing { /* ... */ } + +// GOOD: Parameter object +interface CreateListingParams { + price: number; + address: string; + bedrooms: number; + bathrooms: number; + squareMeters: number; + propertyType: PropertyType; + district: District; +} + +function createListing(params: CreateListingParams): Listing { /* ... */ } +``` + +### The Stepdown Rule + +Code should read like a top-down narrative. Each function should be followed by the +functions it calls, at the next level of abstraction. A reader should be able to read +the code from top to bottom, understanding each level before diving deeper. + +--- + +## 3. Deep Modules, Not Shallow Ones + +> "The best modules are those whose interfaces are much simpler than their +> implementations." — John Ousterhout + +### The Depth Principle + +A module's value is the functionality it provides minus the complexity of its interface. + +- **Deep module**: Simple interface, rich functionality. The Unix file I/O API + (`open`, `read`, `write`, `close`) hides enormous complexity behind five calls. +- **Shallow module**: Complex interface relative to what it does. A class with 15 + methods that each delegate to one other call adds complexity without hiding any. + +### Rules + +- **Pull complexity downward**: When choosing where to put complexity, push it into the + implementation, not into the interface. Make the caller's life simple. +- **Information hiding**: Each module should encapsulate design decisions that are likely + to change. The interface exposes *what*, never *how*. +- **Avoid pass-through methods**: A method that does nothing but forward to another + method is a sign of a shallow decomposition. Collapse the layers. +- **Avoid pass-through variables**: Threading a variable through multiple layers just to + reach the one place that needs it is a design smell. Consider dependency injection or + context objects. +- **General-purpose interfaces**: A somewhat general interface is often simpler and + deeper than a special-purpose one. Design for the common case, not every possible use. +- **Define errors out of existence**: Instead of throwing an error for "file not found + on delete," just make delete idempotent. Fewer error paths = less complexity. + +### Python + +```python +# BAD: Shallow — every method is a trivial pass-through, caller must orchestrate +class ListingExporter: + def open_file(self, path: str) -> IO: ... + def write_header(self, file: IO, columns: list[str]) -> None: ... + def write_row(self, file: IO, listing: Listing) -> None: ... + def close_file(self, file: IO) -> None: ... + +# Caller has to know the entire protocol: +exporter = ListingExporter() +f = exporter.open_file("out.csv") +exporter.write_header(f, ["price", "sqm"]) +for listing in listings: + exporter.write_row(f, listing) +exporter.close_file(f) + +# GOOD: Deep — simple interface hides all the complexity +class ListingExporter: + def export_to_csv(self, listings: list[Listing], path: Path) -> None: + """Export listings to CSV. Handles file lifecycle, headers, encoding.""" + ... + +# Caller just says what they want: +exporter.export_to_csv(listings, Path("out.csv")) +``` + +### TypeScript + +```typescript +// BAD: Shallow wrapper that adds nothing — just pass-through +class ApiClient { + private axios: AxiosInstance; + + get(url: string): Promise { + return this.axios.get(url); + } + post(url: string, data: unknown): Promise { + return this.axios.post(url, data); + } + // ... every axios method duplicated +} + +// GOOD: Deep module — simple interface hiding retry, auth, error mapping +class ListingApiClient { + async fetchListings(query: ListingQuery): Promise { + // Internally handles: auth headers, pagination, retry with backoff, + // rate limiting, response parsing, error mapping to domain errors + } + + async fetchListingDetails(id: string): Promise { + // Same concerns handled internally, caller doesn't know or care + } +} +``` + +--- + +## 4. SOLID Principles + +### Single Responsibility Principle (SRP) + +"A module should be responsible to one, and only one, actor." + +Not "do one thing" (that's for functions) — a *class* should have one reason to change, +meaning one stakeholder whose requirements could cause it to be modified. + +**Smell**: When you change a class for reason A and it breaks something related to +reason B, SRP is violated. + +#### Python + +```python +# BAD: This class changes for three different reasons +class Listing: + def calculate_price_per_sqm(self) -> float: ... # business logic + def to_json(self) -> str: ... # serialization + def save_to_database(self, db: Session) -> None: ... # persistence + +# GOOD: Each class has one reason to change +class Listing: + def calculate_price_per_sqm(self) -> float: ... + +class ListingSerializer: + def to_json(self, listing: Listing) -> str: ... + +class ListingRepository: + def save(self, listing: Listing) -> None: ... +``` + +### Open/Closed Principle (OCP) + +"Open for extension, closed for modification." + +You should be able to add new behavior without changing existing code. Use polymorphism, +strategy patterns, and plugin architectures. But don't over-engineer for hypothetical +extensions — apply when you see a concrete pattern of change. + +#### TypeScript + +```typescript +// BAD: Adding a new export format requires modifying this function +function exportListings(listings: Listing[], format: string): string { + if (format === "csv") { /* ... */ } + else if (format === "json") { /* ... */ } + else if (format === "geojson") { /* ... */ } + // Every new format = another branch here +} + +// GOOD: New formats are added by implementing an interface, not editing code +interface ListingExporter { + export(listings: Listing[]): string; +} + +class CsvExporter implements ListingExporter { + export(listings: Listing[]): string { /* ... */ } +} + +class GeoJsonExporter implements ListingExporter { + export(listings: Listing[]): string { /* ... */ } +} + +function exportListings(listings: Listing[], exporter: ListingExporter): string { + return exporter.export(listings); +} +``` + +### Liskov Substitution Principle (LSP) + +Subtypes must be substitutable for their base types without altering correctness. + +#### Python + +```python +# BAD: Violates LSP — Square changes the contract of Rectangle +class Rectangle: + def __init__(self, width: float, height: float) -> None: + self.width = width + self.height = height + + def area(self) -> float: + return self.width * self.height + +class Square(Rectangle): + def __init__(self, side: float) -> None: + super().__init__(side, side) + + # Setting width without height (or vice versa) breaks the square invariant. + # Code that expects Rectangle behavior will produce wrong results. + +# GOOD: Use a common interface without forcing an "is-a" relationship +from abc import ABC, abstractmethod + +class Shape(ABC): + @abstractmethod + def area(self) -> float: ... + +class Rectangle(Shape): + def __init__(self, width: float, height: float) -> None: + self.width = width + self.height = height + + def area(self) -> float: + return self.width * self.height + +class Square(Shape): + def __init__(self, side: float) -> None: + self.side = side + + def area(self) -> float: + return self.side ** 2 +``` + +### Interface Segregation Principle (ISP) + +No client should be forced to depend on methods it does not use. + +Prefer many small, focused interfaces over one fat interface. + +#### TypeScript + +```typescript +// BAD: Forces every listing source to implement methods it doesn't support +interface ListingSource { + fetchListings(query: Query): Promise; + fetchFloorplans(listingId: string): Promise; + calculateRoute(from: LatLng, to: LatLng): Promise; + sendNotification(listing: Listing): Promise; +} + +// GOOD: Segregated interfaces — implement only what you provide +interface ListingFetcher { + fetchListings(query: Query): Promise; +} + +interface FloorplanProvider { + fetchFloorplans(listingId: string): Promise; +} + +interface RouteCalculator { + calculateRoute(from: LatLng, to: LatLng): Promise; +} +``` + +### Dependency Inversion Principle (DIP) + +High-level modules should not depend on low-level modules. Both should depend on +abstractions. + +#### Python + +```python +# BAD: Business logic directly depends on infrastructure +from sqlalchemy.orm import Session + +class ListingService: + def __init__(self) -> None: + self.db = Session() # Hardcoded dependency on SQLAlchemy + + def get_affordable_listings(self, max_price: int) -> list[Listing]: + return self.db.query(Listing).filter(Listing.price <= max_price).all() + +# GOOD: Business logic depends on an abstraction +from abc import ABC, abstractmethod + +class ListingRepository(ABC): + @abstractmethod + def find_by_max_price(self, max_price: int) -> list[Listing]: ... + +class SqlAlchemyListingRepository(ListingRepository): + def __init__(self, session: Session) -> None: + self._session = session + + def find_by_max_price(self, max_price: int) -> list[Listing]: + return self._session.query(Listing).filter(Listing.price <= max_price).all() + +class ListingService: + def __init__(self, repository: ListingRepository) -> None: + self._repository = repository # Depends on abstraction, not SQLAlchemy + + def get_affordable_listings(self, max_price: int) -> list[Listing]: + return self._repository.find_by_max_price(max_price) +``` + +--- + +## 5. Error Handling + +> "Error handling is important, but if it obscures logic, it's wrong." +> — Robert C. Martin + +### Rules + +- **Fail fast**: Detect errors at the earliest possible point. A corrupted input should + be caught at the boundary, not three layers deep where it causes a cryptic failure. +- **Guard clauses at the top**: Validate preconditions first, return or raise early. + The happy path should be the least-indented code. +- **Exceptions for exceptional things**: Don't use exceptions for control flow. If a + user enters an invalid email, that's expected input — handle it with a validation + return, not a thrown exception. +- **Don't return null/None**: Returning null forces every caller to add a null check. + Return an empty collection, a Null Object, or use Optional types. +- **Don't pass null/None**: If a function doesn't expect null, passing it is a bug + waiting to happen. Fix the caller, don't add a null guard to every function. +- **Write informative error messages**: Include the context: what was being attempted, + what the actual value was, and what was expected. +- **Catch at the right level**: Catch exceptions where you can actually do something + meaningful about them. Don't catch-and-rethrow just to add a log line. + +### Python + +```python +# BAD: Returns None, vague error, catches too broadly +def fetch_listing(listing_id: str) -> dict | None: + try: + response = requests.get(f"{API_URL}/listings/{listing_id}") + return response.json() + except Exception: + logger.error("Something went wrong") + return None + +# GOOD: Specific exceptions, informative messages, no None return +class ListingNotFoundError(Exception): + pass + +class ListingFetchError(Exception): + pass + +def fetch_listing(listing_id: str) -> Listing: + try: + response = requests.get( + f"{API_URL}/listings/{listing_id}", + timeout=10, + ) + except requests.ConnectionError as e: + raise ListingFetchError( + f"Failed to connect to API while fetching listing {listing_id}" + ) from e + + if response.status_code == 404: + raise ListingNotFoundError( + f"Listing {listing_id} not found on Rightmove" + ) + + response.raise_for_status() + return Listing.from_api_response(response.json()) +``` + +```python +# BAD: Exception for control flow +def find_user_district(address: str) -> str: + try: + return district_lookup[address] + except KeyError: + raise DistrictNotFoundError(address) + +# Caller: +try: + district = find_user_district(address) +except DistrictNotFoundError: + district = "Unknown" + +# GOOD: Expected case handled with normal control flow +def find_user_district(address: str) -> str: + return district_lookup.get(address, "Unknown") +``` + +### TypeScript + +```typescript +// BAD: Returns undefined, caller must guess what went wrong +async function getListingDetails(id: string): Promise { + try { + const response = await fetch(`/api/listings/${id}`); + return await response.json(); + } catch { + return undefined; + } +} + +// GOOD: Typed errors, informative messages, never returns undefined +class ListingNotFoundError extends Error { + constructor(public readonly listingId: string) { + super(`Listing ${listingId} not found`); + this.name = "ListingNotFoundError"; + } +} + +async function getListingDetails(id: string): Promise { + const response = await fetch(`/api/listings/${id}`); + + if (response.status === 404) { + throw new ListingNotFoundError(id); + } + + if (!response.ok) { + throw new Error( + `Failed to fetch listing ${id}: ${response.status} ${response.statusText}`, + ); + } + + return (await response.json()) as ListingDetails; +} +``` + +```typescript +// BAD: Returns empty array for errors — caller can't distinguish "no results" from "failed" +async function searchListings(query: Query): Promise { + try { + return await api.search(query); + } catch { + return []; // Was this an empty search or a network failure? + } +} + +// GOOD: Use a Result type to distinguish success from failure +type Result = + | { ok: true; value: T } + | { ok: false; error: E }; + +async function searchListings(query: Query): Promise> { + try { + const listings = await api.search(query); + return { ok: true, value: listings }; + } catch (error) { + return { ok: false, error: error instanceof Error ? error : new Error(String(error)) }; + } +} +``` + +--- + +## 6. Code Structure and Dependencies + +### The Dependency Rule (Clean Architecture) + +Source code dependencies must point inward — from outer layers (frameworks, UI, DB) +toward inner layers (business rules, domain entities). Never let the core domain +know about infrastructure details. + +``` +Frameworks & Drivers → Interface Adapters → Use Cases → Entities + (outer) (inner) +``` + +### Separation of Concerns + +Each module, class, and function should address a single concern. UI code doesn't +calculate business rules. Database code doesn't format user-facing messages. + +### Composition Over Inheritance + +Favor composing objects over inheriting behavior. Inheritance creates tight coupling +and fragile hierarchies. Composition lets you swap behaviors at runtime and avoids +the "diamond problem." + +Use inheritance for genuine "is-a" relationships within a single conceptual hierarchy. +Use composition for "has-a" or "uses-a" relationships. + +#### Python + +```python +# BAD: Inheritance for code reuse — fragile, tightly coupled +class BaseProcessor: + def fetch(self): ... + def validate(self): ... + def save(self): ... + def notify(self): ... + +class ListingProcessor(BaseProcessor): + def process(self): + self.fetch() # Which fetch? What if I need a different one? + self.validate() + self.save() + self.notify() + +# GOOD: Composition — flexible, each piece is independent and swappable +class ListingProcessor: + def __init__( + self, + fetcher: ListingFetcher, + validator: ListingValidator, + repository: ListingRepository, + notifier: Notifier, + ) -> None: + self._fetcher = fetcher + self._validator = validator + self._repository = repository + self._notifier = notifier + + def process(self, listing_id: str) -> None: + listing = self._fetcher.fetch(listing_id) + self._validator.validate(listing) + self._repository.save(listing) + self._notifier.notify_new_listing(listing) +``` + +### Law of Demeter + +A method should only call methods on: +1. Its own object (`self`) +2. Objects passed as arguments +3. Objects it creates +4. Its direct component objects + +#### TypeScript + +```typescript +// BAD: Reaching through a chain of objects +function getListingCity(listing: Listing): string { + return listing.getProperty().getAddress().getCity(); +} + +// GOOD: Ask the object directly +function getListingCity(listing: Listing): string { + return listing.city; // Listing exposes what callers need +} +``` + +### Tell, Don't Ask + +Don't ask an object for data, make a decision, then tell it what to do. Instead, tell +the object to do the work. + +#### Python + +```python +# BAD: Ask, decide externally, then act +if listing.price is not None and listing.square_meters is not None: + if listing.square_meters > 0: + listing.price_per_sqm = listing.price / listing.square_meters + +# GOOD: Tell the object to do its own work +listing.calculate_price_per_sqm() # Listing owns this logic internally +``` + +--- + +## 7. Testing + +> "Code without tests is bad code. It doesn't matter how well written it is." +> — Michael Feathers + +### Kent Beck's First Rule of Simple Design + +**Pass all the tests.** Everything else follows. Untested code is uncertain code. + +### Rules + +- **Test behavior, not implementation**: Tests should verify *what* code does, not *how* + it does it. If you refactor internals and tests break, the tests are too coupled. +- **Arrange-Act-Assert**: Every test has three clear sections: set up the state, perform + the action, verify the outcome. One blank line between each. +- **One assertion per test** (as a guideline): Each test should verify one logical + concept. Multiple assertions are fine when they verify aspects of the same behavior. +- **Fast**: Tests that take seconds instead of milliseconds don't get run. Keep the + feedback loop tight. +- **Independent**: Tests must not depend on each other or on execution order. No shared + mutable state between tests. +- **Repeatable**: Same result every time, in any environment. No dependence on network, + clock, or file system state. +- **Descriptive test names**: The name IS the documentation. A failing test name should + tell you exactly what's broken without reading the test body. +- **Test the boundaries**: Zero, one, many. Empty, null, maximum. Off-by-one. Negative. + Boundary conditions are where bugs hide. +- **Use test doubles wisely**: Mock external dependencies (APIs, databases, clocks), + not internal collaborators. Over-mocking creates brittle tests that test nothing. + +### Python (pytest) + +```python +# BAD: Vague name, tests multiple things, no clear structure +def test_listing(): + listing = Listing(price=500000, square_meters=50) + assert listing.price_per_sqm == 10000 + listing2 = Listing(price=None, square_meters=50) + assert listing2.price_per_sqm is None + listing3 = Listing(price=500000, square_meters=0) + assert listing3.price_per_sqm is None + +# GOOD: One behavior per test, descriptive names, Arrange-Act-Assert +def test_price_per_sqm_divides_price_by_area(): + listing = Listing(price=500_000, square_meters=50) + + result = listing.calculate_price_per_sqm() + + assert result == 10_000 + +def test_price_per_sqm_returns_none_when_price_missing(): + listing = Listing(price=None, square_meters=50) + + result = listing.calculate_price_per_sqm() + + assert result is None + +def test_price_per_sqm_returns_none_when_area_is_zero(): + listing = Listing(price=500_000, square_meters=0) + + result = listing.calculate_price_per_sqm() + + assert result is None +``` + +```python +# BAD: Mocking internals — test is coupled to implementation +def test_fetch_listings(mocker): + mock_session = mocker.patch("requests.Session") + mock_response = mocker.Mock() + mock_response.json.return_value = [{"id": "1"}] + mock_session.return_value.get.return_value = mock_response + + result = fetch_listings(Query(min_price=1000)) + + mock_session.return_value.get.assert_called_once_with( + "https://api.rightmove.co.uk/api/rent/find", + params={"minPrice": 1000}, + ) + # This test breaks if you change the HTTP library, URL structure, or param names + +# GOOD: Mock at the boundary, test the behavior +def test_fetch_listings_returns_parsed_listings(fake_api: FakeRightmoveApi): + fake_api.add_listing(id="123", price=2000, bedrooms=2) + + listings = fetch_listings(Query(min_price=1000)) + + assert len(listings) == 1 + assert listings[0].id == "123" + assert listings[0].price == 2000 +``` + +### TypeScript (vitest/jest) + +```typescript +// BAD: "test1", no structure +test("test1", () => { + const svc = new ListingService(mockRepo); + expect(svc.getAffordable(3000).length).toBe(2); + expect(svc.getAffordable(3000)[0].price).toBeLessThanOrEqual(3000); +}); + +// GOOD: Descriptive name, clear AAA structure +describe("ListingService", () => { + describe("getAffordableListings", () => { + it("returns only listings at or below the price threshold", () => { + const repository = new InMemoryListingRepository([ + createListing({ price: 2000 }), + createListing({ price: 3000 }), + createListing({ price: 5000 }), + ]); + const service = new ListingService(repository); + + const results = service.getAffordableListings(3000); + + expect(results).toHaveLength(2); + expect(results.every((l) => l.price <= 3000)).toBe(true); + }); + + it("returns empty array when no listings are affordable", () => { + const repository = new InMemoryListingRepository([ + createListing({ price: 5000 }), + ]); + const service = new ListingService(repository); + + const results = service.getAffordableListings(1000); + + expect(results).toHaveLength(0); + }); + }); +}); +``` + +### The Testing Pyramid + +1. **Many unit tests**: Fast, isolated, testing individual functions and classes +2. **Some integration tests**: Verify components work together correctly +3. **Few end-to-end tests**: Verify critical user journeys through the whole system + +--- + +## 8. Kent Beck's Four Rules of Simple Design + +In priority order: + +1. **Passes all the tests** — Working software is the baseline. Nothing else matters if + the code doesn't work. +2. **Reveals intention** — Code should communicate its purpose clearly. Another developer + should understand *why* each piece exists. +3. **No duplication** — Every piece of knowledge should have a single, authoritative + representation. DRY applies to concepts, not just characters. +4. **Fewest elements** — Remove anything that doesn't serve rules 1-3. No speculative + generality. No unused abstractions. The simplest code that could work. + +### Python + +```python +# Violates Rule 2 (intention) and Rule 4 (fewest elements) +class AbstractListingFilterStrategyFactory: + def create_filter_strategy(self, filter_type: str) -> AbstractFilterStrategy: + ... + +# Follows all four rules — simple, clear, tested, no duplication +def filter_by_price(listings: list[Listing], max_price: int) -> list[Listing]: + return [l for l in listings if l.price <= max_price] +``` + +### TypeScript + +```typescript +// Violates Rule 3 (duplication of knowledge) +function formatRentPrice(price: number): string { + return `£${price.toLocaleString()} pcm`; +} +function formatBuyPrice(price: number): string { + return `£${price.toLocaleString()}`; +} + +// Follows all rules — one representation of "format price" +function formatPrice(price: number, suffix?: string): string { + const formatted = `£${price.toLocaleString()}`; + return suffix ? `${formatted} ${suffix}` : formatted; +} +``` + +--- + +## 9. Pragmatic Principles + +From Hunt & Thomas: + +- **DRY (Don't Repeat Yourself)**: Every piece of knowledge must have a single, + unambiguous, authoritative representation within a system. +- **Orthogonality**: Components should be independent. Changing one should not affect + others. If you change the database, the UI should not need to change. +- **Reversibility**: Don't bake in decisions that are hard to undo. Abstract external + dependencies so you can swap them. +- **Tracer Bullets**: Build a thin end-to-end slice first. Get something working across + all layers, then fill in the details. This reduces integration risk. +- **Broken Windows Theory**: Don't tolerate "broken windows" — bad designs, wrong + decisions, poor code. Fix them as you find them, or they multiply. + (This is Uncle Bob's Boy Scout Rule: "Leave the code cleaner than you found it.") +- **Good Enough Software**: Know when to stop. Don't over-polish at the expense of + shipping. Perfection is the enemy of "done" — but sloppy is the enemy of "maintainable". + +### Python — DRY violation and fix + +```python +# BAD: Validation logic duplicated in two places +class RentListingService: + def create(self, data: dict) -> RentListing: + if data["price"] <= 0: + raise ValueError("Price must be positive") + if not data["address"]: + raise ValueError("Address is required") + ... + +class BuyListingService: + def create(self, data: dict) -> BuyListing: + if data["price"] <= 0: + raise ValueError("Price must be positive") + if not data["address"]: + raise ValueError("Address is required") + ... + +# GOOD: Single source of truth for validation +class ListingValidator: + def validate(self, data: dict) -> None: + if data["price"] <= 0: + raise ValueError("Price must be positive") + if not data["address"]: + raise ValueError("Address is required") + +class RentListingService: + def __init__(self, validator: ListingValidator) -> None: + self._validator = validator + + def create(self, data: dict) -> RentListing: + self._validator.validate(data) + ... +``` + +### TypeScript — Orthogonality + +```typescript +// BAD: UI component knows about database schema +function ListingCard({ listing }: { listing: any }) { + const price = listing.price_in_pence / 100; // knows DB stores pence + const sqm = listing.floor_area_sqm ?? listing.epc_floor_area; // knows DB fallback logic + return
{price} — {sqm}m²
; +} + +// GOOD: UI receives a view model, doesn't know storage details +interface ListingCardProps { + displayPrice: string; + squareMeters: number | null; +} + +function ListingCard({ displayPrice, squareMeters }: ListingCardProps) { + return
{displayPrice} — {squareMeters}m²
; +} +``` + +--- + +## 10. Refactoring Discipline + +From Martin Fowler: + +- **Refactor continuously, not in big batches**: Small, frequent improvements integrated + into daily work beat scheduled "refactoring sprints." +- **Each refactoring is a tiny, behavior-preserving change**: Run tests after every + step. If a test breaks, undo and take a smaller step. +- **Code smells drive refactoring**: Don't refactor on a whim. Refactor when you smell: + +| Smell | Symptom | Refactoring | +|---|---|---| +| **Long Method** | Scrolling to read a function | Extract Method | +| **Feature Envy** | Method uses another class's data more than its own | Move Method | +| **Data Clumps** | Same group of fields travel together | Extract Class / parameter object | +| **Primitive Obsession** | Strings/ints for domain concepts | Replace with Value Object | +| **Shotgun Surgery** | One change touches many files | Move / Inline to consolidate | +| **Divergent Change** | One class changes for unrelated reasons | Extract Class (SRP) | +| **Switch on type** | `if isinstance` / `switch(type)` chains | Replace with Polymorphism | + +### Python — Primitive Obsession → Value Object + +```python +# BAD: Price is just a float, no domain meaning +def apply_discount(price: float, discount_pct: float) -> float: + return price * (1 - discount_pct / 100) + +# What currency? Can it be negative? Is discount 0-100 or 0-1? + +# GOOD: Value object encapsulates rules and meaning +@dataclass(frozen=True) +class Price: + amount_pence: int + currency: str = "GBP" + + def apply_discount(self, discount: Discount) -> "Price": + reduced = int(self.amount_pence * (1 - discount.as_fraction())) + return Price(amount_pence=max(0, reduced), currency=self.currency) + + def format(self) -> str: + pounds = self.amount_pence / 100 + return f"£{pounds:,.2f}" + +@dataclass(frozen=True) +class Discount: + percentage: int # 0-100 + + def __post_init__(self) -> None: + if not 0 <= self.percentage <= 100: + raise ValueError(f"Discount must be 0-100, got {self.percentage}") + + def as_fraction(self) -> float: + return self.percentage / 100 +``` + +### TypeScript — Extract Method + +```typescript +// BAD: Long function mixing abstraction levels +async function processListing(raw: RawListing): Promise { + // 15 lines of validation... + // 10 lines of geocoding... + // 20 lines of image processing... + // 10 lines of database insert... +} + +// GOOD: Each step is a named function at one level of abstraction +async function processListing(raw: RawListing): Promise { + const validated = validateListing(raw); + const geocoded = await geocodeAddress(validated.address); + const images = await processFloorplanImages(validated.imageUrls); + await saveListing({ ...validated, coordinates: geocoded, images }); +} +``` + +--- + +## 11. Working with Existing Code + +From Michael Feathers: + +- **Characterization tests first**: Before changing legacy code, write tests that + capture its current behavior — even if that behavior is wrong. Now you have a safety + net. +- **Find seams**: A seam is a place where you can alter behavior without editing the + code in that place. Constructor injection, method overriding, and configuration + switches are common seams. +- **The Strangler Fig Pattern**: When rewriting a system, wrap the old system with a new + interface and incrementally replace pieces. Don't do big-bang rewrites. +- **Make the change easy, then make the easy change**: If the code isn't structured for + the change you need, first refactor until the change is trivial, then make it. + +### Python — Characterization test + +```python +# Before modifying calculate_score(), lock down its current behavior +def test_calculate_score_characterization(): + """Captures existing behavior. If this fails after a refactor, investigate.""" + listing = Listing( + price=350_000, + square_meters=75, + bedrooms=2, + transit_minutes=25, + ) + + score = calculate_score(listing) + + # This is what the function currently returns — we're not asserting it's + # correct, we're asserting it hasn't changed unexpectedly. + assert score == 7.3 +``` + +### TypeScript — Seam via constructor injection + +```typescript +// Legacy code with hardcoded dependency (no seam) +class ListingImporter { + async import(): Promise { + const response = await fetch("https://api.rightmove.co.uk/listings"); + // ... + } +} + +// Introduce a seam: inject the dependency so tests can substitute it +class ListingImporter { + constructor(private readonly httpClient: HttpClient = new FetchHttpClient()) {} + + async import(): Promise { + const response = await this.httpClient.get("/listings"); + // ... + } +} + +// Now testable without hitting the network +const importer = new ListingImporter(new FakeHttpClient()); +``` + +--- + +## Pre-Flight Checklist + +Before submitting any code, mentally verify: + +- [ ] **Can I explain every name without referring to the implementation?** +- [ ] **Does each function do exactly one thing?** +- [ ] **Are there any functions longer than ~20 lines that could be decomposed?** +- [ ] **Am I mixing levels of abstraction in any function?** +- [ ] **Are dependencies pointing inward (toward the domain)?** +- [ ] **Would a new team member understand this code without me explaining it?** +- [ ] **Are error cases handled at the right level with informative messages?** +- [ ] **Is there any duplication that represents duplicated knowledge (not coincidence)?** +- [ ] **Could I remove any code and still pass all tests?** (If yes, remove it.) +- [ ] **Have I left the code cleaner than I found it?** + +--- + +## References + +- Robert C. Martin, [*Clean Code: A Handbook of Agile Software Craftsmanship*](https://www.oreilly.com/library/view/clean-code-a/9780135398586/) (2008) +- Robert C. Martin, *Clean Architecture* (2017) +- John Ousterhout, *A Philosophy of Software Design* (2018) +- Kent Beck, *Four Rules of Simple Design* (late 1990s, XP) +- Martin Fowler, *Refactoring: Improving the Design of Existing Code* (1999, 2nd ed. 2018) +- Andrew Hunt & David Thomas, *The Pragmatic Programmer* (1999, 20th Anniversary Ed. 2019) +- Michael Feathers, *Working Effectively with Legacy Code* (2004) +- [Clean Code Summary (GitHub Gist)](https://gist.github.com/wojteklu/73c6914cc446146b8b533c0988cf8d29) +- [Clean Architecture Summary (GitHub Gist)](https://gist.github.com/ygrenzinger/14812a56b9221c9feca0b3621518635b) +- [A Philosophy of Software Design - Review](https://www.mattduck.com/2021-04-a-philosophy-of-software-design.html) +- [Kent Beck's 4 Rules of Simple Design](https://DEV.to/jmir17/the-4-rules-of-simple-design-14eo)