Refactoring command pattern

Question

I have implemented a command pattern for an SDK that I am building in C#, but it has a few special requirements that maybe not all command implementations have:

1. I want to prevent it from being misused - the request should not expose the ExecuteCommand() methods. This is to prevent the clients who use this SDK from bypassing the validations that are imposed by the ExecuteSdk() method.

2. I don't want to use reflection. Avoiding reflection will make it easier for developers in my company to follow the code, and also for optimum performance of the SDK.

3. There is a return type from the execution of a command. This is not common with commands, but I do need to return some basic data sometimes, for example an ID when creating new records. And I prefer not to pass data back out through the request object.

4. There are hundreds of commands. The sample below just has 2 commands for brevity. But with hundreds of commands I don't want to be maintaining collections or using if/switch statements eg. for type casting.

5. I want to keep the code as simple as possible and avoid conventions that will get in the developers' way. For example avoid casting the input and output types inside each command.

6. I have heard that dynamic dispatching will have a similar performance hit as reflection, so I also want to avoid this.

Given all those requirements!... please can someone show me how I can remove dynamic from the following code while meeting the above requirements? I'm actually wondering if its possible. Maybe with some clever use of polymorphism?

    using Microsoft.VisualStudio.TestTools.UnitTesting;
    
    namespace Hack6;
    
    [TestClass]
    public class Tests
    {
        private Sdk sdk;
        public Tests()
        {
            this.sdk = new Sdk();
        }
    
        [TestMethod]
        public void CanCreateOrder()
        {
            var createOrderRequest = new CreateOrderRequest();
            var createOrderResponse = this.sdk.ExecuteSdk(createOrderRequest);
            Assert.IsInstanceOfType(createOrderResponse, typeof(CreateOrderResponse));
            Assert.AreEqual(createOrderResponse.OrderId, "1234");
            Assert.AreEqual(createOrderResponse.Status, "success");
        }
        [TestMethod]
        public void CanDeleteOrder()
        {
            var sdk = new Sdk();
            var deleteOrderRequest = new DeleteOrderRequest { OrderNum = "1234" };
            var deleteOrderResponse = this.sdk.ExecuteSdk(deleteOrderRequest);
            Assert.IsInstanceOfType(deleteOrderResponse, typeof(DeleteOrderResponse));
            Assert.AreEqual(deleteOrderResponse.Status, "deleted");
        }
    }
    
    public class Sdk
    {
        public TOut ExecuteSdk<TOut>(IRequest<TOut> request)
            where TOut : IResponse
        {
            // validations and other logic goes here
            var commands = new Commands();
            var response = commands.ExecuteCommand((dynamic)request);
            return response;
        }
    }

    public interface IResponse { }
    
    public class DeleteOrderResponse : IResponse
    {
        public string Status { get; set; }
    }
    
    public class CreateOrderResponse : IResponse
    {
        public string OrderId { get; set; }
        public string Status { get; set; }
    }
    
    public interface IRequest<TOut> where TOut : IResponse
    {
    }
    
    public class DeleteOrderRequest : IRequest<DeleteOrderResponse>
    {
        public string OrderNum { get; set; }
    }
    
    public class CreateOrderRequest : IRequest<CreateOrderResponse>
    {
    }

    // note: internal - so clients outside the SDK cannot access this
    internal class Commands
    {
        public DeleteOrderResponse ExecuteCommand(DeleteOrderRequest request)
        {
            // logic goes here
            return new DeleteOrderResponse { Status = "deleted" };
        }
    
        public CreateOrderResponse ExecuteCommand(CreateOrderRequest request)
        {
            // logic goes here
            return new CreateOrderResponse
            {
                OrderId = "1234",
                Status = "success"
            };
        }
    }

The above is my preferred implementation so far, but the following is an alternative that does not use dynamic but does expose the command Execute() method - since it is inside the request object. This violates requirement (1), which is a big no-no, but maybe there is some way of making the execute method internal while keeping the remaining request object fields public? If so that would be a nice and simple solution.

using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;

namespace Hack7;

[TestClass]
public class Tests
{
    private Sdk sdk;
    public Tests()
    {
        this.sdk = new Sdk();
    }

