# Refactoring Direction > Refactoring direction rules for Gum. Trigger when proposing or performing refactors that change how code is shaped — extracting helpers, choosing between static and instance, deciding where new logic should live. Applies to all Gum source projects. - Skill: `vchelaru/refactoring-direction` (Agent Skill) - Install (CLI): `npx skillmds@latest add vchelaru/refactoring-direction` - Raw SKILL.md: https://api.skillmd.com/api/skills/vchelaru/refactoring-direction/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: vchelaru (https://skillmd.com/u/vchelaru) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/vchelaru/refactoring-direction --- # Refactoring direction When refactoring Gum code, **always move toward instances, interfaces, and dedicated single-responsibility classes**. Never move the other way. ## Specific rules 1. **Never promote an instance method, field, or class to `static`.** Even if the method "has no instance state right now," keep it as an instance member. Static methods are sticky — they grow callers everywhere, can't be substituted in tests, and can't be evolved without source-breaking every caller. Instance methods preserve future optionality. 2. **Never demote an interface to a concrete type.** If a parameter or field is currently typed as an interface (`ISelectedState`, `IDialogService`, `IUndoManager`, etc.), keep it that way. Adding a new dependency? Inject it as an interface, not a concrete class. 3. **Prefer extracting a new single-responsibility class over adding to a "god class."** `GraphicalUiElement`, `CodeGenerator`, and similar large classes should not grow. New behavior goes in a new controller/service class that the existing class composes with, even if the new class starts with one method. **Worked example — composition when there's no shared base class available.** `CircleRuntime` and `RectangleRuntime` (`MonoGameGum/GueDeriving/`) each spend their one inheritance slot on a platform base (`GraphicalUiElement`/`SkiaShapeRuntime`), so when their XNALIKE-only Gradient/Dropshadow logic turned out to be near-identical, composition was the only option (PR #3427). The extraction used `struct`s, not classes: `Clone()` relies on `MemberwiseClone` to copy backing fields by value for free, and a class field would instead let the clone and the original share one mutable instance. The structs also don't cache the renderable slot references they push into — callers pass the current slots on every call — so nothing can go stale when `Clone()` rebuilds fresh slots. Reach for this shape — composed struct, no cached mutable refs — whenever two sibling classes duplicate logic but can't share a base class and the state is safe to copy by value. This is distinct from [gum-cross-platform-unification](../gum-cross-platform-unification/SKILL.md), which shares one runtime's code *across platforms*, not between two different runtime classes on the same platform. 4. **Prefer constructor injection over `*.Self` singletons.** Don't reverse the tool's migration off static singletons. - **`ObjectFinder.Self` is the sanctioned exception** — fine in new or refactored code; it won't be removed. It is intentionally still a static singleton across the entire codebase (tool, runtimes, tests); an `IObjectFinder` interface exists and is DI-registered (`Builder.cs` binds it to the `ObjectFinder.Self` instance), but the ~476 `.Self` call sites are intentionally left as-is — replacing them wholesale is a project-wide refactor, **not** a drive-by cleanup. Calls like `ObjectFinder.Self.GetBaseElements(...)` / `ObjectFinder.Self.GetDefaultChildName(...)` are acceptable in any context. No *tool* singletons should be reintroduced. - **`RenderingLibrary.*` / `InputLibrary.*` runtime singletons are also sanctioned exceptions — out of scope in any phase.** `Renderer.Self`, `SpriteManager.Self`, `ShapeManager.Self`, `LoaderManager.Self`, `Cursor.Self`, `Keyboard.Self`, etc. are shared across *all* runtimes (not tool-only), so draining them is a runtime-wide refactor outside the tool's decoupling goal — leave them as statics, like `ObjectFinder.Self`. The drain line is **ownership**: tool-owned singletons (`SelectedState`, `PluginManager`, …) are in scope; runtime-owned ones are not. - **Ownership isn't decided by which folder a class's `.cs` file sits in** — check whether `Gum.csproj` excludes it (``) in favor of a link into `GumCommon.csproj`, the same treatment as `ObjectFinder.cs`; that makes it a shared, headless (net8.0, no WPF) singleton already reachable from every runtime backend and `Gum.Presentation`, an `ObjectFinder`-class exception rather than a drain target. `StandardElementsManager` (`Gum/Managers/`) is exactly this case. - **Plugins are MEF parts and drain the same as core classes — in scope for Phase 2.** Inject either via an `[ImportingConstructor]` param or a public settable `[Import]` property (satisfied by an `[Export]`; live example: `MainPropertiesWindowPlugin`'s `[Import("LocalizationService")]`) — both are real DI. The axis that matters is **service-location (`Locator.GetRequiredService()`) vs injection**, not ctor-vs-property. MEF can only inject types bridged into its container in `PluginManager.LoadPlugins` via `batch.AddExportedValue`. **Draining a plugin relocates the `Locator` call to that composition root — it does not delete it:** bridge `T` once in `LoadPlugins` (`AddExportedValue(Locator.GetRequiredService())`), then the plugin injects `T`. Use explicit typed per-service bridges, not a generic `ExportProvider` (over-engineering at this scale). A plugin taking `PluginManager`/`IPluginManager` itself as a ctor param is **not** a construction cycle, despite looking like the plugin host injecting itself into its own plugin: `PluginManager` is DI-constructed with an empty ctor (`Builder.cs`) and is fully built *before* `LoadPlugins` ever composes a plugin, so `batch.AddExportedValue(instance)` in `LoadPlugins` bridges the same already-built singleton a `Locator` call would have fetched — it drains like any other service. The one constraint: the consuming ctor must not call a method on the injected `PluginManager` synchronously during construction — MEF populates `PluginManager`'s own loaded-plugin list via `ComposeParts(instance)` *after* all plugins (including the consumer) are constructed, so a same-ctor call touching that list would see it incomplete. Storing the reference for later use (`StartUp`, event handlers) is always safe. - Replacing a `Locator.GetRequiredService()` call with an injected `T` (one already exported to MEF) is good drive-by cleanup. - **Drain a blocking singleton on the spot — don't ask about timing.** There is a dedicated later phase for draining `.Self` singletons wholesale, but when a singleton is *in the way of the current task* — most often because the class can't be unit-tested until it takes its dependencies via the constructor — convert it to constructor injection as part of the current PR. Do **not** stop to ask whether this conflicts with the dedicated drain phase; it doesn't, and that question is noise. Make the judgment call yourself; only pause to ask if the drain is genuinely *dangerous* (a construction cycle you can't break cleanly) or its call-site blast radius is large enough to deserve its own PR. Expanding an interface (adding a method) or switching a concrete dependency to its interface in service of the drain is fine. - **Breaking the DI cycle the `Self`+`Initialize`+`Locator` pattern was hiding.** A class kept as a `Self` singleton that resolves its own dependencies inside an `Initialize()` via `Locator` is often doing that to dodge a *construction cycle* (a dependency's constructor needs the class back). When you move it to real constructor injection, that cycle resurfaces as a DI exception at startup. Break it by injecting the back-edge dependency as `Lazy` — the tool's DI already registers `Lazy<>` (see `Builder.cs`), so `Lazy` resolves and you access `.Value` at call time, past graph construction. Inject only the cycling dependencies lazily; keep the acyclic ones direct. **When you can choose which edge to lazy, prefer lazying the *consumer's* edge to the drained class** (`Lazy` in the higher-level class) rather than the drained class's back-edge: that defers the *entire* drained subtree off the consumer's construction path, which stays correct even when the cycle travels through a third hop (`A → B → drained → A`) — whereas lazying only the direct back-edge does not break such multi-hop cycles. Example: `CommandLineManager` (drained, #3277) ↔ `ProjectManager`; the `Lazy` went into `ProjectManager` (the consumer), keeping `CommandLineManager`'s own deps direct. ## When testability is the driver If the reason you're tempted to make something `static` is "so a test can call it without constructing the owner," the right answer is the opposite: keep it instance, and either (a) construct the owner in the test with stub dependencies, or (b) extract the logic into a new small class that's cheap to construct. The test's friction is a signal that the owner has too many responsibilities, not that the method should be static. **An extraction-for-testability is not done until the extracted unit has a test.** When you pull logic into a new class/service/ViewModel/utility specifically to make it testable, add the test before considering the refactor complete — test-first if the move also changes behavior, or a *characterization (pinning) test* capturing current behavior if the move preserves it. Do not let the [tdd](../tdd/SKILL.md) skill's refactor/rename exemption talk you out of it: extracting a new unit is not a pure rename, and an extracted-but-untested seam wastes the entire point of the extraction. **Drains are usually behavior-preserving — calibrate the test to that.** Most singleton/`Locator` drains change only *how* a dependency is obtained (static `Locator` → injected field), not what the code does, so test-first TDD has nothing to specify — the fitting tool is a *characterization (pinning) test*. But the compiler and DI container already verify most of a drain: injection wiring is type-checked, construction cycles throw at startup, and a `static`→instance conversion makes the compiler flag every stale call site. So a pinning test's marginal value is modest — add one where the injected seam is clean and the assertion is meaningful, and skip it where the harness would have to contort (e.g. hand-rolling a delegate to mock an `out` parameter) in favor of the compiler plus a manual check. The drain's real payoff is *enabling* later behavior tests, not the test written during the drain itself. This does **not** loosen the extraction rule above: pulling out a *new unit* still requires its own test — a `static`→instance conversion is not an extraction. ## Before refactoring across runtimes If a refactor touches shared runtime/rendering code (`GumCommon`, `RenderingLibrary`, anything under `Runtimes/`, or `MonoGameGum`), read [gum-runtime-topology](../gum-runtime-topology/SKILL.md) first. The same source is compiled into many assemblies and into FlatRedBall via shared projects, so "builds clean in `AllLibraries.sln`" does not mean "didn't break a consumer" (the WPF runtime and FRB are not in that solution). ## Converging per-platform duplicate files A recurring Gum refactor is driving two (or more) per-platform copies of the same file toward byte-for-byte identical content, so they can eventually collapse to one `#if`-gated linked source. This is done incrementally — block by block, mirroring `#if RAYLIB` / `#if !RAYLIB` in *both* copies wherever a difference is genuinely platform-specific, so the cross-file diff shrinks toward empty. If you're touching code that exists as duplicated per-platform copies (e.g. `CustomSetPropertyOnRenderable.cs`, `GueDeriving/*Runtime.cs`), read [gum-cross-platform-unification](../gum-cross-platform-unification/SKILL.md) for the full technique before editing. ## Why this matters in Gum Gum's tool code is mid-migration from static singletons (`PluginManager.Self`, `*Manager.Self`, etc.) to constructor-injected services. Every new `static` is a step backward and undoes that migration's payoff. The runtime libraries are similar: `GraphicalUiElement` is already bloated, and the project memory explicitly calls out "avoid adding new properties/methods directly to this class — prefer separate controller/manager classes." The direction in this skill is the same direction the rest of the codebase is moving.