Refactor - #268
Open
tameware wants to merge 6 commits into
Open
Conversation
Deduplicate the trump-first / skip-trump iteration logic used throughout QuickTricks behind next_quick_trick_suit, with unit tests pinning the order. Co-authored-by: Cursor <cursoragent@cursor.com>
Add characterization tests for the hand-0..2 make/undo path, then factor the identical rank/aggr/length updates into remove_card and restore_card. Co-authored-by: Cursor <cursoragent@cursor.com>
Collapse the thrp and ctx duplicates into a single make_3/undo_0 pair that uses the search facade, reuse remove_card/restore_card, and delete the unused simple helpers. Characterization tests cover trick completion and undo. Co-authored-by: Cursor <cursoragent@cursor.com>
Collapse the identical depth>=20 and depth<20 TT lookup blocks into apply_ab_tt_lookup, with unit tests covering miss, hit polarity, and best-move TT updates on both depth paths. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors duplicated logic in core solver components by extracting shared helpers (QuickTricks suit iteration and AB-search TT lookup / make+undo card handling) and adds characterization/unit tests to lock in existing behavior before further consolidation.
Changes:
- Introduces
next_quick_trick_suit()and replaces repeated suit-advance blocks inQuickTricks()with the helper. - Factors the duplicated ab_search TT lookup logic into
apply_ab_tt_lookup()and consolidates repeated card remove/restore code paths. - Adds new Bazel test targets and unit/characterization tests for the extracted helpers, plus updates test visibility to allow new test subpackages to link against
testable_dds.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| library/tests/quick_tricks/next_suit_test.cpp | Adds unit tests that pin QuickTricks suit-iteration order and skipping behavior. |
| library/tests/quick_tricks/BUILD.bazel | Introduces a dedicated Bazel cc_test target for the QuickTricks suit-iteration tests. |
| library/tests/ab_search/tt_lookup_test.cpp | Adds tests that pin TT hit/miss behavior and best-move propagation for the shared lookup helper. |
| library/tests/ab_search/make3_test.cpp | Adds characterization tests for make_3/undo_0 trick completion and winners snapshot/restore behavior. |
| library/tests/ab_search/make_undo_test.cpp | Adds characterization tests for make/undo behavior for hands 0–2 and confirms isolation from other hands. |
| library/tests/ab_search/BUILD.bazel | Introduces Bazel cc_test targets for the new ab_search unit/characterization tests. |
| library/src/quick_tricks.hpp | Declares the new next_quick_trick_suit() helper used by QuickTricks and tested by new unit tests. |
| library/src/quick_tricks.cpp | Implements next_quick_trick_suit() and replaces duplicated suit-advance logic with the helper. |
| library/src/BUILD.bazel | Extends testable_dds visibility to new test subpackages (ab_search, quick_tricks). |
| library/src/ab_search.hpp | Exposes undo helpers and the new apply_ab_tt_lookup() function for unit testing and shared use. |
| library/src/ab_search.cpp | Implements apply_ab_tt_lookup() and consolidates repeated card remove/restore logic across make/undo paths. |
Co-authored-by: Cursor <cursoragent@cursor.com>
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.
Factor out duplication. Each change is a separate commit.
Reviewers, let me know if you'd prefer to see this split into four PRs.