Java Guild course · Lesson 0005
Lesson 0005 · the second intermediate requirement
A delimiter that appears in the data, a parser with no validation, and a load that destroys the library before it fails.
The save/load code works — for well-behaved input. The Intermediate requirement asks for persistence, and the starter has persistence, so it is tempting to tick it and move on. Three bugs say otherwise, and all three are the kind that reach production.
Create src/test/java/com/solirius/intermediate/library/LibraryPersistenceTest.java.
Five tests, one per defect plus two controls:
package com.solirius.intermediate.library;
import com.solirius.intermediate.library.exceptions.LibraryStorageException;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
@DisplayName("Saving and reloading the library")
class LibraryPersistenceTest {
@TempDir
private Path directory;
@Test
@DisplayName("preserves a title that contains the field separator")
void preservesATitleContainingTheSeparator() throws Exception {
Path file = directory.resolve("state.txt");
Library saved = new Library();
saved.addBook(new Book("Coming Home; A Novel", "Rosamunde Pilcher"));
saved.saveLibraryState(file.toString());
Library reloaded = new Library();
reloaded.loadLibraryState(file.toString());
Book book = reloaded.viewAvailableBooks().get(0);
assertEquals("Coming Home; A Novel", book.getTitle());
assertEquals("Rosamunde Pilcher", book.getAuthor());
}
@Test
@DisplayName("preserves a title that contains the escape character")
void preservesATitleContainingTheEscapeCharacter() throws Exception {
Path file = directory.resolve("state.txt");
Library saved = new Library();
saved.addBook(new Book("AC\\DC: The Story", "Murray Engleheart"));
saved.saveLibraryState(file.toString());
Library reloaded = new Library();
reloaded.loadLibraryState(file.toString());
assertEquals("AC\\DC: The Story",
reloaded.viewAvailableBooks().get(0).getTitle());
}
@Test
@DisplayName("keeps a book borrowed across a save and reload")
void keepsABookBorrowedAcrossASaveAndReload() throws Exception {
Path file = directory.resolve("state.txt");
Library saved = new Library();
saved.addBook(new Book("Dune", "Frank Herbert"));
saved.borrowBook("Dune");
saved.saveLibraryState(file.toString());
Library reloaded = new Library();
reloaded.loadLibraryState(file.toString());
assertTrue(reloaded.viewAvailableBooks().isEmpty());
assertTrue(reloaded.searchBook("Dune").orElseThrow().isBorrowed());
}
@Test
@DisplayName("rejects a corrupt file instead of crashing on an array index")
void rejectsACorruptFile() throws Exception {
Path file = directory.resolve("state.txt");
Files.write(file, List.of("Dune;Frank Herbert"));
LibraryStorageException thrown = assertThrows(LibraryStorageException.class,
() -> new Library().loadLibraryState(file.toString()));
assertEquals("Corrupt library state at line 1: "
+ "expected 3 fields but found 2", thrown.getMessage());
}
@Test
@DisplayName("leaves the library untouched when the file cannot be read")
void leavesTheLibraryUntouchedWhenTheFileCannotBeRead() {
Library library = new Library();
library.addBook(new Book("Dune", "Frank Herbert"));
assertThrows(LibraryStorageException.class,
() -> library.loadLibraryState(
directory.resolve("absent.txt").toString()));
assertEquals(1, library.viewAvailableBooks().size(),
"a failed load must not destroy the books already held");
}
}
throws Exception, not throws IOException
Same trap as Lesson 0004.
saveLibraryState declares IOException today, but you
are about to change it, and then:
LibraryPersistenceTest.java:[23,31] unreported exception
LibraryStorageException; must be caught or declared to be thrown
cd ~/Desktop/java-guild-candidate-test-intermediate-main
mvn clean test
preservesATitleContainingTheSeparator
expected: <Coming Home; A Novel> but was: <Coming Home>
rejectsACorruptFile
Unexpected exception type thrown, expected: <...LibraryStorageException>
but was: <java.lang.ArrayIndexOutOfBoundsException>
leavesTheLibraryUntouchedWhenTheFileCannotBeRead
a failed load must not destroy the books already held
expected: <1> but was: <0>
Three failures, three distinct defects. The fix is one coherent rewrite of the
persistence half of Library, so the steps below build it in order rather
than defect by defect.
LibraryStorageExceptionCreate
src/main/java/com/solirius/intermediate/library/exceptions/LibraryStorageException.java:
package com.solirius.intermediate.library.exceptions;
/**
* Exception thrown when the saved library state cannot be read or written.
*/
public class LibraryStorageException extends LibraryException {
/**
* Constructs a new LibraryStorageException with the specified message.
*
* @param message the detail message
*/
public LibraryStorageException(final String message) {
super(message);
}
/**
* Constructs a new LibraryStorageException with a message and a cause.
*
* @param message the detail message
* @param cause the underlying failure
*/
public LibraryStorageException(final String message, final Throwable cause) {
super(message);
initCause(cause);
}
}
extends LibraryException, and the objection
It means the console still catches one type for every failure it
can report. Without it, Main needs a second catch block around load and
save.
The purist objection is real and you should raise it before an interviewer does: storage failure is infrastructure, not domain. Bundling them means a caller cannot distinguish “your input was wrong” from “the disk is broken”.
The defence: in this application the response is identical — tell the user, keep running — and the subtype still exists, so any caller that needs the distinction can catch it. Saying both halves shows you understood a trade-off rather than stumbled into one side of it.
Note the two-argument constructor takes a cause. Never swallow the
original IOException; chain it, so a stack trace still shows the real
disk error underneath.
/** Field separator used by the flat-file format. */
private static final String SEPARATOR = ";";
/** Escape character used to protect separators inside a field. */
private static final String ESCAPE = "\\";
/** Number of fields on every well-formed line. */
private static final int FIELD_COUNT = 3;
Two constants, used by the save, the load and the validation. A disagreement between any two of them is a silent round-trip bug, so they are named once.
The problem in one sentence: you are using ; as a separator,
and ; can appear in the data. There are two families of fix.
Escape the separator on the way out — ; becomes \; —
and unescape on the way in. The write side is four lines and looks harmless:
private static String escape(final String field) {
return field.replace(ESCAPE, ESCAPE + ESCAPE)
.replace(SEPARATOR, ESCAPE + SEPARATOR);
}
The read side is where the cost lands. You cannot use
String.split(";") any more — it has no idea that \; means
“a literal semicolon” — so you hand-roll a character-by-character state
machine: 21 further lines, a boolean escaped flag, and a
final fields.add() after the loop that is easy to forget.
escape and splitEscaped
are one algorithm written twice in opposite directions. Nothing in the compiler
checks they agree; only a round-trip test does.If you started down this road and your test now says
expected <Coming Home; A Novel> but was <Coming Home\>, that is
the diagnosis: you added escape on the write side and left plain
split(";") on the read side. The trailing backslash in the actual value
is the escape character that nothing consumed.
Instead of teaching the parser about escapes, encode each field so it cannot contain the separator in the first place. The JDK already does this:
/**
* Encodes a field so that it cannot contain the separator.
* <p>
* Percent-encoding is delegated to the JDK rather than hand-rolled: it
* makes every character legal in every field, including the separator
* itself, and it is symmetric with {@link #decode(String)} by
* construction.
* </p>
*
* @param field the raw field value
* @return the field, safe to place between separators
*/
private static String encode(final String field) {
return URLEncoder.encode(field, StandardCharsets.UTF_8);
}
/**
* Reverses {@link #encode(String)}.
*
* @param field the encoded field value
* @return the original text
*/
private static String decode(final String field) {
return URLDecoder.decode(field, StandardCharsets.UTF_8);
}
That is the entire mechanism. URLEncoder percent-encodes anything that
is not alphanumeric — including ; — so a plain
split(SEPARATOR, -1) is now correct, and parseLine just
decodes each field:
/**
* Parses one line of the saved format into a book.
*
* @param line the raw line
* @param lineNumber the 1-based line number, used in error messages
* @return the book the line describes
* @throws LibraryStorageException if the line is not well formed
*/
private Book parseLine(final String line, final int lineNumber)
throws LibraryStorageException {
String[] fields = line.split(SEPARATOR, -1);
if (fields.length != FIELD_COUNT) {
throw new LibraryStorageException("Corrupt library state at line "
+ lineNumber + ": expected " + FIELD_COUNT
+ " fields but found " + fields.length);
}
Book book = new Book(decode(fields[0]), decode(fields[1]));
if (Boolean.parseBoolean(fields[2])) {
book.borrowBook();
}
return book;
}
| Hand-rolled escaping | Percent-encoding | |
|---|---|---|
| Lines of code | 25 | 6 |
| Read side | 21-line state machine | split(";", -1) |
| Ordering trap | Yes | None |
| Symmetry | Two methods you must keep in step by hand | Guaranteed — they are a documented inverse pair |
| Dependency | None | None — java.net, in the JDK since 1.0 |
Verified across eight awkward inputs, all round-tripping correctly: a title with
five semicolons, a title ending in a backslash, an empty title, accented
characters, a literal %, and a literal +.
This is the one real cost, and you should raise it before an interviewer does:
escaped: Coming Home\; A Novel;Rosamunde Pilcher;false
encoded: Coming+Home%3B+A+Novel;Rosamunde+Pilcher;false
Still perfectly greppable and debuggable, but a human skimming it has to decode
%3B and + in their head. You are trading a little
human readability for a large amount of correctness and 19 fewer lines. For a
machine-written state file that is the right trade; for a config file people edit by
hand it would not be.
URLEncoder.encode(String, Charset) needs Java 10+,
so it is fine on this project’s Java 11 target. The older
encode(String, String) overload throws a checked
UnsupportedEncodingException — do not use it.
“The delimiter could appear in the data, so I had two options: teach the
parser about escape sequences, or make the delimiter unrepresentable. I picked the
second — percent-encode each field with URLEncoder — because the
escaping version needs a hand-written splitter that has to stay exactly in step with
the escaper, and nothing but a test enforces that. Delegating to the JDK made it six
lines instead of twenty-five, and the encode/decode pair is symmetric by
construction. The cost is that the state file is less pleasant to read by eye.”
That is a simple solution to a complex problem, which is exactly what the evaluation criteria are looking for. Note the shape of the answer: you named the alternative, said why you rejected it, and volunteered the downside.
saveLibraryState /**
* Saves the state of the library to a file.
*
* @param filePath the path to save the file to
* @throws LibraryStorageException if the state cannot be written
*/
public void saveLibraryState(final String filePath)
throws LibraryStorageException {
try (BufferedWriter writer =
new BufferedWriter(new FileWriter(filePath))) {
for (Book book : books) {
writer.write(encode(book.getTitle()) + SEPARATOR
+ encode(book.getAuthor()) + SEPARATOR
+ book.isBorrowed());
writer.newLine();
}
} catch (IOException e) {
throw new LibraryStorageException(
"Could not save library state to " + filePath, e);
}
}
Two changes: every field goes through escape(), and the
IOException is caught and re-thrown as a LibraryStorageException
that names the file.
loadLibraryState and add parseLine /**
* Loads the state of the library from a file.
* <p>
* The current contents are replaced only once the whole file has been
* read successfully, so a failed load never destroys the books already
* held in memory.
* </p>
*
* @param filePath the path to load the file from
* @throws LibraryStorageException if the state cannot be read or is corrupt
*/
public void loadLibraryState(final String filePath)
throws LibraryStorageException {
List<Book> loaded = new ArrayList<>();
try (BufferedReader reader = new BufferedReader(
new FileReader(filePath))) {
String line;
int lineNumber = 0;
while ((line = reader.readLine()) != null) {
lineNumber++;
loaded.add(parseLine(line, lineNumber));
}
} catch (IOException e) {
throw new LibraryStorageException(
"Could not read library state from " + filePath, e);
}
books.clear();
books.addAll(loaded);
}
/**
* Parses one line of the saved format into a book.
*
* @param line the raw line
* @param lineNumber the 1-based line number, used in error messages
* @return the book the line describes
* @throws LibraryStorageException if the line is not well formed
*/
private Book parseLine(final String line, final int lineNumber)
throws LibraryStorageException {
List<String> fields = splitEscaped(line);
if (fields.size() != FIELD_COUNT) {
throw new LibraryStorageException("Corrupt library state at line "
+ lineNumber + ": expected " + FIELD_COUNT
+ " fields but found " + fields.size());
}
Book book = new Book(fields.get(0), fields.get(1));
if (Boolean.parseBoolean(fields.get(2))) {
book.borrowBook();
}
return book;
}
The original did books.clear() first, then opened the
file. So a missing or corrupt file destroyed the in-memory library and the exception
arrived too late to matter:
library.addBook(new Book("Dune", "Frank Herbert"));
library.loadLibraryState("/nonexistent/nope.txt"); // throws
library.viewAvailableBooks().size(); → 0
The new version builds loaded completely — every line parsed, every
chance to throw already taken — and only then does:
books.clear();
books.addAll(loaded); // only reached on success
This is the strong exception guarantee: if an operation throws, the object is left in the state it started in. Name it in interview. Most candidates fix the ordering by instinct without knowing there is a term for it, and naming it is the difference between “I moved a line” and “I gave this method a guarantee”.
parseLine
parts[2] with no length check became
ArrayIndexOutOfBoundsException: Index 2 out of bounds for length 2 — a
message that tells the user nothing. The replacement validates the count and reports
what was wrong and where:
Corrupt library state at line 1: expected 3 fields but found 2
The line number is the part that matters. An exception naming the line is one somebody can act on: they open the file, go to that line, and see the problem.
MainTwo catch blocks — the load at startup and the save on exit — still say
IOException:
// startup
try {
library.loadLibraryState(filePath);
System.out.println("Library state loaded successfully.");
} catch (IOException e) {
} catch (LibraryException e) {
System.out.println("Could not load library state. Starting fresh.");
}
// on exit
try {
library.saveLibraryState(filePath);
System.out.println("Library state saved successfully.");
} catch (IOException e) {
} catch (LibraryException e) {
System.out.println(e.getMessage());
}
import java.io.IOException; can now come out of Main
entirely. It stays in Library, which genuinely does I/O.
You now have four new pieces in Library. This section walks each one,
then traces a single book all the way out to disk and back. If you can narrate this,
you can defend the whole change.
private static String encode(final String field) {
return URLEncoder.encode(field, StandardCharsets.UTF_8);
}
private static String decode(final String field) {
return URLDecoder.decode(field, StandardCharsets.UTF_8);
}
Percent-encoding replaces any character that is not a plain letter,
digit, or one of a few safe symbols with a % followed by its byte value in
hexadecimal. It is the same scheme URLs use, which is why the class lives in
java.net.
| Character | Becomes | Why it matters here |
|---|---|---|
| ; | %3B | This is the whole point. A semicolon in the data can no longer look like a separator. |
| space | + | Side effect. Harmless, but it is why the file looks odd. |
| % | %25 | The escape character escapes itself — so a title containing a literal % still round-trips. |
| é | %C3%A9 | Two bytes in UTF-8, so two percent-groups. Accents survive. |
static, and why private
static because they touch no instance state — they are pure
functions from one string to another. Marking them so says
“this cannot depend on which Library you call it on”.
private because the file format is an implementation detail. Nothing
outside Library should know or care that fields are encoded.
Why pass the charset explicitly? The one-argument
URLEncoder.encode(String) is deprecated and uses the platform default,
so the same code could produce different bytes on your Mac and on a build server.
StandardCharsets.UTF_8 removes the question.
saveLibraryState — three things happen per bookwriter.write(encode(book.getTitle()) + SEPARATOR
+ encode(book.getAuthor()) + SEPARATOR
+ book.isBorrowed());
writer.newLine();
Each field is encoded, joined with ;, and given its own line. The
boolean is not encoded — true and false contain no
special characters, so there is nothing to protect.
} catch (IOException e) {
throw new LibraryStorageException(
"Could not save library state to " + filePath, e);
}
It is translating an exception to the right abstraction. A caller
of Library should not have to know that state happens to live in a file
— that is the same criticism you made of borrowBook in Lesson 0004,
applied consistently.
The e as the second argument is the cause. Never drop
it: without it you lose the real reason (disk full, permission denied) and a stack
trace stops at your message. With it, the trace shows both layers.
Note the try (BufferedWriter writer = ...) is try-with-resources:
the writer is closed automatically, on the success path and the failure path alike.
That is what flushes the buffer to disk.
parseLine — validate, then buildString[] fields = line.split(SEPARATOR, -1);
if (fields.length != FIELD_COUNT) {
throw new LibraryStorageException("Corrupt library state at line "
+ lineNumber + ": expected " + FIELD_COUNT
+ " fields but found " + fields.length);
}
Book book = new Book(decode(fields[0]), decode(fields[1]));
if (Boolean.parseBoolean(fields[2])) {
book.borrowBook();
}
return book;
-1 is doing real work — do not drop it
split’s second argument is a limit. Without it, Java
silently discards trailing empty strings:
"Dune;;".split(";") → [Dune] length 1
"Dune;;".split(";", -1) → [Dune, "", ""] length 3
A book with an empty author saves as Dune;;false — fine. But a book
whose encoded fields end up empty would, without the -1, come
back as one field and be rejected as corrupt. The -1 means “keep
every field, including empty ones”, so the count check is honest.
The original did parts[2] with no check, so a short line threw
ArrayIndexOutOfBoundsException: Index 2 out of bounds for length 2 — an
exception about an array, from a method about a library, telling the user nothing.
Checking fields.length first means the only exception that can escape
is one you wrote, with a message naming the problem and the line number. Someone can
open the file, go to line 1, and see it.
Boolean.parseBoolean is worth knowing about: it returns
true only for a case-insensitive "true", and
false for every other string, including garbage. It never
throws. That leniency is precisely why the original corruption was silent.
loadLibraryState — the order of the last three linesList<Book> loaded = new ArrayList<>(); // 1. a NEW list, not the real one
try (BufferedReader reader = ...) {
String line;
int lineNumber = 0;
while ((line = reader.readLine()) != null) {
lineNumber++; // 2. 1-based, for the message
loaded.add(parseLine(line, lineNumber)); // 3. may throw - nothing lost yet
}
} catch (IOException e) {
throw new LibraryStorageException(...); // 4. still nothing lost
}
books.clear(); // 5. only reached if we got here
books.addAll(loaded);
Read lines 1, 5 and 6 together. Everything that can fail happens
before books is touched. Compare the original:
public void loadLibraryState(final String filePath) throws IOException {
books.clear(); // destroyed first...
try (BufferedReader reader = ...) { // ...then discovered the file is missing
The strong exception guarantee: if an operation throws, the object is left exactly as it was. Your version has it; the original did not.
Say it in those words. Most candidates move the clear() by instinct
and describe it as “I fixed the ordering”. Naming the guarantee is the
difference between moving a line and giving a method a contract.
Notice you also get it for free on a corrupt file, not just a missing one —
because parseLine throws from inside the loop, before the swap. A file
that is fine for 900 lines and broken on line 901 leaves your library untouched.
Title Coming Home; A Novel, author Rosamunde Pilcher, not
borrowed.
| Stage | Value |
|---|---|
| In memory | title="Coming Home; A Novel" author="Rosamunde Pilcher" |
encode(title) | Coming+Home%3B+A+Novel |
encode(author) | Rosamunde+Pilcher |
| Line written | Coming+Home%3B+A+Novel;Rosamunde+Pilcher;false |
split(";", -1) | ["Coming+Home%3B+A+Novel", "Rosamunde+Pilcher", "false"] |
| Length check | 3 == FIELD_COUNT → ok |
decode(fields[0]) | Coming Home; A Novel |
| Back in memory | Coming Home; A Novel by Rosamunde Pilcher (Available) |
The one semicolon that is data became %3B and never reached the
splitter. The two semicolons that are structure were written by
saveLibraryState itself and survive untouched. That separation — data
cannot impersonate structure — is the whole idea.
You are deciding to skip encode() on the author because a semicolon in
an author name is rarer. It is rarer — but the failure is not cosmetic, and the fix
costs nothing, so it is worth knowing exactly what you are accepting.
An un-encoded author with a semicolon produces a line with four fields, which your
own parseLine then rejects:
The+Great+Gatsby;Fitzgerald; F. Scott;false fields=4 -> LOAD FAILS
Good+Omens;Neil Gaiman; Terry Pratchett;false fields=4 -> LOAD FAILS
So it is not “one author displays oddly”. The application cannot load its own state file at all — every book is lost, and the user sees “Could not load library state. Starting fresh.” until someone edits the file by hand. “Surname; Firstname” and two co-authors are both realistic inputs.
The other cost is that an interviewer will ask “why is the title encoded
and the author not?”, and asymmetric code reads as an oversight rather than
a decision. Adding encode(...) around one more expression is not extra
work — it is the same work applied consistently.
If you still want to leave it, that is your call — but write the limitation into the README rather than leaving it silent, so it reads as a decision.
mvn -q clean compile
LMS() { java -cp target/classes com.solirius.intermediate.library.Main; }
rm -f libraryState.txt
printf '1\nComing Home; A Novel\nRosamunde Pilcher\n6\n' | LMS > /dev/null
cat libraryState.txt
Coming Home\; A Novel;Rosamunde Pilcher;false
There is the fix, visible on disk: the semicolon inside the title is now preceded by a backslash, and the two separators are not. Reload it:
printf '2\n6\n' | LMS | grep -A2 'Available books:'
rm -f libraryState.txt
Available books:
Coming Home; A Novel by Rosamunde Pilcher (Available)
Rosamunde Pilcher is back.
mvn clean test
Tests run: 5 ... in Saving and reloading the library
[ERROR] GoldenMasterTest.behavesAsApproved » Failed Approval
[ERROR] Tests run: 18, Failures: 0, Errors: 1, Skipped: 0
Everything green except the golden master, which is correct — you changed what the user sees. Check the diff is only that:
D=src/test/java/com/solirius/intermediate/library
diff -u $D/GoldenMasterTest.behavesAsApproved.approved.txt \
$D/GoldenMasterTest.behavesAsApproved.received.txt
The entire diff is one hunk:
-Coming Home by A Novel (Available)
+Coming Home; A Novel by Rosamunde Pilcher (Available)
Twelve other scenarios — every one of them — are byte-identical. You rewrote the entire persistence layer and the golden master proves you broke nothing on the way past. That is what the eight control scenarios were for.
mv -f $D/GoldenMasterTest.behavesAsApproved.received.txt \
$D/GoldenMasterTest.behavesAsApproved.approved.txt
mvn clean test
[INFO] Tests run: 18, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
git add src/main/java/com/solirius/intermediate/library/Library.java
git add src/main/java/com/solirius/intermediate/library/Main.java
git add src/main/java/com/solirius/intermediate/library/exceptions/LibraryStorageException.java
git add src/test/java/com/solirius/intermediate/library/LibraryPersistenceTest.java
git add $D/GoldenMasterTest.behavesAsApproved.approved.txt
git commit -F - <<'EOF'
fix: stop the saved format losing and corrupting data
Three defects in save/load, each covered by a test added here:
- Titles containing the ';' separator were split across fields, so
"Coming Home; A Novel" by Rosamunde Pilcher reloaded as "Coming Home"
by " A Novel". Worse, every field shifted one place left, so the
author was parsed as the borrowed flag and a book on loan came back
marked available. Fields are now escaped, so any character is legal.
- A truncated line raised ArrayIndexOutOfBoundsException from parts[2].
The field count is validated and reported with its line number.
- loadLibraryState cleared the collection before opening the file, so a
missing or corrupt file destroyed the in-memory library. The
replacement list is built first and swapped in only once the whole
file has parsed, giving the strong exception guarantee.
Storage failures now raise LibraryStorageException rather than
IOException, so the console handles them alongside domain failures.
EOF
git log --oneline
Make it work, then make it right. With the tests green you can address the
design problem: Library is both a collection of books and a file
format. Two reasons to change, one class.
You write the interface, then reach for the obvious move:
public class Library implements LibraryRepository {
@Override
public void save(Library library) throws LibraryStorageException {
}
@Override
public Library load() throws LibraryStorageException {
return null;
}
This is backwards, and it makes the problem worse. The whole
point was that Library should stop being a file format. Making
it implement the storage interface means it is now formally declared as one.
You have added a layer of indirection and changed nothing.
The tell is in the signature: save(Library library) on
Library means an object being handed another object of its own
type to save. And load() would have to return a different
Library than the one you called it on. Nonsense, both ways round.
Your FileLibraryRepository is probably already finished and correct.
The compiler errors you are seeing are the remaining work, and there are only three
pieces. The rest of this section explains why; this box is what.
getBooks() to LibraryFixes: cannot find symbol: method getBooks()
Add this method at the end of Library, just before the closing brace:
/**
* Gets the books this library holds, in the order they were added.
*
* @return an unmodifiable view of the books
*/
public List<Book> getBooks() {
return Collections.unmodifiableList(books);
}
and add one import:
import java.util.Collections;
LibraryFixes: Library.java: cannot find symbol (from
encode and parseLine, which you moved to the repository)
Delete all of this from Library — it now lives in
FileLibraryRepository:
saveLibraryState(...) and loadLibraryState(...)parseLine(...), encode(...), decode(...)SEPARATOR, FIELD_COUNT and ESCAPE constantsjava.io, java.net and java.nio importLibraryStorageException import — Library no longer throws itTwo commands confirm you got it all. Both must print nothing:
grep -nE 'saveLibraryState|loadLibraryState|parseLine|encode\(|decode\(|SEPARATOR|FIELD_COUNT|ESCAPE' \
src/main/java/com/solirius/intermediate/library/Library.java
grep -nE 'import java\.(io|net|nio)' \
src/main/java/com/solirius/intermediate/library/Library.java
Library.java should drop to roughly 108 lines of pure
domain.
Fixes: cannot find symbol: method saveLibraryState / loadLibraryState
cd ~/Desktop/java-guild-candidate-test-intermediate-main
git mv src/test/java/com/solirius/intermediate/library/LibraryPersistenceTest.java \
src/test/java/com/solirius/intermediate/library/FileLibraryRepositoryTest.java
Then replace the whole file with this. Your four tests are all here — three translated mechanically, and the fourth replaced for the reason explained further down:
package com.solirius.intermediate.library;
import com.solirius.intermediate.library.exceptions.LibraryStorageException;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
import static org.junit.jupiter.api.Assertions.*;
@DisplayName("The file-backed library repository")
class FileLibraryRepositoryTest {
@TempDir
private Path directory;
private LibraryRepository repositoryFor(final String fileName) {
return new FileLibraryRepository(directory.resolve(fileName).toString());
}
@Test
@DisplayName("preserves a title that contains the field separator")
void preservesATitleContainingTheSeparator() throws Exception {
LibraryRepository repository = repositoryFor("state.txt");
Library saved = new Library();
saved.addBook(new Book("Coming Home; A Novel", "Rosamunde Pilcher"));
repository.save(saved);
Book book = repository.load().viewAvailableBooks().get(0);
assertEquals("Coming Home; A Novel", book.getTitle());
assertEquals("Rosamunde Pilcher", book.getAuthor());
}
@Test
@DisplayName("preserves a title that contains the escape character")
void preservesATitleContainingTheEscapeCharacter() throws Exception {
LibraryRepository repository = repositoryFor("state.txt");
Library saved = new Library();
saved.addBook(new Book("AC\\DC: The Story", "Murray Engleheart"));
repository.save(saved);
assertEquals("AC\\DC: The Story",
repository.load().viewAvailableBooks().get(0).getTitle());
}
@Test
@DisplayName("rejects a corrupt file instead of crashing on an array index")
void rejectsACorruptFile() throws Exception {
Path file = directory.resolve("state.txt");
Files.write(file, List.of("Dune;Frank Herbert")); // only 2 fields
LibraryStorageException thrown = assertThrows(LibraryStorageException.class,
() -> repositoryFor("state.txt").load());
assertEquals("Corrupt library state at line 1: "
+ "expected 3 fields but found 2", thrown.getMessage());
}
@Test
@DisplayName("reports a missing file rather than returning an empty library")
void reportsAMissingFileRatherThanReturningAnEmptyLibrary() {
assertThrows(LibraryStorageException.class,
() -> repositoryFor("absent.txt").load(),
"a missing file must be reported, not silently treated as empty");
}
}
If you have already renamed the class but the body still calls
saveLibraryState, here are the exact edits. There are six, and nothing
else in the file changes — every assertion stays as it is.
- @DisplayName("Library data persistence")
+ @DisplayName("The file-backed library repository")
The subject under test is the repository now, not the library.
@TempDir field
@TempDir
private Path directory;
+
+ private LibraryRepository repositoryFor(final String fileName) {
+ return new FileLibraryRepository(directory.resolve(fileName).toString());
+ }
Every test needs a repository pointed at a file inside the temporary directory.
This does it in one line each, instead of four tests repeating
directory.resolve(...).toString().
preservesATitleContainingTheSeparator
- Path file = directory.resolve("state.txt");
+ LibraryRepository repository = repositoryFor("state.txt");
Library saved = new Library();
saved.addBook(new Book("Coming Home; A Novel", "Rosamunde Pilcher"));
- saved.saveLibraryState(file.toString());
+ repository.save(saved);
- Library reloaded = new Library();
- reloaded.loadLibraryState(file.toString());
-
- Book book = reloaded.viewAvailableBooks().get(0);
+ Book book = repository.load().viewAvailableBooks().get(0);
assertEquals("Coming Home; A Novel", book.getTitle()); // unchanged
assertEquals("Rosamunde Pilcher", book.getAuthor()); // unchanged
Three lines collapse into one. You no longer construct an empty
Library and load into it — load() returns the
library, so you use its result directly.
preservesATitleContainingTheEscapeCharacter
- Path file = directory.resolve("state.txt");
+ LibraryRepository repository = repositoryFor("state.txt");
Library saved = new Library();
saved.addBook(new Book("AC\DC: The Story", "Murray Engleheart"));
- saved.saveLibraryState(file.toString());
-
- Library reloaded = new Library();
- reloaded.loadLibraryState(file.toString());
+ repository.save(saved);
assertEquals("AC\DC: The Story",
- reloaded.viewAvailableBooks().get(0).getTitle());
+ repository.load().viewAvailableBooks().get(0).getTitle());
Same shape as change 3.
rejectsACorruptFile — one line
LibraryStorageException thrown = assertThrows(LibraryStorageException.class,
- () -> new Library().loadLibraryState(file.toString()));
+ () -> repositoryFor("state.txt").load());
Keep the two lines above it —
Path file = directory.resolve("state.txt"); and the
Files.write(...). This test writes the corrupt file by hand, so it still
needs the Path. It is the only test that does.
- @DisplayName("leaves the library untouched when the file cannot be read")
- void leavesTheLibraryUntouchedWhenTheFileCannotBeRead() {
- Library library = new Library();
- library.addBook(new Book("Dune", "Frank Herbert"));
-
- assertThrows(LibraryStorageException.class,
- () -> library.loadLibraryState(
- directory.resolve("absent.txt").toString()));
-
- assertEquals(1, library.viewAvailableBooks().size(),
- "a failed load must not destroy the books already held");
- }
+ @DisplayName("reports a missing file rather than returning an empty library")
+ void reportsAMissingFileRatherThanReturningAnEmptyLibrary() {
+ assertThrows(LibraryStorageException.class,
+ () -> repositoryFor("absent.txt").load(),
+ "a missing file must be reported, not silently treated as empty");
+ }
This is the only edit that is not mechanical. The old test cannot be translated
because load() takes no library — there is nothing you can hand it that
it could damage, so there is nothing to assert on afterwards. The failure mode it
guarded is gone by construction.
The replacement covers the risk that does exist in the new design: that
load() quietly returns an empty library rather than reporting that the
file is unreadable. See the explanation below for the full
reasoning.
All of your existing imports are still needed — Files,
Path and List are all still used by
rejectsACorruptFile. Nothing to add and nothing to remove.
mvn clean test
[INFO] Tests run: 17, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
17, not 18 — one test was replaced rather than added.
0 errors means the golden master passed without being
re-approved. You moved persistence into a new class, changed the constructor
wiring in Main, and the application still produces byte-identical output
across all thirteen scenarios. That is the definition of a refactor, and it is a
claim your commit message can make and a reviewer can check.
| Yours | This one | Why |
|---|---|---|
| class LibraryPersistenceTest | class FileLibraryRepositoryTest | The name must match the file, and the subject under test is now the repository. |
| — | repositoryFor(String) | One helper, so no test repeats directory.resolve(...).toString(). |
| saved.saveLibraryState(file) reloaded.loadLibraryState(file) | repository.save(saved) repository.load() | The mechanical translation. No assertion changed. |
| leavesTheLibraryUntouched WhenTheFileCannotBeRead | reportsAMissingFile RatherThanReturningAnEmptyLibrary | The old one is now unwritable — see below. |
Note the tests are typed as LibraryRepository, not
FileLibraryRepository. The same suite could be pointed at a future SQLite
implementation by changing one line in repositoryFor.
Your FileLibraryRepository uses import java.io.*;.
Checkstyle’s AvoidStarImport flags it, and the supplied code uses
explicit imports throughout. Expand it to the four you actually use:
import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
Check the whole picture with:
mvn checkstyle:check -Dcheckstyle.failOnViolation=false -Dcheckstyle.consoleOutput=true
The starter scored zero violations, so anything you see is
something you introduced. DesignForExtension on save and
load is easily silenced by marking the class final, which is
also the right design call — nothing should be subclassing it.
| Type | Its one job | What it must NOT know |
|---|---|---|
| Library | Hold books; borrow and return them | That files exist |
| LibraryRepository | Declare that state can be saved and loaded | How, or where |
| FileLibraryRepository | Turn a Library into text and back | Anything about borrowing rules |
Library does not implement the interface. A separate
class does, and Library becomes the thing it is handed.
LibraryDelete saveLibraryState, loadLibraryState,
parseLine, encode, decode, the
SEPARATOR and FIELD_COUNT constants, and every
java.io / java.net import. Library drops to
about a hundred lines of pure domain.
It needs one new method, so a repository can read what to save:
/**
* Gets the books this library holds, in the order they were added.
*
* @return an unmodifiable view of the books
*/
public List<Book> getBooks() {
return Collections.unmodifiableList(books);
}
unmodifiableList
Returning books directly would hand every caller a live handle on the
library’s internal list — they could add or remove books without going through
addBook, bypassing any rule you add later. The wrapper makes the read
possible and the write impossible. It is one word, and it is the difference between
exposing data and exposing state.
package com.solirius.intermediate.library;
import com.solirius.intermediate.library.exceptions.LibraryStorageException;
/**
* Stores and retrieves the state of a library.
* <p>
* The domain depends on this abstraction rather than on a file, so an
* alternative store - a portable database, for instance - can be added as a
* new implementation without changing {@link Library}.
* </p>
*/
public interface LibraryRepository {
/**
* Saves the given library, replacing anything previously stored.
*
* @param library the library to store
* @throws LibraryStorageException if the state cannot be written
*/
void save(Library library) throws LibraryStorageException;
/**
* Loads the stored library.
*
* @return the stored library
* @throws LibraryStorageException if the state cannot be read or is corrupt
*/
Library load() throws LibraryStorageException;
}
Note there is no file path in either signature. That is deliberate: a path is a detail of this implementation. A database version would need a connection string instead, and the interface should not have to change.
The path goes in the constructor, so each repository instance knows where its own state lives:
public class FileLibraryRepository implements LibraryRepository {
/** Field separator used by the flat-file format. */
private static final String SEPARATOR = ";";
/** Number of fields on every well-formed line. */
private static final int FIELD_COUNT = 3;
/** Where the state is kept. */
private final String filePath;
/**
* Creates a repository backed by the given file.
*
* @param theFilePath the file to read from and write to
*/
public FileLibraryRepository(final String theFilePath) {
this.filePath = theFilePath;
}
save is your old saveLibraryState, with two changes: no
path parameter (it is a field now), and the books come from the argument:
@Override
public void save(final Library library) throws LibraryStorageException {
try (BufferedWriter writer =
new BufferedWriter(new FileWriter(filePath))) {
for (Book book : library.getBooks()) {
writer.write(encode(book.getTitle()) + SEPARATOR
+ encode(book.getAuthor()) + SEPARATOR
+ book.isBorrowed());
writer.newLine();
}
} catch (IOException e) {
throw new LibraryStorageException(
"Could not save library state to " + filePath, e);
}
}
load is your old loadLibraryState, with one addition at the
end — it must build and return a library rather than mutate one:
@Override
public Library load() throws LibraryStorageException {
List<Book> loaded = new ArrayList<>();
try (BufferedReader reader = new BufferedReader(
new FileReader(filePath))) {
String line;
int lineNumber = 0;
while ((line = reader.readLine()) != null) {
lineNumber++;
loaded.add(parseLine(line, lineNumber));
}
} catch (IOException e) {
throw new LibraryStorageException(
"Could not read library state from " + filePath, e);
}
Library library = new Library();
for (Book book : loaded) {
library.addBook(book);
}
return library;
}
It got simpler, and this is a nice thing to notice out loud. The old
version needed the careful loaded-then-swap dance because it was
mutating an existing library. This version constructs a brand new
Library and only returns it at the end — so if anything throws, the
caller still holds whatever it had, and the half-built one is garbage.
The guarantee is now structural rather than a matter of statement order. That is what a good refactor does: it makes a property you had to maintain by hand into one you get for free.
Main// before
Library library = new Library();
String filePath = "libraryState.txt";
try {
library.loadLibraryState(filePath);
System.out.println("Library state loaded successfully.");
} catch (LibraryStorageException e) {
System.out.println("Could not load library state. Starting fresh.");
}
// after
String filePath = "libraryState.txt";
LibraryRepository repository = new FileLibraryRepository(filePath);
Library library;
try {
library = repository.load();
System.out.println("Library state loaded successfully.");
} catch (LibraryStorageException e) {
library = new Library(); // no stored state yet - start empty
System.out.println("Could not load library state. Starting fresh.");
}
and on exit:
library.saveLibraryState(filePath);
repository.save(library);
LibraryRepository repository = new FileLibraryRepository(filePath);
The variable is typed as the interface, not the class. Everything
after that line works against the abstraction, so swapping in
new SqliteLibraryRepository(...) is a one-line change in
Main and nothing else moves. That single line is the Dependency
Inversion Principle doing its job, and you can point at it.
Note also that library is now assigned inside the
try/catch, because load() returns it. The catch supplies
an empty Library as the fallback — which reads more honestly than the
old version, where a failed load silently left you with an object that happened to
be empty already.
Rename LibraryPersistenceTest to
FileLibraryRepositoryTest and swap how the subject is built. The
assertions stay byte for byte identical:
// before
Library saved = new Library();
saved.addBook(new Book("Coming Home; A Novel", "Rosamunde Pilcher"));
saved.saveLibraryState(file.toString());
Library reloaded = new Library();
reloaded.loadLibraryState(file.toString());
Book book = reloaded.viewAvailableBooks().get(0);
// after
LibraryRepository repository =
new FileLibraryRepository(directory.resolve("state.txt").toString());
Library saved = new Library();
saved.addBook(new Book("Coming Home; A Novel", "Rosamunde Pilcher"));
repository.save(saved);
Book book = repository.load().viewAvailableBooks().get(0);
// unchanged
assertEquals("Coming Home; A Novel", book.getTitle());
assertEquals("Rosamunde Pilcher", book.getAuthor());
Two checks, both of which a reviewer can run in seconds:
18 tests,
0 failures, 0 errors with the approved file untouched. The user sees
exactly what they saw before.Put that second point in the commit message. It is a falsifiable claim, and making claims a reviewer can check is a habit worth showing.
The refactor has four moving parts and they must all land before anything compiles. It is normal for the build to be red in the middle of it. Use the compiler as your to-do list:
| Part | Done when | Compiler error if not |
|---|---|---|
B. LibraryRepository interface exists | Two methods, no file path in either | cannot find symbol: class LibraryRepository |
C. FileLibraryRepository implements it | Holds the path; owns encode, decode, parseLine | is not abstract and does not override save/load |
A. Library stripped | Persistence methods deleted; getBooks() added | cannot find symbol: method getBooks() |
D. Main rewired | Builds a repository; assigns library from load() | cannot find symbol: method saveLibraryState |
| E. Tests migrated | Renamed, driving the repository | cannot find symbol: method loadLibraryState |
cannot find symbol: method getBooks()
You wrote FileLibraryRepository before stripping Library.
That is the right order — the repository is the thing that tells you what
Library needs to expose. Add the accessor:
/**
* Gets the books this library holds, in the order they were added.
*
* @return an unmodifiable view of the books
*/
public List<Book> getBooks() {
return Collections.unmodifiableList(books);
}
and add import java.util.Collections;.
Library
It is easy to write the repository and forget to delete the originals, leaving
saveLibraryState, loadLibraryState, parseLine,
encode, decode, SEPARATOR and
FIELD_COUNT sitting in both classes. Two implementations of one format
is strictly worse than the problem you set out to fix.
Check with:
grep -nE 'saveLibraryState|loadLibraryState|parseLine|encode\(|decode\(|SEPARATOR|FIELD_COUNT' \
src/main/java/com/solirius/intermediate/library/Library.java
That must print nothing. So must this:
grep -nE 'import java\.(io|net|nio)' \
src/main/java/com/solirius/intermediate/library/Library.java
A domain class with no I/O imports is the whole point, and it is a one-command proof you can show an interviewer.
Three are mechanical. The fourth is not, and it is the interesting one.
| Test | What happens to it |
|---|---|
| preservesATitleContainingTheSeparator | Mechanical — build a repository, call save/load. Assertions unchanged. |
| preservesATitleContainingTheEscapeCharacter | Mechanical. Assertions unchanged. |
| rejectsACorruptFile | Mechanical — repositoryFor("state.txt").load() replaces new Library().loadLibraryState(...). Message assertion unchanged. |
| leavesTheLibraryUntouchedWhenTheFileCannotBeRead | Cannot be written any more. See below. |
It asserted on a library it passed in:
library.loadLibraryState(path); // mutates 'library'
assertEquals(1, library.viewAvailableBooks().size()); // did it survive?
But the new signature is:
Library load() throws LibraryStorageException;
load() takes no library. There is nothing the caller
owns for it to damage — it builds a fresh Library and hands it back. So
the test has no subject to assert on, and the bug it guarded against
cannot occur.
That is the strongest thing a refactor can do: it did not fix the bug, it
made the bug unrepresentable. Before, the guarantee depended on
books.clear() being on the correct line; now it depends on nothing,
because the method has no access to the caller’s state.
Verified on the finished code: the repository contains zero references to any library’s internal collection.
Deleting a test without replacing it should always make you uneasy, so ask: what
is the new risk in this area? It is no longer “destroys existing data”.
It is that load() might silently return an empty library
when the file is unreadable, which would look like success and quietly wipe the
user’s catalogue on the next save.
@Test
@DisplayName("reports a missing file rather than returning an empty library")
void reportsAMissingFileRatherThanReturningAnEmptyLibrary() {
assertThrows(LibraryStorageException.class,
() -> repositoryFor("absent.txt").load(),
"a missing file must be reported, not silently treated as empty");
}
“One test disappeared in the refactor. It checked that a failed load
didn’t destroy the books already in memory — but once load() returns
a new Library instead of mutating one, it has no access to the
caller’s state, so that failure mode is gone by construction. I replaced it with a
test for the new risk in that area: that a missing file is reported rather than
quietly coming back as an empty library.”
Deleting a test because the design made it meaningless, and saying what you put in its place, is a much better story than a suite that only ever grows.
package com.solirius.intermediate.library;
import com.solirius.intermediate.library.exceptions.LibraryStorageException;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
import static org.junit.jupiter.api.Assertions.*;
@DisplayName("The file-backed library repository")
class FileLibraryRepositoryTest {
@TempDir
private Path directory;
private LibraryRepository repositoryFor(final String fileName) {
return new FileLibraryRepository(directory.resolve(fileName).toString());
}
@Test
@DisplayName("preserves a title that contains the field separator")
void preservesATitleContainingTheSeparator() throws Exception {
LibraryRepository repository = repositoryFor("state.txt");
Library saved = new Library();
saved.addBook(new Book("Coming Home; A Novel", "Rosamunde Pilcher"));
repository.save(saved);
Book book = repository.load().viewAvailableBooks().get(0);
assertEquals("Coming Home; A Novel", book.getTitle());
assertEquals("Rosamunde Pilcher", book.getAuthor());
}
@Test
@DisplayName("preserves a title that contains the escape character")
void preservesATitleContainingTheEscapeCharacter() throws Exception {
LibraryRepository repository = repositoryFor("state.txt");
Library saved = new Library();
saved.addBook(new Book("AC\\DC: The Story", "Murray Engleheart"));
repository.save(saved);
assertEquals("AC\\DC: The Story",
repository.load().viewAvailableBooks().get(0).getTitle());
}
@Test
@DisplayName("rejects a corrupt file instead of crashing on an array index")
void rejectsACorruptFile() throws Exception {
Path file = directory.resolve("state.txt");
Files.write(file, List.of("Dune;Frank Herbert")); // only 2 fields
LibraryStorageException thrown = assertThrows(LibraryStorageException.class,
() -> repositoryFor("state.txt").load());
assertEquals("Corrupt library state at line 1: "
+ "expected 3 fields but found 2", thrown.getMessage());
}
@Test
@DisplayName("reports a missing file rather than returning an empty library")
void reportsAMissingFileRatherThanReturningAnEmptyLibrary() {
assertThrows(LibraryStorageException.class,
() -> repositoryFor("absent.txt").load(),
"a missing file must be reported, not silently treated as empty");
}
}
Note the repositoryFor helper: one line, and it keeps every test from
repeating the directory.resolve(...).toString() plumbing. Note also that
the tests are typed against LibraryRepository, not
FileLibraryRepository — so the same suite could be pointed at a future
SQLite implementation with one line changed.
[INFO] Tests run: 17, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
Seventeen rather than eighteen, because one test was deliberately replaced rather than added. The golden master is green and unapproved — which is the proof that none of this changed what the user sees.
This is the commit where a reviewer decides whether you understand what a refactor is. Get it green first, then use this message.
cd ~/Desktop/java-guild-candidate-test-intermediate-main
mvn clean test # 17 green, golden master among them
git add src/main/java/com/solirius/intermediate/library/LibraryRepository.java
git add src/main/java/com/solirius/intermediate/library/FileLibraryRepository.java
git add src/main/java/com/solirius/intermediate/library/Library.java
git add src/main/java/com/solirius/intermediate/library/Main.java
git add src/test/java/com/solirius/intermediate/library/
git commit -F - <<'EOF'
refactor: move persistence out of Library into a repository
Library was two things at once: the rules about borrowing books, and the
format those books are stored in. That gave it two unrelated reasons to
change - a new lending rule, or a new file layout - and meant the domain
class carried java.io and java.net imports it had no business knowing
about.
Persistence now sits behind a LibraryRepository interface, implemented by
FileLibraryRepository, which owns the separator, the percent-encoding and
the line parsing. Library keeps only the book rules and exposes
getBooks(), an unmodifiable view the repository reads from. Main builds
the repository and is the only place that names the concrete type.
This matters beyond tidiness: the Advanced tier of the brief asks for a
portable file-based database, and the pom already declares sqlite-jdbc.
With this seam, that becomes a new class implementing the same interface
and a one-line change in Main. Without it, it is a rewrite of Library.
One test was removed rather than translated. It asserted that a failed
load left the in-memory books untouched, but load() now returns a new
Library instead of mutating one, so it has no access to the caller's
state and that failure mode cannot occur. It is replaced by a test for
the risk the new design does carry: that an unreadable file is reported
rather than quietly coming back as an empty library.
No behaviour change. The tests moved to FileLibraryRepositoryTest but no
assertion was altered, and the golden master passes against its existing
approved file.
EOF
git log --oneline
Five paragraphs, each doing a job the diff cannot do for you.
| Paragraph | Its job |
|---|---|
| 1. The problem | States the defect in terms of reasons to change, not “it did two things”. That is Martin’s actual formulation of SRP, and the phrasing signals you have read the source rather than the slogan. The java.io detail makes it concrete and checkable. |
| 2. What moved | Names the new types and, crucially, what each one now owns. A reviewer can read this and predict the diff. |
| 3. Why it was worth doing | Ties the change to a requirement in their brief. This is the difference between “I applied a pattern” and “I designed for a change you asked for”. Without this paragraph the commit is vulnerable to “YAGNI”. |
| 4. The deleted test | The paragraph most candidates would omit. Removing a test looks like weakening the suite unless you explain it. Saying the failure mode became unreachable by construction, and naming what replaced it, turns a suspicious deletion into evidence of judgement. |
| 5. The falsifiable claim | Two assertions a reviewer can verify in under a minute: no assertion changed, and the golden master passes unapproved. Making checkable claims is a habit worth demonstrating. |
“the golden master passes against its existing approved file”
This is the strongest thing you can say about a refactor, and it is checkable in one command. It means you restructured four files and the application’s output across thirteen scenarios did not move by a single byte. Very few candidates will be able to make that claim at all, because very few will have taken a golden master in the first place.
Verified before publishing: 17 tests, 0 failures, 0 errors, with the
approved file untouched by the commit.
git show --stat already
prints them, accurately and for free. Prose that duplicates the diff goes stale and
adds nothing.You now have the answer pre-written; it is the commit message. Compressed to twenty seconds:
“Librarywas both the borrowing rules and the file format, so it had two reasons to change. I put persistence behind aLibraryRepositoryinterface — partly for its own sake, but mainly because your Advanced tier asks for a database store andsqlite-jdbcis already in the pom, so that becomes an added class rather than a rewrite. I kept it as a separate commit with no assertion changed and the golden master still green against its existing approved file, so it is verifiable as a refactor rather than a rewrite.”
It touches five files and gains you nothing the brief actually requires. If you are short of time, leave it and write one paragraph in the README:
Persistence currently lives onLibrary, which gives that class two reasons to change. The next step would be to extract aLibraryRepositoryinterface with a file-backed implementation, so the Advanced tier’s database store becomes an added class rather than a change to the domain.
A named, reasoned non-goal costs you nothing. A half-finished refactor costs a lot.
Why is the semicolon bug worse than the array-index crash?
Far fewer, in fact — most titles have no semicolon. Rarity makes it worse, not better, because it survives casual testing.
It reproduces perfectly: save a title with a semicolon, load it. Reproducibility is not the issue.
Right. The crash announces itself with a stack trace and gets fixed the same day. The corruption produces a plausible-looking book with the wrong author, and is discovered — if ever — long after the correct data is unrecoverable. Severity is about detectability, not frequency.
The save is faithful; the file genuinely contains every character. The information is lost at parse time, on the way back in.
What does @TempDir buy you that a hardcoded path does not?
All three matter here. Each test gets its own directory, so one cannot see another’s leftovers; nothing lands in your working tree to be accidentally committed; and JUnit deletes it afterwards. The starter’s own libraryState.txt landing in the project root is exactly the mess this avoids.
JUnit forbids nothing. Any test can write anywhere the JVM has permission to write — which is the problem, not the restriction.
It helps, but JUnit 5 does not run tests in parallel unless you configure it to, and @TempDir makes no such guarantee on its own.
Real directory, real disk, real files. It is a genuine temporary directory, not a fake filesystem.
Which is the strongest argument for extracting LibraryRepository?
No such rule exists, and inventing rules is how codebases fill with interfaces that have exactly one implementation forever.
You can, but you should not want to here — a real file in a @TempDir tests the actual format, and mocking the thing under test proves nothing about whether the file parses.
A common misquote. Martin’s own formulation is about reasons to change, not counting responsibilities. Say “one thing” in interview and expect to be asked what a thing is.
This is the argument with teeth, because it names a concrete future change that the brief itself asks for and the pom already ships the driver for. “Design for change” is only meaningful when you can name the change.
JUnit 5 User Guide — the @TempDir extension.
Short. Covers field versus parameter injection, per-test versus per-class
lifetime, and the cleanup modes.
If you want to attempt the Advanced SQLite repository, tell me — it is a good use of an evening only once everything else is finished and committed.