Code-Review Standards

Cross-cutting standards codified during Phases 19 and 20. These supplement Coding Rules (formatting, naming, header conventions) with behavioural standards that reviewers should enforce.

C++ style

Constructor-sink ownership

Pass owning collections by value and std::move them into fields.

// Good
Part(juce::Uuid id, std::vector<Measure> measures)
    : id_(id), measures_(std::move(measures)) {}

// Bad — caller's vector is left in an unspecified state
Part(juce::Uuid id, std::vector<Measure>&& measures)
    : id_(id), measures_(measures) {}

Private-by-default class members

Class members are private unless a deliberate public-field decision is documented (see IntermediatePart for the parser-internal IR exception). Getters return const& for non-trivial types, by value for int / bool / small enums.

Reference > pointer > value

Function parameters prefer const T& for non-owning input, T* when nullable or representing weak ownership, and T by value only for small / cheap-to- copy types. Never pass T& for mutable output — return-by-value with a struct (see “Out-parameter purge” below).

Out-parameter purge

Functions that need to return multiple values return a struct, not multiple non-const reference parameters. Phase 19 examples:

// Good — DecodedSlideFlags is a translation-unit-local return struct
DecodedSlideFlags decodeSlideFlags(int flags);

// Bad — caller has to allocate three uninitialised vars
void applySlideFlags(int flags, TechniqueSet&, bool&, bool&);

The accumulator-style “walk a container and add to a set” pattern (e.g. collectTechniquesFromContainer(node, TechniqueSet&)) is the only acceptable exception — it mirrors std::ranges output-iterator conventions.

File-local transfer structs

Parser-internal types like GpNoteProps, GpBeatProps, ParsedScorePart stay as anonymous-namespace structs with public fields, not classes with private fields + getters. Document the “build then read as const” invariant in a header comment so the pattern is clear.

Public-header documentation

Every public type, function, and non-obvious invariant gets Doxygen comments with @brief, @param, @return. Reference standard: src/public/midi/MidiWriter.h.

Skip Doxygen for:

File-local helpers (anonymous-namespace functions and structs).
Trivial getters where the field name is self-documenting.

Tests

No tests reaching into private state

Tests assert via the public API. Measure::notes / Measure::spans are public fields by design — walking them is the public API for spans. Engraving::techniques_ is private — use getTechniques() instead.

No construct-then-getter-equals-constructor-arg tests

// Bad — tests the language, not the code
const Part p(uuid, "Lead", "Guitar", 1, 25, 0, measures, {}, {}, tuning);
CHECK(p.getTuningMidiPitches() == tuning);

// Good — tests behaviour that's observable through real code paths
const auto body = readFixture("staff-details-tuning.xml");
const auto result = MusicXmlParser::parse(std::string_view(body));
REQUIRE(result.ok());
CHECK(result.score->getParts()[0].getTuningMidiPitches().size() == 6);

Shared test support

tests/support/Builders.h and tests/support/XmlFixtures.h host the shared builders and fixture loader. Add to them rather than duplicating per file — seven copies of makeNote is the symptom that drove the refactor.

Inline XML → fixture files

Multi-line XML literals belong in tests/fixtures/*.xml, loaded via readFixture. Single-element snippets passed to a builder function are OK to keep inline.

Property tests for stochastic code

The humanizer is seeded — but tests assert properties (mean within sigma, floor / ceiling honoured, same seed → identical output) rather than pinning specific byte sequences. New stochastic features should follow the same pattern.

Commit hygiene

Subject line ≤ 50 chars, body wrapped at 80 (see .claude/rules/commit- messages.md).
Each phase’s incremental commits squash to one commit at the phase boundary via git reset --soft HEAD~N && git commit.
Never git push without explicit user instruction.
Never --no-verify to bypass a failing hook — fix the underlying issue.

Cross-platform

New file IO goes through Util::getPersistenceRoot() — never hardcode ~/Library/Application Support or %AppData%. The Windows path is portable-mode (exe parent dir); the macOS path is OS-conventional.

Licence

Project licence is CC BY-NC-ND 4.0 today; a custom licence will replace it in the future. Never embed specific licence names in code comments, docs, or commit messages — refer to “the project’s licence” so the prose stays accurate across future licence changes.