The two previous lessons answered "is it well made?" from two angles: what to do and what to avoid. The third is missing, and it produces no errors at all yet decides the project's future: how it reads.

A RentalService can pass all thirty-eight checks from 10-01, contain not one of the mistakes from 10-02 and still be a two-hundred-line method with three boolean flags, variables called data and temp, comments that repeat what the code already says and a domain that is a bag of setters. None of that fails today. What fails is six months from now, when Ribalta council asks for dynamic fares and nobody dares touch that method.

This lesson is about that, with a single criterion that runs through every section and is worth fixing before we start: the audience for the code is not the compiler, it is the next person who reads it. And that next person is frequently you, a year from now, remembering nothing.

1. What clean code means here
2. Names
3. Functions and classes
4. A complete refactoring: RentalService
5. SOLID in Spring, with examples from Ribalta
6. Comments
7. Error handling
8. Immutability
9. Optional used well
10. The anaemic domain model
11. Verifiable architecture with ArchUnit
12. Style and automation
13. Safe refactoring
14. Technical debt
15. Common Mistakes and Tips
16. Exercises

"Clean code" is often confused with "pretty code", and they are not the same thing. An operational criterion, one you can apply without arguing about taste:

The fourth is the most objective and the most informative. If testing a method requires mocking static methods, instantiating six collaborators or manipulating the system clock, the problem is not the test: it is the design. It is the reason we injected a Clock back in 02-01 and the reason RentalSecurity is an ordinary bean instead of a three-line SpEL expression. And a warning about scope: nothing in this lesson is a matter of personal style — indentation, braces and line length are settled by a tool in section 12 and are not argued about in reviews. What follows is design.

Naming is the most frequent activity in programming and the one with the highest return per minute invested.

CicloUrbana has an explicit rule worth stating because it is not universal: the domain is named in the council's own words — Station, Bike, Rental, calculateAmount, availableBikes — and technical vocabulary is reserved for what belongs to the framework or a library: annotations, Spring types, findById, Pageable. The reason is not aesthetic, it is ubiquitous language: Ribalta council talks about stations, docks and fares, and when the code uses those same words, the mental translation between the conversation and the program disappears. What must be avoided at all costs is the hybrid: StationService.getStationRowByLabel() makes you switch vocabulary twice in a single line.

The difference between a name and a good name is whether it forces you to read the implementation.

getData() fails on three fronts: it does not say what data, it does not say where it comes from and it does not say by what criterion it is filtered. findWithAvailability(int minimumBikes) answers all three, and the parameter name turns the number at the call site into information: findWithAvailability(3) reads as "stations with at least three bikes".

Four practical rules: no abbreviations except the universally known ones (id, url, http); no numbers in names, because process1 and process2 mean you do not know what distinguishes them; the name indicates the return type — anything starting with is or can returns a boolean, anything starting with find may not find anything and returns an Optional or a list —; and the same concept, always the same word: if it is find, it is not get in the class next door.

The most useful rule about the size of a function is not "under twenty lines", but: every statement in a method must be at the same level of detail.

The public method reads like a paragraph and is understood in five seconds. Whoever needs the detail goes down a level; whoever only wants to know what happens, does not.

Zero, one or two arguments is ideal; three is acceptable if they belong to the same concept; with four or more there is almost always an object missing to group them.

A boolean flag is an argument that makes the method do two different things, and it has three defects: nothing can be read at the call site (finish(9L, true) does not say what true is), it forces an if inside that separates two flows which are already two methods, and it grows: tomorrow there are two flags and four combinations, two of which make no sense.

"One class, one responsibility" is quoted a lot and applied badly, because "responsibility" is vague. The operational formulation is better: a class must have a single reason to change, that is, a single stakeholder who might ask for it to be modified. If RentalService changes when the council revises the fares, when billing changes the invoice format and when the mail provider changes its API, it has three reasons to change and three different people asking for them. The next section fixes it.

We start from a real, plausible version of RentalService.finish: it works, it passes the tests and it does too much.

The five responsibilities are five different reasons to change this class, and each has a different stakeholder: validating the finishing rules (the council), calculating the amount (the fares department), applying the overtime surcharge (the council, in another meeting), mutating the state of the rental and the bike (the domain team) and notifying and measuring (marketing and operations).

The if/else if/else reimplements what FareSelector and the three FareCalculator implementations from 02-02 already did. It is pure duplication, and worse: a second source of truth that can diverge from the first. The fifteen lines become fareSelector.forUser(rental.getUser()).calculate(duration).

The if (minutes > 120) is a business rule hidden inside a service. Since the fare system is already extensible by design, the surcharge fits as a decorator:

Now the surcharge is tested on its own, combines with any present or future fare and is configured through properties. RentalService stops knowing it exists.