    [TestMethod]
    public void CanCreateOrder()
    {
        var createOrderRequest = new CreateOrderRequest();
        var createOrderResponse = this.sdk.ExecuteSdk(createOrderRequest);
        Assert.IsInstanceOfType(createOrderResponse, typeof(CreateOrderResponse));
        Assert.AreEqual(createOrderResponse.OrderId, "1234");
        Assert.AreEqual(createOrderResponse.Status, "success");

        // fail because we can call createOrderRequest.Execute() - bypassing the validation logic in Sdk.ExecuteSdk()
        Assert.ThrowsException<Exception>(() => createOrderRequest.Execute(this.sdk));
    }
    [TestMethod]
    public void CanDeleteOrder()
    {
        var sdk = new Sdk();
        var deleteOrderRequest = new DeleteOrderRequest { OrderNum = "1234" };
        var deleteOrderResponse = this.sdk.ExecuteSdk(deleteOrderRequest);
        Assert.IsInstanceOfType(deleteOrderResponse, typeof(DeleteOrderResponse));
        Assert.AreEqual(deleteOrderResponse.Status, "deleted");

        // fail because we can call deleteOrderRequest.Execute() - bypassing the validation logic in Sdk.ExecuteSdk()
        Assert.ThrowsException<Exception>(() => deleteOrderRequest.Execute(this.sdk));
    }
}

public class Sdk
{
    public TOut ExecuteSdk<TOut>(ICommandRequest<TOut> commandRequest)
        where TOut : ICommandResponse
    {
        // validation logic goes here
        return commandRequest.Execute(this);
    }
}

public interface IResponse { }

public interface ICommandResponse : IResponse { }

public interface IRequest { }

public interface ICommandRequest<TOut> : IRequest
    where TOut : ICommandResponse
{
    TOut Execute(Sdk sdk);
}

public class DeleteOrderResponse : ICommandResponse
{
    public string Status { get; set; }
}

public sealed class DeleteOrderRequest : ICommandRequest<DeleteOrderResponse>
{
    public string OrderNum { get; set; }

    public DeleteOrderResponse Execute(Sdk sdk)
    {
        // logic goes here
        return new DeleteOrderResponse { Status = "deleted" };
    }
}

public class CreateOrderResponse : ICommandResponse
{
    public string OrderId { get; set; }
    public string Status { get; set; }
}

public sealed class CreateOrderRequest : ICommandRequest<CreateOrderResponse>
{
    public CreateOrderResponse Execute(Sdk sdk)
    {
        // logic goes here
        return new CreateOrderResponse
        {
            Status = "success",
            OrderId = "1234"
        };
    }
}

And finally another alternative that does not use dynamic dispatching. Its probably not a viable alternative because I couldn't figure out how to have multiple instances of the commands using this pattern. Anyway, I'll leave it in in case it triggers any ideas. And notice that this one also does explicit casts inside the commands, so that violates requirement (5).

 public class TestItOut2
 {
     public async Task Test()
     {
         var sdk = new Sdk();
         var request = new DeleteOrderRequest2 { OrderNum = 1 };
         DeleteOrderResponse2 response = await sdk.Execute2(request);
     }
 }
 public class Sdk
 {
     public async Task<TOut> Execute2<TOut>(ICommandRequest2<TOut> commandRequest)
         where TOut : class, ICommandResponse2
     {
         // validation logic and other logic goes here
         return await new CommandExecutor2<TOut>().Execute2(commandRequest, this);
     }
 }

 public interface IRequest2 { }
 public interface IResponse2 { }
 public interface ICommandResponse2 : IResponse2 { }

 public interface ICommandExecutor2<TOut> : IRequest2
    where TOut : ICommandResponse2
 {
     Task<TOut> Execute2(ICommandRequest2<TOut> request, Sdk sdk);
 }

 public interface ICommandRequest2<TOut> : IRequest2
     where TOut : ICommandResponse2
 {
 }

 public class DeleteOrderResponse2 : ICommandResponse2
 {
     public int OrderNum { get; set; }
 }
 public class DeleteOrderRequest2 : ICommandRequest2<DeleteOrderResponse2>
 {
     public int OrderNum { get; set; }
 }

 // there are many command executors. this one is for deleting orders.
 // note: internal - so clients outside the SDK cannot access this
 internal class CommandExecutor2<TOut> : ICommandRequest2<DeleteOrderResponse2>, ICommandExecutor2<TOut>
     where TOut : class, ICommandResponse2
 {
     public async Task<TOut> Execute2(ICommandRequest2<TOut> inputRequest, Sdk sdk)
     {
         // I don't like having to do a cast here - see requirement 5
         DeleteOrderRequest2 request = inputRequest as DeleteOrderRequest2;

         // logic goes here

         // I also don't like having to do a cast here - see requirement 5
         return (await Task.FromResult(new DeleteOrderResponse2())) as TOut; // example response
     }
 }

