-
Notifications
You must be signed in to change notification settings - Fork 1
Description
Summary
Code review of the jg/variant-aware-paths branch identified several KISS (Keep It Simple) and DRY (Don't Repeat Yourself) violations worth addressing.
DRY Violations
1. Massive boilerplate duplication across vertex-weighted graph problems
Impact: High — ~325 duplicated lines across 5 files.
These files share nearly identical struct shapes, constructors, and accessors:
src/models/graph/maximum_independent_set.rssrc/models/graph/minimum_vertex_cover.rssrc/models/graph/maximum_clique.rssrc/models/graph/maximal_is.rssrc/models/graph/minimum_dominating_set.rs
Each repeats ~65 lines of identical code:
new(),with_weights(),from_graph(),from_graph_unit_weights()constructorsgraph(),num_vertices(),num_edges(),edges(),has_edge(),weights_ref(),set_weights(),weights(),is_weighted()accessors- The same
evaluate()pattern (check constraint → sum weights → returnSolutionSize::Valid(total))
Suggestion: Extract a VertexWeightedProblem<G, W> wrapper struct or a macro to share the common boilerplate. Each problem would only need to define its constraint check and optimization direction.
2. PlanarGraph / BipartiteGraph manual VariantParam impls
File: src/graph_types.rs:14-46
These manually implement VariantParam + inventory::submit! (12 lines each) — exactly what impl_variant_param! does. The macro currently requires a cast: closure for the parent: variant, which these ZST marker types can't meaningfully provide.
Suggestion: Add a parent-only (no cast) arm to impl_variant_param!:
impl_variant_param!(PlanarGraph, "graph", parent: SimpleGraph);3. source_graph() / target_graph() on ReductionEdge
File: src/rules/graph.rs:202-220
Two identical methods differing only in which field (source_variant / target_variant) they search.
Suggestion: Extract a single helper:
fn variant_graph(variant: &[(&str, &str)]) -> &str { ... }4. graph_hierarchy() / weight_hierarchy()
File: src/rules/graph.rs:370-382
Same pattern with duplicate LazyLock<HashMap> statics, differing only in the category key.
Suggestion: Replace with fn hierarchy(&self, category: &str) -> &HashMap<...>.
5. is_graph_subtype() / is_weight_subtype() / is_k_subtype()
File: src/rules/graph.rs:355-367
Three trivial one-liner wrappers all calling is_subtype() with different category strings. Callers could use is_subtype("graph", ...) directly.
KISS Violations
1. find_shortest_path enumerates all paths
File: src/rules/graph.rs:542-553
Currently finds all simple paths (exponential) then picks the shortest. Dijkstra with MinimizeSteps cost already exists in the same file via find_cheapest_path.
2. Premature cost function zoo
File: src/rules/cost.rs
Defines 5 specialized cost structs plus CustomCost<F>. The closure-based CustomCost already handles any case. Unless the specific strategies are used in multiple call sites, most are speculative generality.
3. Double normalization of empty graph variant
variant_to_map() (src/rules/graph.rs:636-648) normalizes empty "graph" to "SimpleGraph". But source_graph() / target_graph() (lines 202-220) also do the same check. Same concern handled in two places.
4. to_json() is a 190-line monolith
File: src/rules/graph.rs:775-964
Single method doing 5 logically distinct things: collect nodes, collect edges, generate natural edges, sort, resolve indices. Could be split into helper methods.
Suggested Priority
- DRY Roadmap #1 (vertex-weighted problem boilerplate) — highest impact, most error-prone
- DRY Implement remaining reduction rules #2 (macro arm for parent-only) — small effort, improves consistency
- KISS Roadmap #1 (shortest path via Dijkstra) — correctness/perf improvement
- KISS feat: Feature parity with ProblemReductions.jl #4 (split
to_json()) — readability - Remaining items — nice-to-have cleanup