The six consecutive setters are the symptom of the anaemic model in section 10. The "finish a rental" operation belongs to Rental, which is what knows its invariants:

Note what you gain: the "already finished" check stops depending on the service remembering it. Any path that calls finish applies it.

The email and the metric are moved out to an AFTER_COMMIT event, as we already established in Transactions and Scheduled Tasks and Asynchrony.

And the condition without which none of this gets done: the module 6 tests were green before we started and stayed green after every step. Without them, this refactoring is a blind rewrite.

Two misunderstandings worth clearing up. DIP does not mean "one interface per class": it means the dependency points at the abstraction when there is a real boundary. RentalService depends on FareCalculator because there are three implementations and there will be more; an IRentalService with a single implementation is ceremony with no benefit. And LSP is the most ignored and the one that produces the strangest failures: it is typically violated not through inheritance but through the contract — an implementation that returns null where the others return an empty list, or that throws where the others return zero — and the consumer, written against the first one's behaviour, breaks with the second.

A comment is a maintenance debt: the compiler does not check it, no test runs it and it ages silently. That is why the criterion is demanding.

The javadoc worth writing is the one on boundaries: public interfaces, domain contracts and any method whose behaviour is not obvious at the edge cases — FareCalculator.calculate documents what happens with a zero duration, because that is not in the signature. By contrast, a /** Returns the name. @return the name */ above getName() only adds lines.

Domain-specific exceptions, not generic ones. throw new RuntimeException("Error") forces whoever catches it to read the message to find out what happened. The hierarchy from 03-06 — CicloUrbanaException with ResourceNotFoundException, ResourceConflictException, StationFullException and BikeUnavailableException — lets the @RestControllerAdvice translate each one into its HTTP code without a single if on strings.

Do not use exceptions for the normal flow. "There are no bikes available" at a busy station is not exceptional: it happens a hundred times a day. An exception costs you building the stack trace and, above all, communicates the wrong thing to the reader; if the case is expected, return an Optional or a result type. And do not catch Exception: it also traps what you do not know how to handle and turns it into a log line. Catch what you can deal with, and let the rest rise.

Useful messages. "Error" is worth nothing; "Station 3 is full: 18 bikes for 18 docks" says what happened, with which data, and lets you reproduce it. With one condition you must not forget: the message that goes to the client and the one that goes to the log are not the same. Detail to the log; to the client, the version with no internal information.

An immutable object cannot be in an invalid state after construction, is thread-safe without any reasoning and cannot change between the moment it is validated and the moment it is used — a real source of vulnerabilities.

record for DTOs and value objects. Every CicloUrbana DTO is one, with the compact constructor as the natural place to normalise; and so are the domain's value objects, such as LocationResponse. Immutable collections in return values, because returning the internal list lets the caller modify it behind your back:

And the honest limit: JPA entities cannot be immutable. Hibernate needs a no-argument constructor and modifies state when loading and when applying changes. The answer is not to force immutability but to control the mutation: private fields, no indiscriminate public setters, and methods with business names — finish, dockAt, markAsBroken — that are the only things that change the state and that can protect the invariants.

Optional was introduced for one specific case — the return value of a method that may not find anything — and is frequently used for three more, where it gets in the way.

And the most frequent antipattern of all: .get() without checking, which turns an expected case into a NoSuchElementException with a 500 and a stack trace in the log.

orElseThrow, orElseGet, map, filter and ifPresent cover practically everything. One detail that gets overlooked: orElse always evaluates its argument, even when there is a value, so if building the alternative is expensive it must be orElseGet with a lazy supplier.

An anaemic model is one in which the entities hold only data — fields, getters and setters — and all the logic lives in the services. It is the default style of most Spring projects, and it deserves an honest discussion because it is not always wrong.

The case for putting behaviour in the domain. Compare the two ways of finishing a rental:

Four differences that matter. The invariant travels with the data: the "already finished" check is applied on every path, not only the one that remembered to write it. The entity cannot be left half done: with public setters, a method can set the amount and forget the status, and nothing prevents it. The name is in the council's language, not the database's. And it can be tested with nothing at all: Rental.finish is a one-millisecond unit test.

The case for the anaemic model exists and should not be dismissed:

CicloUrbana's practical criterion, which is a defensible middle ground: rules that depend only on the state of the aggregate itself go in the entity — finish, canBeRented, isFull, dockAt —; those that need collaborators or cross aggregates go in the service — selecting the fare, verifying there is no other open rental, checking permissions. With that division, Rental knows no repository and RentalService does not manipulate state with setters.

The dependency rules from 10-01 — the controller does not touch the repository, the domain does not import the framework — are agreements that get broken sooner or later if they depend on somebody noticing during a review. ArchUnit turns them into tests.