I posted a similar question on stackoverflow but didn't get any bites and they recommended I try posting here.

Answer 1

non-requirement

        public TOut ExecuteSdk ...
        {
            ...
            var response = commands.ExecuteCommand((dynamic)request);

Not a viable solution:

        var createOrderResponse = this.sdk.ExecuteSdk(createOrderRequest);

6. I have heard that dynamic dispatching will have a similar performance hit as reflection, so I also want to avoid this.

This is not a requirement; it is at best hearsay, from some uncited source.

What you have done is to invent a new task to complete prior to releasing this SDK:

• Review the hundreds of existing commands to identify the command which completes in minimum elapsed time.
• Benchmark repeated executions of that command twice, with and without dynamic dispatch.
• Knowing the magnitude of the elapsed time difference, ask stakeholders to make a business decision: Is it worthwhile to forgo the benefits of dynamic dispatch in this use case?

Answer 2

In the end the solution turned out to be quite simple - use the pattern where the command is a method inside the request class, but also make the request classes members of 2 interfaces - 1 public interface that does not contain an ExecuteCommand() method, and one internal interface that does contain an ExecuteCommand() method. This way the SDK can accept the public interface in its ExecuteSdk() method, and immediately convert that object to the internal interface to use the ExecuteCommand() method internally.

Assembly 1 - client:

    [TestClass]
    public class Tests
    {
        [TestMethod]
        public async Task CanCreateOrder()
        {
            var sdk = new Sdk();
            var createOrderRequest = new CreateOrderRequest();
    
            // note: createOrderRequest.Execute() does not exist in this assembly - good
            // this means the client cannot bypass sdk.ExecuteSdk()
            // createOrderRequest.Execute();
    
            // but sdk.ExecuteSdk does exist in this assembly - exactly as I want
            var createOrderResponse = await sdk.ExecuteSdk(createOrderRequest);
    
            Assert.IsInstanceOfType(createOrderResponse, typeof(CreateOrderResponse));
            Assert.AreEqual(createOrderResponse.OrderId, 1234);
            Assert.AreEqual(createOrderResponse.Status, "success");
        }
    }

Assembly 2 - SDK:

public class Sdk
{
    public async Task<TOut> ExecuteSdk<TOut>(ICommandRequest<TOut> commandRequest)
        where TOut : ICommandResponse
    {
        // validation logic and other logic goes here
        return await ((IInternalCommandRequest<TOut>)commandRequest).ExecuteCommand(this);
    }
}

public interface IResponse { }
public interface ICommandResponse : IResponse { }

public interface ICommandRequest<TOut>
    where TOut : ICommandResponse
{
    // note: do not expose the ExecuteCommand() method here, otherwise it will be public
    // and can be called directly by users of this SDK, bypassing the validations in the
    // Sdk.ExecuteSdk() methods
}

internal interface IInternalCommandRequest<TOut>
    where TOut : ICommandResponse
{
    // note: do define the ExecuteCommand() method here, only to be used internally by
    // the SDK
    Task<TOut> ExecuteCommand(Sdk sdk);
}

public class CreateOrderResponse : ICommandResponse
{
    public int OrderId { get; set; }
    public string Status { get; set; }
}

// there are many request objects. this one is for creating orders.
public class CreateOrderRequest : IInternalCommandRequest<CreateOrderResponse>, ICommandRequest<CreateOrderResponse>
{
    async Task<CreateOrderResponse> IInternalCommandRequest<CreateOrderResponse>.ExecuteCommand(Sdk sdk)
    {
        // logic goes here
        var createOrderResponse = new CreateOrderResponse
        {
            OrderId = 1234,
            Status = "success"
        };
        return await Task.FromResult(createOrderResponse); // example response
    }
}

public class DeleteOrderResponse : ICommandResponse
{
    public string Status { get; set; }
}

public class DeleteOrderRequest : IInternalCommandRequest<DeleteOrderResponse>, ICommandRequest<DeleteOrderResponse>
{
    async Task<DeleteOrderResponse> IInternalCommandRequest<DeleteOrderResponse>.ExecuteCommand(Sdk sdk)
    {
        // logic goes here
        return await Task.FromResult(new DeleteOrderResponse { Status = "failed"}); // example response
    }
}