Skip to content

refactor: simplify Result validation logic#115

Merged
Chris-Wolfgang merged 6 commits into
mainfrom
refactor/result-validation-cleanup
May 12, 2026
Merged

refactor: simplify Result validation logic#115
Chris-Wolfgang merged 6 commits into
mainfrom
refactor/result-validation-cleanup

Conversation

@Chris-Wolfgang
Copy link
Copy Markdown
Owner

Summary

  • Replace switch (succeeded) { case true when... } in Result constructor with two idiomatic guard-clause ifs.
  • Remove redundant IsNullOrWhiteSpace pre-check from Result.Failure — the constructor already enforces the same invariant with the same ParamName.
  • Replace ?? throw in AllSucceeded / AnyFailed with explicit null-check guards, matching the style of Flatten.

Behavior unchanged.

Test plan

  • dotnet build -c Release — 0 warnings, 0 errors
  • dotnet test -c Release --framework net8.0 — 97/97 passing
  • CI green across all TFMs

🤖 Generated with Claude Code

- Replace switch-when statement in Result constructor with two
  idiomatic guard-clause ifs. Easier to read and a closer match
  to the prevailing style elsewhere in the codebase.
- Remove redundant IsNullOrWhiteSpace pre-check from Result.Failure;
  the constructor already enforces the same invariant with the same
  ParamName.
- Replace `?? throw` idiom in AllSucceeded/AnyFailed with explicit
  null-check guard clauses for consistency with Flatten.

Behavior is unchanged. All 97 tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 11, 2026 16:30
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors Result validation and helper logic to use guard clauses and explicit null checks, aiming to simplify the implementation while preserving the library’s Try-pattern invariants.

Changes:

  • Replaced the switch-based validation in Result’s constructor with two guard-clause if checks.
  • Removed the explicit IsNullOrWhiteSpace pre-check from Result.Failure, relying on constructor validation instead.
  • Refactored AnyFailed / AllSucceeded to use explicit null guards rather than ?? throw.

Comment thread src/Wolfgang.TryPattern/Result.cs Outdated
Comment thread src/Wolfgang.TryPattern/Result.cs Outdated
Chris-Wolfgang and others added 3 commits May 11, 2026 17:24
- Update Failure XML <exception> docs to "null, empty, or whitespace"
  so they match the actual `IsNullOrWhiteSpace` validation.
- Align the ctor's failure-path ArgumentException message to the
  original "errorMessage cannot be empty" so the exception text is
  identical to what callers got before this refactor.
- Apply the same DRY-ification to Result<T>.Failure: drop the
  redundant IsNullOrWhiteSpace pre-check (the base ctor enforces it
  with the same ParamName and message). This also keeps the two
  Failure factories behaviorally identical.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Revert the behavioral changes from 584d6e2:

- Restore the ctor's failed-path exception message to the original
  "A failed result must have an error message." (was changed to
  "errorMessage cannot be empty" in the prior fixup).
- Restore the IsNullOrWhiteSpace pre-check in Result.Failure and
  Result<T>.Failure so they keep throwing "errorMessage cannot be
  empty" exactly as they did before this PR.

Net result for callers: every public exception message is now
identical to what shipped before this PR. The only changes that
remain in the PR are pure refactors with no observable effect:

  - switch-when in the ctor -> two guard-clause ifs
  - `?? throw` in AllSucceeded/AnyFailed -> explicit null check

Doc-only fix kept: the <exception> XML for both Failure overloads
now correctly says "null, empty, or whitespace" (matches the actual
IsNullOrWhiteSpace validation).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 11, 2026 21:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/Wolfgang.TryPattern/Result.cs:64

  • PR description says the redundant IsNullOrWhiteSpace pre-check was removed from Result.Failure, but the guard is still present here. Either update the PR description to match the current implementation, or remove the pre-check and ensure the resulting exception behavior/messages are consistent with Result.Failure and the Result ctor.
    public static Result Failure(string errorMessage) =>
        string.IsNullOrWhiteSpace(errorMessage)
            ? throw new ArgumentException("errorMessage cannot be empty", nameof(errorMessage))
            : new Result(succeeded: false, errorMessage);

src/Wolfgang.TryPattern/Result.cs:64

  • The guard uses IsNullOrWhiteSpace(errorMessage) (and the XML docs now mention whitespace), but the thrown message says "errorMessage cannot be empty", which is misleading for whitespace-only (and null) inputs. Consider updating the exception message to reflect the actual validation (null/empty/whitespace), or changing the validation to match the message.
    public static Result Failure(string errorMessage) =>
        string.IsNullOrWhiteSpace(errorMessage)
            ? throw new ArgumentException("errorMessage cannot be empty", nameof(errorMessage))
            : new Result(succeeded: false, errorMessage);