With the dependency com.tngtech.archunit:archunit-junit5:1.3.0 in test scope, the rules are written as static fields annotated with @ArchTest:

Five rules, one file, and from that moment on ./mvnw test turns red the day somebody breaks them, with a message that says which class, which dependency and — thanks to the because — why it is forbidden. It is the difference between a documented convention and a convention that is actually followed. Three tips so the investment does not backfire: start with three or four rules, the ones that genuinely hurt when broken; always write the because, because the failure message is the only thing whoever breaks it a year from now will see; and if a rule needs exceptions, declare them explicitly instead of deleting the whole rule.

Style — where the braces go, how many spaces, the order of the imports, the line length — is the discussion with the worst ratio of time invested to value obtained in the whole of software engineering. The solution is not to agree on a style: it is to delegate it to a tool.

./mvnw spotless:apply formats the whole project and ./mvnw spotless:check fails if anything is not, which is what the pipeline from 08-05 runs on every change.

Why style is not argued about in reviews. A comment saying "two spaces missing" consumes the attention that should go to the logic, generates friction and adds nothing a tool does not do for free. With Spotless in validate, the code arrives already formatted and the review deals with what no tool detects: whether the design is correct, whether the name reveals the intent and whether an edge case is missing. And the order matters when adopting it: first apply the formatter to the whole project in a dedicated commit, with no functional change, and only then switch the check on; mixing reformatting and logic produces unreviewable diffs.

Refactoring is changing the internal structure without changing the observable behaviour, and the definition contains its own requirement: to know the behaviour has not changed, you have to be able to check it.

The four rules, in order of importance:

1. If there are no tests, the first task is to write them. Not for the whole system: for the behaviour you are about to move. They are called characterisation tests and they assert what the code does today, quirks included. Without them you are not refactoring, you are rewriting.
2. One step at a time, with the tests in between. The refactoring in section 4 was four steps, each with the suite green. If step 3 broke something, you would know exactly which one it was.
3. Do not mix refactoring and functionality in the same commit. A change that moves two hundred lines and also fixes a bug is unreviewable: nobody can separate the movement from the change.
4. Trust the IDE for the mechanical parts. Rename, extract method or class, change signature and introduce parameter are transformations the IDE performs without making mistakes; doing them with find-and-replace is where the errors appear.

When to refactor. The sustainable answer is not "we'll set aside a sprint": it is the campsite rule, leaving the code a little better than you found it every time you touch it for another reason. A refactoring sprint competes with features and always loses; a ten-minute improvement inside a task you were doing anyway competes with nothing.

The metaphor is exact, and that is why it works: you borrow time today and pay interest every time you touch that code. As with real finance, there is reasonable debt and ruinous debt.

How to recognise it. Four signals: an estimate that grows without the requirement growing; a file that turns up in 80% of the commits; a part of the code that "only so-and-so touches"; and the phrase "don't touch it, it works", an explicit admission that nobody understands it.

How to record it. A // TODO with no date or owner is decoration: six months later there are a hundred and forty and nobody reads them. What does work:

The difference from a TODO is that it has an owner, a cost and an impact, which is the information needed to prioritise it against a feature; and it comes with a ticket in the same system where the rest of the work lives, because debt that exists only in the code never gets planned.

When to pay it. Three moments, and none of them is "when there is time": when you are going to touch that area for another reason — the interest pays for itself —; when it blocks something the council actually wants; and when the cost of the interest exceeds the cost of paying it off, which is detected by measuring, not by arguing. Debt that bothers nobody can stay: not everything improvable deserves to be improved.

Confusing clean code with "elegant" code. A chain of five nested stream()s can be very clever and unreadable. If you have to read it three times, a loop with clear names is better code.

Refactoring without tests. That is rewriting under another name. If there are no tests, write them first: the characterisation ones first, then the change. And applying SOLID as a ritual — one interface per class, one mapper per DTO, one factory per service — is not design: it is ceremony. Every abstraction has to be paying for something concrete.

Commenting what could be named. If you need a comment to explain what a block does, what you almost always need is to extract that block into a method with that name. And leaving commented-out code "just in case" only raises doubts about whether it should be active: it is in version control.

Confusing the rich model with putting repositories inside entities. An entity that injects a repository is worse than the anaemic model: it mixes persistence and domain and makes it impossible to test in isolation.

Tip: the best readability test is reading it out loud. If rental.finish(destination, endedAt, totalAmount) reads like a sentence from the council and process(a, true, 2) does not, you already have the answer without arguing about style.

Tip: write the code thinking about whoever will delete it. Code that is easy to delete — clear boundaries, few incoming dependencies — is the same code that is easy to change: if removing SeniorFare means deleting one class, the design is good.

