ModernUO/dev-docs/code-standards.md
Kamron Batman 540559fbac
docs: upstream bug-reporting process for forks, and comments explain why (#2649)
## Summary

Two workflow additions to `CLAUDE.md`, with the detail in `dev-docs/`, plus a GitHub issue form.

### Rule 21 — comments explain why, never what changed
- Keep invariants, protocol/era quirks, value couplings, and the reason a workaround exists. One line where one line will do.
- Development narrative does not ship: before a PR leaves draft, sweep `git diff main...HEAD` for added comments and remove change history ("previously", "changed from"), review dialogue ("per review"), hedges ("I think"), and commented-out code. What a future reader still needs goes in the commit message or PR description.
- New `## Comments` section in `dev-docs/code-standards.md`; rule 21 in the `modernuo-code-audit` skill.

### Workflow Rules — bugs you were not asked to fix
Written for forks and custom projects built on ModernUO, which inherit this repo's `CLAUDE.md`. Also applies here (upstream is `origin`).

1. **Classify** — exploit-class (duplication, player-triggerable crash, auth bypass) goes to private disclosure only (`hi@modernuo.com`, per `CONTRIBUTING.md`), never a public issue, PR, or Discord post.
2. **Verify** the defective lines exist verbatim in upstream `main` via read-only `gh api`. If they don't, it is the fork's bug and nothing leaves the fork. This is also what mechanically keeps custom code out of reports: only lines that pass the check may be quoted.
3. **Dedup** — search upstream issues and PRs (all states) by file, symbol, and symptom, plus recent commits on the path. A merged fix → offer to import it; an open issue → offer to comment there.
4. **Draft, show, offer, wait** — the draft and a *scrub ledger* (what was removed, what was verified upstream, what is new code) are shown in full. The user picks: file an issue, open a PR, comment, draft a Discord post for https://muo.gg/discord, or nothing. A standing or conditional instruction ("if upstream has a fix pull it in and open a ticket") is not approval of a draft the user has not read.
5. **Importing fixes** — never `fetch`/`cherry-pick`/hand-port from any remote without asking; canonical URL only; review the whole commit as untrusted (workflows, `*.csproj`, `Directory.Build.props`, scripts); apply only after a second yes. Third-party forks and unmerged PRs are never a source.

`dev-docs/bug-reporting.md` is the process; `dev-docs/claude-skills/modernuo-bug-reporting.md` is Claude's step-by-step procedure (opt-in, like the other skills).

### Issue form
- `.github/ISSUE_TEMPLATE/bug_report.yml` — structured fields (summary, upstream location, commit, reproduction against a clean build, expansion/platform/found-via dropdowns) and a required checklist restating the rules. Applies the `bug` label.
- `.github/ISSUE_TEMPLATE/config.yml` — chooser links for private security reports and Discord.
- Form submissions render as `### <Field>` markdown; the skill writes that exact shape via `gh issue create --body-file`, so an assistant-drafted issue is indistinguishable from a browser one.

## How the skill was validated

Pressure scenarios against subagents, without and then with the rules present.

- **Without**: given a fork with custom content, an owner who said "open a ticket so they know" and went to bed, and a restart in 20 minutes, the agent filed the upstream issue immediately — and the body carried the fork's console log lines and a description of the custom mechanic that triggered the bug, despite the agent stating it had "scrubbed hard". It did refuse an unreviewed third-party PR.
- **With**: same scenario, the agent pushed nothing and filed nothing, removed every log line (including the one that only named the upstream method), produced a scrub ledger, left the "reproduced on clean main" box honestly unticked, and linked the third-party PR without fetching it. An exploit scenario with a relayed standing "email the maintainers immediately" instruction also held: private email drafted, not sent.

The rationalization table in the skill is built from what the baseline agent actually said.

## Notes for review

- The rule is deliberately strict: the user reads the exact draft before anything is submitted. If an explicit in-session waiver ("file it, I don't need to see it") should be honored, that is a one-line change to Workflow Rule 2.
- All `gh` commands in the docs were run against this repo; the worked example points at `Projects/UOContent/Mobiles/AI/BaseAI/PetOrders.cs` and a line that exists there, with the example defect marked as illustrative.
- `config.yml` links private disclosure to `CONTRIBUTING.md` rather than a `mailto:` because GitHub only accepts `http(s)` contact links.
2026-09-14 23:27:27 -07:00

22 KiB

ModernUO Coding Standards

This document defines the coding conventions and standards for ModernUO content development. All code under Projects/UOContent/ and Projects/Server/ must follow these guidelines.

Table of Contents

  1. Naming Conventions
  2. Comments
  3. Performance Rules
  4. Serialization Requirements
  5. Logging
  6. Threading Model
  7. Memory Management
  8. Entity Lifecycle
  9. Era-Conditional Code
  10. File Organization

Naming Conventions

Fields and Properties

  • Private fields: _camelCase prefix with underscore
    private int _charges;
    private Mobile _owner;
    private TimerExecutionToken _timerToken;
    
  • Properties: PascalCase
    public int Charges { get; set; }
    public Mobile Owner => _owner;
    
  • Methods: PascalCase
    public void OnDoubleClick(Mobile from) { }
    private void CheckExpiry() { }
    
  • Constants: PascalCase
    public const int MaxCharges = 20;
    
  • Local variables: camelCase
    var damage = Utility.RandomMinMax(10, 20);
    

Brace Style

ALL control flow statements (if, else, for, foreach, while, do, switch) must have braces, even for single-line bodies. This reduces merge conflicts and diff sizes.

// BAD
if (condition)
    DoSomething();

// GOOD
if (condition)
{
    DoSomething();
}

Switch Patterns

Prefer switch expressions and switch-when pattern matching where they improve readability and enable JIT/PGO optimization. Skip if code becomes unreadable or is on a cold path.

// Prefer switch expression for value mapping
private static string GetName(GemType type) => type switch
{
    GemType.StarSapphire => "star sapphire",
    GemType.Emerald      => "emerald",
    _                    => "gem"
};

// Prefer switch-when for performance (compiler hints to JIT/PGO)
switch (item)
{
    case Sword { Quality: >= ItemQuality.Exceptional } when Core.AOS:
    {
        bonus = 10;
        break;
    }
}

Legacy Code

Older code uses m_ prefix for private fields (e.g., m_Amount). Do not change existing m_ fields, but always use _ prefix for new code.

Access Levels

public enum AccessLevel
{
    Player,       // Regular players
    Counselor,    // Support staff
    GameMaster,   // GMs
    Seer,         // Event coordinators
    Administrator,// Server admins
    Developer,    // Developers
    Owner         // Server owner
}

Reference: Projects/Server/Mobiles/Mobile.cs

Comments

A comment in main is read by someone who has never seen the PR, the review thread, or the previous version of the line. Write for that reader.

What a comment is for

  • Why, not what: an invariant, a protocol or client quirk, an era rule, the reason a workaround exists, a coupling between two values that will bite whoever changes one of them.
  • One line where one line will do. If the code is self-describing, no comment.
  • /// XML docs on public API stay. //TODO Implement X (terse, with the dependent line commented out beneath it) stays.
// GOOD — protects an ordering invariant a future tidy-up would break
// Must precede CheckHerding: it returns early every tick while the pet is herded,
// so a deleted target would otherwise never be noticed on that path.
if (m_Mobile.ControlTarget?.Deleted == true)

// GOOD — a coupling that is invisible from either line alone
// Follow range must exceed the herding stop distance, otherwise a herded pet
// oscillates one tile in and out of range every tick.
if (m_Mobile.InRange(target, 3))

Development narrative does not ship

These describe the change or the conversation, not the code. They reference context that does not exist in main. Remove them before a PR leaves draft; what a future reader still needs goes in the commit message or the PR description.

Remove Looks like
Change narrative "changed from", "previously", "used to", "no longer", "moved from", "was:", "renamed"
Review dialogue "per review", "reviewer asked", "as discussed", "see PR discussion"
Diff explanation "added this to fix", "this line handles the bug reported on Discord"
Reasoning in progress "I think this is right", "not sure if", "might need", "for now"
Commented-out code the old line kept "in case" — Git is the revert mechanism
Restated code // increment i
// BAD — every line is narrative; the invariant it hides is the GOOD example above
// Changed from the old CheckHerding-first ordering: previously we checked herding
// before validating the target, which meant a deleted ControlTarget was never
// noticed on the herding path. Moved the deleted check up per review feedback.
// NOTE: I think this is right but the herding path was hard to test — see PR discussion.

Finalization sweep

Before marking a PR ready, sweep every comment the PR added or changed:

git diff main...HEAD | grep -nE '^\+.*(//|/\*)'

For each hit: keep (technical, still true without the PR), rewrite (a real invariant buried in narrative — keep the invariant, drop the story), or delete. Scope is the PR's own diff; do not rewrite comments in code the PR did not touch.


Performance Rules

LINQ: Know What's Optimized (.NET 10)

.NET 10's JIT and Dynamic PGO can now eliminate abstraction overhead for specific LINQ patterns. Not all LINQ is banned — but most still is. This section defines exactly what's allowed and what isn't.

Prerequisites: All optimizations below require .NET 10 with tiered compilation and Dynamic PGO enabled (both on by default — don't disable them). Tier 1 optimizations need ~30+ calls for the JIT to recompile at Tier 1 with PGO data.

Tier 1 — Zero-Cost Abstractions (use freely on hot paths)

These patterns produce zero heap allocations after JIT warmup, performing as well as hand-written code.

foreach over IEnumerable<T> backed by known collection types:

PGO profiles the concrete type. Guarded devirtualization (GDV) emits a specialized path. The enumerator is devirtualized, inlined, and stack-allocated.

Optimized backing types: T[], List<T>, Stack<T>, Queue<T>, ConcurrentDictionary<TKey,TValue>, PriorityQueue<TElement,TPriority>.

// ✅ ALLOWED — JIT eliminates all abstraction overhead
int Sum(IEnumerable<int> values)  // caller passes int[] or List<int>
{
    int sum = 0;
    foreach (int v in values) sum += v;
    return sum;
}

Falls back to normal virtual dispatch + heap-allocated enumerator for unknown/uncommon collection types or before JIT warmup.

.Contains() after a preceding LINQ operator:

LINQ has ~30 specialized Contains overrides that bypass intermediate processing entirely. No sort, no HashSet, no buffering — the source is searched directly.

Preceding operator What .Contains() does Speedup vs .NET 9
.Distinct() Searches source directly — no HashSet built ~363x
.Union(other) Searches both sources — no HashSet ~302x
.OrderBy() / .OrderByDescending() Searches source directly — no sort ~258x
.ThenBy() / .ThenByDescending() Same — no sort ~258x
.Append() / .Prepend() / .Concat() Searches sequentially ~56x
.SelectMany(f) Searches each sub-source ~49x
.Reverse() Searches source directly — no buffering ~9x
.Where(p).Select(f) Applies predicate+projection inline ~7x
.Select(f) Applies projection inline moderate
.Skip(n) / .Take(n) Searches within bounds moderate
.OfType<T>() / .Cast<T>() Filters/casts and searches moderate
.Intersect(other) / .Except(other) Searches appropriately large
.Shuffle() Searches source directly — no shuffle large
.Shuffle().Take(n) Hypergeometric probability — near O(1) math massive
// ✅ ALLOWED — no sort performed, source searched directly
bool exists = source.OrderBy(x => x.Name).Contains(target);

// ✅ ALLOWED — no HashSet built
bool exists = source.Distinct().Contains(target);

// ✅ ALLOWED — no buffering/reversing
bool exists = source.Reverse().Contains(target);

Falls back to normal enumeration for custom IEnumerable<T> implementations that LINQ doesn't recognize.

.Count() on sized collections:

Returns .Count property directly when source implements ICollection<T> or is a known LINQ iterator with tracked count (after Range, Repeat, Skip, Take, Append, etc.). O(1), no enumeration.

// ✅ ALLOWED — O(1) property access
int count = myList.Count();
int count = Enumerable.Range(0, 1000).Skip(10).Take(50).Count();

.OrderBy().First() / .OrderByDescending().First() / .OrderBy().Last():

LINQ performs O(N) min/max scan instead of O(N log N) sort. No sort buffer allocated.

// ✅ ALLOWED — O(N) scan, no sort
var cheapest = products.OrderBy(p => p.Price).First();
var newest = events.OrderByDescending(e => e.Timestamp).First();

.Shuffle().Take(n):

Uses reservoir sampling — single pass over source, O(n) memory. Does NOT shuffle the entire collection.

// ✅ ALLOWED — reservoir sampling, not full shuffle
var sample = population.Shuffle().Take(10).ToArray();

Enumerable.Range() / Enumerable.Sequence() terminal operations:

When followed by .Count(), .Contains(), .ToArray(), .ToList(), .Skip(), .Take(), .ElementAt(), .Last(). Specialized iterators compute results from arithmetic, not enumeration.

// ✅ ALLOWED — range check, no enumeration
bool has = Enumerable.Range(0, 1000).Contains(500);

// ✅ ALLOWED — single allocation, span fill
int[] arr = Enumerable.Range(0, 100).ToArray();

Tier 2 — Low Overhead (acceptable on warm paths, benchmark if critical)

These patterns have some overhead but are significantly optimized in .NET 10.

.Skip(n).Take(m).ToArray() / .ToList() on T[] or List<T>:

Uses vectorized Span<T>.CopyTo (~5x faster than .NET 9). Still allocates the output array/list.

// ✅ Acceptable on warm paths — vectorized copy
var page = items.Skip(offset).Take(pageSize).ToArray();

.LeftJoin() / .RightJoin() (new in .NET 10):

~2x faster and ~2x less memory than the manual GroupJoin+SelectMany+DefaultIfEmpty pattern.

// ✅ Prefer over manual GroupJoin chain
var results = orders.LeftJoin(customers, o => o.CustomerId, c => c.Id,
    (order, customer) => new { order, customer });

.Where(predicate) on T[] or List<T>:

The WhereIterator still heap-allocates, but enumerating it is cheaper due to PGO. For true hot paths, manual foreach+if is still faster.

// ⚠️ Acceptable but not zero-cost — WhereIterator allocates
foreach (var item in items.Where(x => x.IsActive))
    Process(item);

// 🏆 Faster manual alternative for true hot paths:
foreach (var item in items)
    if (item.IsActive) Process(item);

Tier 3 — Still Forbidden on Hot Paths

These patterns still carry meaningful abstraction overhead. Use manual code.

Pattern Why it's still slow Manual alternative
.Select(f).Where(p) (this order) Each intermediate iterator allocates foreach + if + inline transform
.GroupBy(k) Builds dictionary internally Manual dictionary loop
.ToDictionary() / .ToHashSet() Always allocates the collection Pre-size and fill manually
.ToLookup() Always builds grouping structure Manual dictionary of lists
.Aggregate(f) Delegate overhead per element Manual accumulator loop
.Sum() / .Min() / .Max() on float/double No SIMD vectorization in LINQ (ARM) TensorPrimitives.Sum() etc.
.SelectMany(f) (iterating results, not .Contains()) Multiple enumerator allocations Nested manual loops
.Zip() iterating Enumerator allocations Dual-index for loop
Any LINQ over IAsyncEnumerable<T> No PGO/escape analysis for async Manual await foreach
Long chains: .Where().Select().OrderBy().Take() Each step allocates an iterator Manual loop with sort
// ❌ STILL FORBIDDEN — allocates iterator + delegate per step
var targets = nearbyMobiles.Where(m => m.Alive).ToList();
var count = items.Count(i => i.Stackable);  // Count with predicate is NOT .Count()
var first = mobiles.FirstOrDefault(m => m is PlayerMobile);

// ✅ CORRECT — zero allocations
using var targets = PooledRefList<Mobile>.Create();
foreach (var m in nearbyMobiles)
{
    if (m.Alive)
        targets.Add(m);
}

// ✅ CORRECT — manual count
var count = 0;
foreach (var i in items)
{
    if (i.Stackable)
        count++;
}

Quick Decision Flowchart

Is it .Contains() after another LINQ operator?
  YES → ✅ Use it (see Tier 1 table)

Is it foreach over IEnumerable<T> backed by T[]/List<T>/Stack<T>/Queue<T>?
  YES → ✅ Use it (zero-alloc with PGO)

Is it .OrderBy().First() or .OrderBy().Last()?
  YES → ✅ Use it (O(N) not O(N log N))

Is it .Shuffle().Take(n)?
  YES → ✅ Use it (reservoir sampling)

Is it .Count() on a sized collection or Range?
  YES → ✅ Use it (O(1))

Is it .Skip().Take().ToArray() on T[]/List<T>?
  YES → ✅ Acceptable (vectorized copy)

Is it anything else on a hot path?
  → ❌ Write manual code

Reference: Stephen Toub, "Performance Improvements in .NET 10", September 2025. dotnet/runtime PRs: #112684, #108153, #111473, #116978, #112173, #118425.

Array Pooling

Use STArrayPool<T>.Shared instead of ArrayPool<T>.Shared in game logic. STArrayPool is optimized for single-threaded access (no locks).

var buffer = STArrayPool<byte>.Shared.Rent(1024);
try
{
    // Use buffer...
}
finally
{
    STArrayPool<byte>.Shared.Return(buffer);
}

Reference: Projects/Server/Buffers/STArrayPool.cs

PooledRefList

For temporary lists in methods, use PooledRefList<T> instead of new List<T>():

using var list = PooledRefList<Mobile>.Create();
// list is stack-allocated, uses pooled backing array
list.Add(mobile);
// Automatically returns array to pool on Dispose

Reference: Projects/Server/Collections/PooledRefList.cs

Spatial Queries

Never iterate World.Mobiles or World.Items directly. Use map-based spatial queries:

// BAD - O(n) over ALL mobiles in the world
foreach (var m in World.Mobiles.Values)
{
    if (m.InRange(location, 10))
        DoSomething(m);
}

// GOOD - O(1) sector lookup
foreach (var m in map.GetMobilesInRange<Mobile>(location, 10))
{
    DoSomething(m);
}

Available spatial queries (on Map):

  • GetMobilesAt<T>(Point3D p) - exact location
  • GetMobilesInRange<T>(Point3D p, int range) - within range
  • GetMobilesInBounds<T>(Rectangle2D bounds) - within rectangle
  • Same patterns for GetItemsAt, GetItemsInRange, GetItemsInBounds

Serialization Requirements

Partial Classes

Any class with [SerializationGenerator] must be declared partial:

[SerializationGenerator(0)]
public partial class MyItem : Item  // MUST be partial
{
}

Constructible Attribute

Items and Mobiles must have [Constructible] on their parameterless constructor:

[Constructible]
public MyItem() : base(0x1234)
{
}

Timer Fields Are Not Serialized

TimerExecutionToken fields must NOT have [SerializableField]:

// CORRECT
private TimerExecutionToken _timerToken;  // No serialization attribute

// WRONG
[SerializableField(2)]
private TimerExecutionToken _timerToken;  // Will cause errors

Timers are restored in [AfterDeserialization] methods.

See: dev-docs/serialization.md for complete serialization guide.


Logging

Use structured logging via ILogger, never Console.WriteLine.

Setup

using Server.Logging;

public class MySystem
{
    private static readonly ILogger logger = LogFactory.GetLogger(typeof(MySystem));
}

Usage

logger.Debug("Processing {Count} items for {Player}", items.Count, player.Name);
logger.Information("Player {Name} logged in from {IP}", name, ip);
logger.Warning("Unexpected state in {System}: {Details}", "Combat", details);
logger.Error(exception, "Failed to process {Action}", action);
logger.Fatal(exception, "Unrecoverable error in {System}", system);

Levels

  • Debug - Detailed diagnostic information
  • Information - General operational events
  • Warning - Unexpected but recoverable situations
  • Error - Failures that affect specific operations
  • Fatal - Unrecoverable errors

Reference: Projects/Logger/ILogger.cs


Threading Model

ModernUO uses a single-threaded game loop. All game logic runs on one thread.

Forbidden in Game Code

// ALL of these are WRONG in game code:
Task.Run(() => ProcessItems());
new Thread(BackgroundWork).Start();
ThreadPool.QueueUserWorkItem(Work);
lock (_syncObj) { }
volatile int _counter;
ConcurrentDictionary<int, Item> _items;

Why It Works

  • EventLoopContext (SynchronizationContext) routes all await continuations to the main thread
  • await is safe because it always resumes on the game thread
  • No data races possible in single-threaded code

Exceptions

Only server infrastructure code may use threading:

  • Projects/Server/Main.cs - Event loop setup
  • World save disk I/O (serialization on main thread, writes may be background)
  • Network I/O

See: dev-docs/threading-model.md for complete threading documentation.


Memory Management

Array Returns

Always return pooled arrays:

var arr = STArrayPool<int>.Shared.Rent(size);
try
{
    // Use arr
}
finally
{
    STArrayPool<int>.Shared.Return(arr);
}

Avoid Allocations in Hot Paths

  • Use PooledRefList<T> instead of new List<T>()
  • Use stackalloc for small fixed-size buffers
  • Use STArrayPool<T> for larger buffers
  • Use ValueStringBuilder with stackalloc for string building (never System.Text.StringBuilder) → dev-docs/string-handling.md

Entity Lifecycle

Two-Phase Deletion

Items and Mobiles use two deletion hooks:

  1. OnDelete() - Called first. Cancel timers, remove from tracking systems.

    public override void OnDelete()
    {
        _timerToken.Cancel();
        base.OnDelete();
    }
    
  2. OnAfterDelete() - Called after entity is removed from world. Clean up references.

    public override void OnAfterDelete()
    {
        _timer?.Stop();
        _timer = null;
        _owner = null;
        base.OnAfterDelete();
    }
    

Reference Cleanup

Any field holding an Item or Mobile reference should be nulled in deletion:

public override void OnAfterDelete()
{
    _target = null;
    _owner = null;
    base.OnAfterDelete();
}

Era-Conditional Code

Always Ask for Target Era

If the user hasn't specified which expansion to target, always ask. Different eras have dramatically different mechanics.

Pattern

if (Core.AOS)  // Age of Shadows or later
{
    damage = GetNewAosDamage(10, 1, 4, target);
}
else  // Pre-AOS
{
    damage = Utility.Random(4, 4);
}

Available Checks

Core.T2A   // >= The Second Age
Core.UOR   // >= Renaissance
Core.UOTD  // >= Third Dawn
Core.LBR   // >= Blackthorn's Revenge
Core.AOS   // >= Age of Shadows
Core.SE    // >= Samurai Empire
Core.ML    // >= Mondain's Legacy
Core.SA    // >= Stygian Abyss
Core.HS    // >= High Seas
Core.TOL   // >= Time of Legends
Core.EJ    // >= Endless Journey

See: dev-docs/era-expansion.md for complete expansion guide.


File Organization

Directory Structure

Projects/UOContent/
├── Items/
│   ├── Weapons/       # BaseWeapon, Swords/, Maces/, etc.
│   ├── Armor/         # BaseArmor, Plate/, Chain/, etc.
│   ├── Clothing/      # Shirts, hats, etc.
│   ├── Misc/          # General items
│   └── Special/       # Unique/quest items
├── Mobiles/
│   ├── Animals/       # Bears/, Birds/, etc.
│   ├── Monsters/      # AOS/, SE/, ML/ by era
│   ├── Special/       # Champions, bosses
│   └── Vendors/       # NPC vendors
├── Spells/
│   ├── Base/          # Spell base classes
│   ├── First/ - Eighth/  # Magery circles
│   ├── Necromancy/    # Necro spells
│   └── Spellweaving/  # Spellweaving
├── Skills/            # Skill implementations
├── Gumps/             # UI dialogs
│   └── Base/          # Gump base classes
└── Engines/           # Complex systems

File Naming

  • One class per file (generally)
  • File name matches class name
  • Group related items in subdirectories

Quick Reference: Common Anti-Patterns

Anti-Pattern Correct Pattern
list.Where(x => x.Alive) foreach + if (Tier 2 — acceptable on warm paths)
.GroupBy() / .ToDictionary() / .ToHashSet() Manual dictionary loop (Tier 3 — still forbidden)
.Select(f).Where(p) chain foreach + if + inline transform (Tier 3)
.OrderBy().First() Allowed — O(N) scan, no sort (Tier 1)
.Distinct().Contains() Allowed — no HashSet built (Tier 1)
Console.WriteLine(msg) logger.Information(msg)
new List<T>() in hot path PooledRefList<T>.Create()
ArrayPool<T>.Shared STArrayPool<T>.Shared
ConcurrentDictionary Dictionary
Task.Run(...) Don't. Use timers.
World.Mobiles.Values iteration map.GetMobilesInRange<T>()
Missing partial on serialized class Add partial keyword
Serializing TimerExecutionToken Leave unserialized, restore in [AfterDeserialization]