src/Wolfgang.TryPattern/Result.cs:229

  • Same as Result.Failure: the validation rejects whitespace-only (IsNullOrWhiteSpace), but the exception message says "errorMessage cannot be empty". Consider aligning the message with the actual validation (null/empty/whitespace) for clearer diagnostics.
    public static new Result<T?> Failure(string errorMessage) =>
        string.IsNullOrWhiteSpace(errorMessage)
            ? throw new ArgumentException("errorMessage cannot be empty", nameof(errorMessage))
            : new Result<T?>(succeeded: false, errorMessage, default!);
#else
    public static new Result<T> Failure(string errorMessage) =>
        string.IsNullOrWhiteSpace(errorMessage)
            ? throw new ArgumentException("errorMessage cannot be empty", nameof(errorMessage))
            : new Result<T>(succeeded: false, errorMessage, default!);

Copilot AI review requested due to automatic review settings May 12, 2026 00:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/Wolfgang.TryPattern/Result.cs:65

  • The XML docs were updated to say whitespace-only error messages are invalid, but the thrown ArgumentException message still says "errorMessage cannot be empty". Since this guard uses IsNullOrWhiteSpace, callers passing whitespace get a misleading message; consider updating the exception message to mention null/empty/whitespace (or "null or whitespace") to match the actual validation and docs.
    /// <exception cref="ArgumentException">errorMessage is null, empty, or whitespace</exception>
    public static Result Failure(string errorMessage) =>
        string.IsNullOrWhiteSpace(errorMessage)
            ? throw new ArgumentException("errorMessage cannot be empty", nameof(errorMessage))
            : new Result(succeeded: false, errorMessage);

src/Wolfgang.TryPattern/Result.cs:65

  • PR description says Result.Failure removed its IsNullOrWhiteSpace pre-check because the ctor enforces the invariant, but the method still performs that pre-check. Either update the PR description to match the implementation, or remove the redundant guard (being mindful of any intended exception message/paramName consistency).
    /// <exception cref="ArgumentException">errorMessage is null, empty, or whitespace</exception>
    public static Result Failure(string errorMessage) =>
        string.IsNullOrWhiteSpace(errorMessage)
            ? throw new ArgumentException("errorMessage cannot be empty", nameof(errorMessage))
            : new Result(succeeded: false, errorMessage);

src/Wolfgang.TryPattern/Result.cs:317

  • The XML docs were updated to say whitespace-only error messages are invalid, but Result.Failure still throws ArgumentException("errorMessage cannot be empty") when errorMessage is whitespace (due to IsNullOrWhiteSpace). Consider updating the exception message to mention null/empty/whitespace so callers get an accurate failure reason and the docs match runtime behavior.
    /// <exception cref="ArgumentException">errorMessage is null, empty, or whitespace</exception>
#if NET5_0_OR_GREATER
    public static new Result<T?> Failure(string errorMessage) =>
        string.IsNullOrWhiteSpace(errorMessage)
            ? throw new ArgumentException("errorMessage cannot be empty", nameof(errorMessage))
            : new Result<T?>(succeeded: false, errorMessage, default!);

@Chris-Wolfgang Chris-Wolfgang merged commit 3cbb83c into main May 12, 2026
12 checks passed
@Chris-Wolfgang Chris-Wolfgang deleted the refactor/result-validation-cleanup branch May 12, 2026 01:01
@Chris-Wolfgang Chris-Wolfgang mentioned this pull request May 18, 2026
4 tasks
Chris-Wolfgang added a commit that referenced this pull request May 19, 2026
Changes since v0.3.1:

- fix: null-element validation in Result.Flatten / AllSucceeded /
  AnyFailed (#113) — previously calling `.Failed` on a null element
  threw NullReferenceException; now throws ArgumentException with the
  index of the offending element. Adds [NotNull] post-condition
  attribute to all three methods plus all Try.Run/RunAsync overloads.
  Polyfills NotNullAttribute for net462 / netstandard2.0.
- perf: Result.Success() now returns a cached singleton instead of
  allocating per call (#114). Behavior is locked in by a test using
  Assert.Same; XML docs warn callers not to rely on reference identity.
- perf: replace LINQ in Flatten / AllSucceeded / AnyFailed with index-
  based for-loops (#116). Flatten is now single-pass with a lazy
  StringBuilder so the common single-failure case has zero extra
  allocation. AllSucceeded / AnyFailed short-circuit on first
  failure, so a trailing null past the decisive element no longer
  throws (documented in <exception> XML and locked in by tests).
- refactor: simplify Result ctor validation (#115) — switch-when
  replaced with two guard-clause ifs. No behavior change; exception
  messages and ParamName preserved.
- chore: test cleanup (#117) — remove redundant inheritance-
  verification tests, fix tabs vs spaces, drop unused pragma.
- chore(deps): Bump Meziantou.Analyzer 3.0.77 → 3.0.85 (#118).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants