Handling Server Responses

Question

I'm not 100% sure this is the right place to post this. It is not about the specific code being written, but more about the design of the code. (Of course any constructive criticism is appreciated)

I have the following interfaces for dealing with talking to a server:

public interface IAction
{
    GatewayResponseHandlerResult Run();
}

public abstract class GatewayResponseHandler
{
    protected GatewayResponseHandlerResult Result = new GatewayResponseHandlerResult();

    private GatewayResponseHandlerResult Handle(GatewayResponse Response)
    {
        //specific handling code omitted
    }
}

The idea is each type of action (ex. Login, Logout) is its own class that implements the IAction interface. The action performs the call to the servers and passes the response to the GatewayResponseHandler.Handle() function which then passes back a GatewayResponseHandlerResult object. The specific IAction object then passes the GatewayResponseHandleResult object back to the caller.

This is the GatewayResponseHandlerResult class:

public class GatewayResponseHandlerResult
{
    public bool Succeeded;
    public bool Failed;
    public bool Errored;
    public object Result { get; set; }

    public void SetAsSuccess()
    {
        Succeeded = true;
        Failed = false;
        Errored = false;
    }

    public void SetAsFailure()
    {
        Succeeded = false;
        Failed = true;
        Errored = false;
    }

    public void SetAsError()
    {
        Succeeded = false;
        Failed = false;
        Errored = true;
    }
}

This is what an Action would look like:

public sealed class LoginAction : IAction
{
    private string Username;
    private string Password;

    public LoginAction(string Username, string Password)
    {
        this.Username = Username;
        this.Password = Password;
    }

    public GatewayResponseHandlerResult Run()
    {
        try
        {
            GatewayResponse LoginResponse = GatewayRequest.Login(Username, Password);
            return GatewayResponseHandler.Handle(LoginResponse);
        }
        catch (Exception ex)
        {
            throw ex.Log();
        }
    }
}

And the corresponding derived response handler:

sealed class LoginHandler : GatewayResponseHandler
{
    protected override bool CanHandle()
    {
        return Response.action == GatewayAction.LOGIN;
    }

    protected override GatewayResponseHandlerResult ProcessError(Exception ex = null)
    {
        Result.SetAsError();
        return Result;
    }

    protected override GatewayResponseHandlerResult ProcessFailure()
    {
        Result.SetAsFailure();
        return Result;
    }

    protected override GatewayResponseHandlerResult ProcessSuccess()
    {
        try
        {
            if (Response.result_code == (int)ResponseCodes.SuccessCode.MFA_REQUIRED)
            {
                //specific MFA handling omitted
            }
            else if (Response.result_code == (int)ResponseCodes.SuccessCode.PW_RESET_REQUIRED)
            {
                //specific password reset code omitted
            }

            MessageBox.Show("Login successful");
            Result.SetAsSuccess();
        }
        catch (Exception Ex)
        {
            ProcessError(Ex);
            Result.SetAsError();
        }
        return Result;
    }
}

As you can see the LoginResponseHandler can need to deal with several possible responses from a successful login since it can require an MFA code to be passed back to complete the login or a password reset might be required on login. This can mean running new Actions. Should the LoginResponseHandler be executing those directly or passing back something to let the caller of the action know they need to perform further Actions?

I'm also not sure if I should have the Actions be instantiated, or perhaps make the Run() method static and have some sort of abstract argument object with specific implementation for each action that actually need it.

For example:

public interface IActionArgs{}

public class LoginActionArgs : IActionArgs 
{
    public string Username;
    public string Password;
}

public abstract class Action
{
    public static GatewayResponseHandlerResult Run(IActionArgs ActionArgs)
    {
        throw new NotImplementedException();
    }
}

public sealed class LoginAction : IAction 
{
    public static new GatewayResponseHandlerResult Run(IActionArgs ActionArgs)
    {
        //code omitted
    }
}

While I like the idea of the methods being static, I don't like the way the code looks, and the fact that you could call Run on the abstract class since interface can't have static methods.

Any general advice on how to improve the architecture of the code is very much appreciated.

Answer 1

You don't show enough code to give you accurate advice on design. But from what I can see, LoginAction class has a bunch of problems:

1) It has two huge static dependencies that are not immediately apparent. Hard to read, hard to unit test.

2) Run method is synchronous. IMHO, network-related code should never block, and it should not be consumer's responsibility to call Run on thread pool. It's your connection implementation that should provide asynchronous api.

3) Since there is a GatewayRequest.Login, I assume there is also GatewayRequest.LogOut, GatewayRequest.DoThis, GatewayRequest.DoThat, etc. Having one method per request will not be that bad for small protocols, but it scales poorly.

4) Storing password in memory in plain text is a security vulnerability, but if the attacker gets access to your memory, you are probably screwed anyway. However sending it in plain text over network is much riskier (since it is much easier to access your network traffic than your memory). Make sure to either encrypt it before sending or to use an encrypted connection (or both).

5) Run method looks like something, that will be copy-pasted over and over again across other IAction implementations with very little modification. Which is a sign of poor design. From your explanations it is not clear why can't you have a message-based api, that does not know and does not care, which type of request you are trying to send:

//pseudocode
public async Task<GatewayResponseHandlerResult> SendAsync(IRequestMessage request)
{
    try
    {
        IResponseMessage response = await _connection.SendAsync(request);
        GatewayResponseHandler handler = _handlers.First(h => h.CanHandle(response));
        return handler.Handle(response);
    }
    catch (Exception ex)
    {
        ex.Log();
        throw;
    }
}

//instead of implementing lots of `IAction`s, you can now call
await _client.SendAsync(new LoginMessage{User = "A", Password = "B"});
await _client.SendAsync(new LogoutMessage{...});

where LoginMessage is:

interface IRequestMessage
{
    RequestCode Code {get;}
    void Write(BinaryWriter writer);
}

class LoginMessage : IRequestMessage
{
    public string User {get;set;}
    public string Password {get;set;}

    public RequestCode Code { get { return RequestCode.Login; } }
    //converts message to binary representation, according to your protcol
    public void Write(BinaryWriter writer)
    {
        writer.Write((int)Code)
        writer.Write(User);
        writer.Write(Password);
    }
}

Answer 2

Having bool Succeeded, bool Failed and bool Errored as members of the GatewayResponseHandlerResult class is a very bad practice. Not to mention that all of them are public mutable so that any code can invalidate the result state, like setting all of them to true of false.

Use an enum for this, clean and elegant. It also removes the need of separate methods, as the enum can simply be passed in the constructor. The class should also be made immutable as I don't see a need to ever change a result once it's constructed.

I also don't see the Result property of GatewayResponseHandlerResult ever being used. Using only one instance of GatewayResponseHandlerResult and mutating it for each call is not thread safe. Feels like the entire GatewayResponseHandlerResult class can be replaced by the enum mentioned above.

As to Response, the code of the class is not provided but it seems to suffer from poorly named fields like result_code (C# uses CamelCase not snake_case).

Moving on to the actions, accepting IActionArgs arguments where the interface provides no information is no more useful than accepting a simple object. This part needs to be redesigned.

Answer 3

I personally prefer something like a generic ActionResult<T> class that allows me the re-usability of a single data structure for handling responses that might fail.

The class would look something similar like:

public class ActionResult {
  public bool Succeeded { get; }
  public string Message { get; }

  public ActionResult(bool succeeded = true, string message = "") {
    Succeeded = succeeded;
    Message = message;
  }

  public static implicit operator bool(ActionResult result) {
    return result.Succeeded;
  }

  public static implicit operator string(ActionResult result) {
    return result.Message;
  }
}

An then you can add a more generic interface like so

public class ActionResult<ResultType> : ActionResult {
  public ResultType Value { get; }

  public ActionResult(ResultType result, bool succeeded = true, string message = "") : base( succeeded, message ) {
    Value = result;
  }

  public static implicit operator ResultType(ActionResult<ResultType> result) {
    return result.Value;
  }
}

I personally feel this is appropriately flexible for any response I would like to use, and I can add a resource key for translatable messages in the message string. If the request fails, it's easy enough to construct my response with null as a value parameter.

An advantage of using this setup would be that you can use the ActionResult in the following way:

var result = DoSomethingThatReturnsResult<ResponseType>();
if (result) {
   // succeeded, return ResponseType
   return result.Value; 
}
return ErrorResponse(result.Message);

Answer 4

It is not about the specific code being written

but I think it probably should too because one part that you left there is very suspicious.

catch (Exception ex)
{
    throw ex.Log();
}

We usually don't do that because you are creating a new stack-trace here. To rethrow the exception should should just use the throw keyword, without the ex:

catch (Exception ex)
{
    ex.Log();
    throw;
}

This will log the exception but also preserve the original stack-trace (which is what you actually always want).

I think you should not have ommited the implementation because it currently needs more improvement then the design.