Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tidy ModifyEntityResult usage #310

Open
donaldgray opened this issue Mar 7, 2025 · 0 comments
Open

Tidy ModifyEntityResult usage #310

donaldgray opened this issue Mar 7, 2025 · 0 comments
Labels
tech debt/improvement internal code or workflow improvements

Comments

@donaldgray
Copy link
Member

donaldgray commented Mar 7, 2025

This is a small, non functional change that is purely around improving developer experience when working with the code.

public class ModifyEntityResult<T, TEnum> : IModifyRequest where T : JsonLdBase is used in ~80 places in the codebase.

This class is based on class of same name from Protagonist, where

  • T is the type of entity being returned
  • TEnum is an enum containing error code and is used to construct problem+json response.

I'm suggesting that this class be simplified, suggestion (not many fields missed for brevity):

public class ModifyEntityResult<TEnum> : IModifyRequest
{
    // Rather than T where T : JsonLdBase, just make entity a JsonLdBase. 
    // everything we edit is a JsonLdBase so just make this one, simplifies signature
    public JsonLdBase? Entity { get; private init; }

    // Code analysis attrs to avoid nullable warnings
    [MemberNotNullWhen(false, nameof(ErrorType))]
    [MemberNotNullWhen(true, nameof(Entity))]
    public bool IsSuccess { get; private init; }
    
    // Keep enum type
    public TEnum? ErrorType { get; private init; }
}

// Have subclasses for specific TEnum error types
public class PresentationResult : ModifyEntityResult<ModifyCollectionType>{}

In addition to tidying signature, this class is passed around more than it needs to be. Rather than passing ModifyEntityResult<TEnum>, we could pass the error enum only. This can then be converted to a ModifyEntityResult<TEnum> in mediatr handler or controller (ie close to returning as it contains details for generating http response).

/* Example code as-is */
// Service interface, returning MER
public interface IService
{
  ModifyEntityResult<PresentationManifest, ModifyCollectionType>? DoSomething(string id);
}

// Mediatr handler returning MER
public class UpsertManifestHandler(IManifestWrite manifestService)
    : IRequestHandler<UpsertManifest, ModifyEntityResult<PresentationManifest, ModifyCollectionType>>
{
    public Task<ModifyEntityResult<PresentationManifest, ModifyCollectionType>> Handle(UpsertManifest request,
        CancellationToken cancellationToken)

// Static method returning MER
public static async Task<ModifyEntityResult<T, ModifyCollectionType>?> TrySave<T>();

/* Becomes.. */
// Service interface, returning error enum, rather than MER
public interface IService
{
  ModifyCollectionType? DoSomething(string id);
}

// Mediatr handler still returns MER but simplified
public class UpsertManifestHandler(IManifestWrite manifestService)
    : IRequestHandler<UpsertManifest, PresentationResult>
{
    public Task<PresentationResult > Handle(UpsertManifest request, CancellationToken cancellationToken)

// Static method returning simplified PresentationResult model
public static async Task<PresentationResult?> TrySave<T>();

Also, consider renaming ModifyCollectionType or adding a new Enum for Manifests.

This isn't a huge change but is fairly wide reaching hence specific ticket to arrange timing.

@donaldgray donaldgray added the tech debt/improvement internal code or workflow improvements label Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech debt/improvement internal code or workflow improvements
Projects
None yet
Development

No branches or pull requests

1 participant