refactor(server): dedupe award handlers and mutate-then-report response tail#158
Merged
Merged
Conversation
Part of #130. The resolved_state -> pending_choices -> serialize_character -> Map.put(:pending_choices, ...) sequence was repeated at five handler tails (build_finish, both award clauses, both resolve_choice clauses). Four of the five also called parse_display_mode(msg) twice in the same pipeline — harmless today because the parse is pure, but a trap if display-mode resolution ever grows side effects or cost, and a source of drift if one site's copy is updated and the others are not (build_finish had already drifted into a differently-shaped copy of the same logic). character_with_choices_response/4 now owns the sequence, parses the display mode exactly once per request, and lives with the other response helpers. The award handler's :awarded_xp enrichment stays at its call site via Map.put on the returned map, since it is specific to that command rather than part of the shared shape.
Closes #130. The explicit-value and computed-value award handlers were ~90% identical: same load/lookup/apply_award!/compute_pending_choice_slots/ save/respond sequence, differing only in where the value came from (the request vs compute_next_level_xp!) and the :awarded_xp key on the computed-value response. One clause now handles both, branching on Map.fetch(msg, "value"). The branch returns {value, response_extras} so the value source and its response enrichment stay paired in one place — :awarded_xp only appears when the server computed the amount, which preserves the response shapes exactly. Map.fetch (not Map.get) keeps an edge case identical to the old pattern-match dispatch: a request carrying an explicit null value matched the explicit-value clause before, and still takes the explicit-value branch now, failing in apply_award! rather than being reinterpreted as a computed award.
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
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.
Closes #130.
What
Two commits, one per dedupe target from the issue:
Shared response tail — the
resolved_state→pending_choices→serialize_character→Map.put(:pending_choices, ...)sequence was repeated at five handler tails (build_finish, bothawardclauses, bothresolve_choiceclauses), four of them callingparse_display_mode(msg)twice.character_with_choices_response/4now owns the sequence and parses the display mode exactly once per request. (build_finishhad already drifted into a differently-shaped copy of the same logic — now it's the same call as everyone else.)One award handler — the two
characters.awardclauses merged into one that branches onMap.fetch(msg, "value"). The branch returns{value, response_extras}so the value source and its:awarded_xpenrichment stay paired: the key appears only when the server computed the amount, preserving response shapes exactly.Notes
Map.fetchrather thanMap.getpreserves an edge case of the old pattern-match dispatch: an explicit JSONnullvalue still takes the explicit-value path (failing inapply_award!) instead of being reinterpreted as a computed award.No behavior change; all 389 tests pass unchanged, credo and dialyzer clean.
Summary by Bito