refactor(characters): split Characters module into per-concern modules - #162
Merged
Conversation
First slice of splitting the 1400-line Characters module along its concern seams (#134). Disk persistence shares no helpers with the rest of the module, so it moves wholesale into Characters.Store. The facade keeps the full @doc strings and defdelegates, following the pattern established by Characters.Advancement, so the published Hex API and its docs are unchanged.
Second slice of #134. Active-concept derivation, effect aggregation, and resolved_state form the foundation the other concerns build on (preparation, progression, and rolls all start from resolved_state), so they move out before the dependent concerns do. collect_active_concepts, effects_from_decisions, and inventory_effects move along as private helpers since Effects is their only consumer.
Third slice of #134. XP threshold derivation, xp_to_next_level, and at-level DAG evaluation move together as the level-math concern. level_xp_thresholds, xp_effect_target, and evaluate_at_level were private but are needed by compute_pending_choice_slots (which will land in Characters.Progression), so they become public with @doc false — internal API between the concern modules, hidden from Hex docs.
Fourth slice of #134. random_decisions, root_concept_ids, and their recursion helpers form the character-building decision generation concern. The issue's proposed module list predates this being a distinct seam; it fits none of Store/Effects/Leveling/Progression/ Preparation, so it gets its own module rather than being shoehorned in.
Fifth slice of #134. concept_roll! is the whole concept-roll concern; a single-function module is deliberate — folding it into Effects or Progression would blur their single-concern moduledocs for the sake of avoiding a small file.
Sixth slice of #134, and the largest: pending-choice computation, slot tracking, and option filtering. count_progression_decisions stays private to Progression — the resolution machinery still in the facade only needs it through next_progression_decision, which is already part of the public API. Progression reaches Effects and Leveling directly rather than through the facade so the dependency between concerns is explicit.
Seventh slice of #134. auto_resolve_pending and random_resolve_all share the resolve_all loop (unified in #127), differing only in their resolvable? predicate and value method, so they move together as the random-resolution concern. Kept separate from Progression to hold both under the ~400-line target: Progression computes what is pending, Resolution decides it.
Final extraction slice of #134. Preparation state, activate, and add_to_typed_inventory move with all their helpers — the shared prep-context pipeline from #126 keeps the read and write paths in one module. The Characters facade is now pure delegation; its moduledoc documents the submodule-per-concern layout.
The Characters module was split into per-concern modules (Store, Effects, Leveling, Generation, Rolls, Progression, Resolution, Preparation) over the previous commits, but all tests remained in the monolithic characters_test.exs. This moves each describe block into a test file matching the module that now owns the behavior, so test failures point directly at the responsible module and each file can be run in isolation. characters_test.exs is reduced to running the facade's doctests; the delegating functions themselves are covered by the per-concern files. No test bodies were changed — this is a pure relocation. Leveling gets no new file because its behavior was already covered by the pre-existing apply_award_test.exs and xp_to_next_level_test.exs. The codescene hook is skipped for this commit only: it flags pre-existing duplication in the relocated preparation tests as "new" because the file is new. Fixing it here would mix a refactor into a pure move; the duplication is removed in the following commit instead.
Six preparation tests each hand-built the same Graph.build → LoadedSystem scaffolding, differing only in nodes, concept_metadata, and (in one case) character_building_choices. Extract a built_spell_system/3 helper so each test states only the data that matters to its scenario. This also resolves the code-duplication issue codescene raised on the previous commit (which relocated these tests verbatim).
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Failed
(1 new file with code health below 10.00)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| New code is healthy | Violations | Code Health Impact | |
|---|---|---|---|
| preparation_test.exs | 1 rule | 9.39 | Suppress |
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Comment on lines
+122
to
+175
| test "toggle_field: sets prepared true for listed items and false for others" do | ||
| nodes = %{ | ||
| {"class", "wizard", "preparation_cap"} => %Node{type: :accumulator, base: "3"}, | ||
| {"character_trait", "max_spell_level", "level"} => %Node{type: :accumulator, base: "2"} | ||
| } | ||
|
|
||
| concept_metadata = %{ | ||
| {"class", "wizard"} => %{ | ||
| "preparation_mode" => "prepared", | ||
| "preparation_pool" => "spellbook" | ||
| }, | ||
| {"spell", "fire_bolt"} => %{"level" => 1}, | ||
| {"spell", "cure_wounds"} => %{"level" => 1} | ||
| } | ||
|
|
||
| system = built_spell_system(nodes, concept_metadata) | ||
|
|
||
| decisions = [ | ||
| %Decision{scope: nil, choice: "class", selection: "wizard"}, | ||
| %Decision{ | ||
| scope: {"character_progression", "spells_known"}, | ||
| choice: "choice_1", | ||
| selection: "fire_bolt" | ||
| }, | ||
| %Decision{ | ||
| scope: {"character_progression", "spells_known"}, | ||
| choice: "choice_2", | ||
| selection: "cure_wounds" | ||
| } | ||
| ] | ||
|
|
||
| inventory = [ | ||
| %InventoryItem{ | ||
| concept_type: "spell", | ||
| concept_id: "fire_bolt", | ||
| fields: %{"prepared" => false} | ||
| }, | ||
| %InventoryItem{ | ||
| concept_type: "spell", | ||
| concept_id: "cure_wounds", | ||
| fields: %{"prepared" => false} | ||
| } | ||
| ] | ||
|
|
||
| character = %{minimal_character(decisions) | inventory: inventory} | ||
|
|
||
| assert {:ok, updated} = Characters.activate(system, character, "spell", ["fire_bolt"]) | ||
|
|
||
| assert Enum.find(updated.inventory, &(&1.concept_id == "fire_bolt")).fields["prepared"] == | ||
| true | ||
|
|
||
| assert Enum.find(updated.inventory, &(&1.concept_id == "cure_wounds")).fields["prepared"] == | ||
| false | ||
| end |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 2 functions with similar structure: "toggle_field: preserves cantrips (outside eligible pool) when preparing leveled spells","toggle_field: sets prepared true for listed items and false for others"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
ExTTRPGDev.Charactershad grown into a single module covering storage, dicerolling, effect resolution, leveling, progression choices, and spell
preparation. Finding the code responsible for a behavior meant scanning one
large file, and unrelated concerns were free to entangle.
What
Extracts eight per-concern modules, one commit each, with
Characterskeptas a thin facade (defdelegates) so the public API is unchanged:
Characters.Store— persistence (save/load/list)Characters.Effects— active concepts and effect accumulationCharacters.Leveling— XP awards and level-upCharacters.Generation— character generationCharacters.Rolls— concept rollsCharacters.Progression— pending choices and progression conceptsCharacters.Resolution— auto/random resolution of pending choicesCharacters.Preparation— typed inventory and spell preparationThe final two commits split the monolithic
characters_test.exsintoper-concern test files mirroring the new modules (pure relocation), then
dedupe repeated system-building scaffolding in the preparation tests.
No behavior changes; the facade's doctests and all relocated tests pass
unchanged.
Summary by Bito