Tip: code review deals with what no tool can see. Spotless looks at the formatting, ArchUnit at the dependencies, SpotBugs at likely bugs and JaCoCo at coverage. What is left for people is whether the name reveals the intent, whether the abstraction is the right one and whether an edge case is missing. That is exactly where a review adds value.

Refactor this class applying what we have covered: names, level of abstraction, boolean flags, Optional, error handling and the domain model. Justify each change.

Write four ArchUnit rules that protect decisions taken throughout the course, different from the five in section 11. For each one, state which decision it protects, in which lesson it was taken and what message it would give on failure.

For each of these six CicloUrbana business rules, decide whether it should live in the entity or in the service, and justify it with the criterion from section 10: (1) a bike with less than 20% battery cannot be rented; (2) a user cannot have two open rentals at once; (3) a station is full when its docked bikes equal its capacity; (4) the amount depends on the user's fare type; (5) a finished rental cannot be finished again; (6) only an operator can mark a bike as broken.

Solution 1

The ten changes, justified.

And the deep change: the closing logic has moved into Incident, where the "it is not closed twice" invariant is applied on every path.

Solution 2

Why these four and not others: all four protect decisions that get broken through carelessness and produce no immediate error. An ArchUnit rule about something that already fails at compile time adds nothing; its value lies exactly in the silent agreements. And the because is not decorative: it is the only thing whoever breaks it in two years' time will see, so it should carry the concrete reason or the lesson, not a "this is forbidden".

Solution 3

The pattern that emerges, and which is the exercise's answer: the rule lives in the entity when it can be evaluated with what the entity already has in front of it. As soon as you need to consult something else — another aggregate, a service, the authenticated user — it moves up to the service. Case 1 usually generates debate, because you could argue that the threshold is configuration and therefore the rule belongs to the service; passing it as an argument keeps the entity clean and the rule next to the data. When in doubt, the useful question is: can I test this rule with a new and nothing else? If the answer is yes, it can live in the entity.

CicloUrbana's code no longer just works and avoids the known mistakes: it can be read. And the criterion that governs everything is a single one, the one that opened the lesson: the audience is not the compiler, it is the next person who reads it, with the most objective property — testability — as the thermometer: if testing something requires acrobatics, the problem is the design.

You know how to name things with the course's convention — the domain in the council's words, the framework's vocabulary only where the framework lives, no hybrids — and how to tell getData() from findWithAvailability(int minimumBikes), which answers what, from where and by what criterion. You write functions with a single level of abstraction, where the public method tells the story and the private ones fill in the detail, and you know why a boolean flag is really two methods that have not been separated yet. You have seen the complete refactoring of RentalService.finish in four steps — extracting the calculation to FareSelector, turning the surcharge into the FareWithOvertimeSurcharge decorator, moving the behaviour into Rental.finish(...) and pushing the side effects out to AFTER_COMMIT — from thirty-eight lines and five reasons to change to eleven lines and one, with the non-negotiable condition of having the tests green before and after every step.

You have SOLID grounded in Ribalta with its two misunderstandings cleared up — DIP is not one interface per class, and LSP is almost always violated by contract rather than by inheritance —; the criterion for comments, which document the why and never the what; error handling with domain exceptions, without using them for the normal flow, without catching Exception and with different messages for the log and for the client; immutability with records and read-only collections, and its honest limit in JPA entities, where the answer is not immutability but controlled mutation through methods with business names; and Optional in its one correct place, the return value, with orElseThrow replacing the .get() that turns a 404 into a 500. And you have the honest discussion about the anaemic model, with CicloUrbana's criterion as a defensible middle ground: rules that depend only on the state of the aggregate itself go in the entity, those that need collaborators or cross aggregates go in the service.

Closing the lesson are the three pieces that turn all of the above into something that holds itself up: ArchUnit, which turns the dependency rules into tests that go red with a message explaining why; Spotless, Checkstyle and static analysis, which take style out of reviews so that reviews can deal with what no tool can see; and safe refactoring supported by module 6, with the campsite rule as the sustainable way to improve and with technical debt recorded with an owner, a cost and an impact instead of with decorative TODOs — and with explicit permission to leave unpaid the debt that bothers nobody.

That closes the reflection on how CicloUrbana is made. Across ten modules we have built the Ribalta network layer by layer and we have never looked at it whole. The lesson Final Project: A Complete Tour of CicloUrbana does exactly that: the final architecture in one diagram, the complete repository structure, the journey of a POST /api/v1/rentals step by step from the load balancer to the metric — citing at each step the lesson where it was studied —, the map of what each module built, the design decisions with their alternatives and trade-offs, the final pom.xml and application.yml annotated, how to get the whole project running from scratch, and where to take it next.