Reduce casting and better styling

Question

Consider the following (nasty) code:

    /// <summary>
    /// Calls matching process error code on response.Code
    /// </summary>
    /// <param name="response">Actually will be of type Response or extend it</param>
    /// <returns>true for successful response, false otherwise</returns>
    private static bool ProcessErrorCode(object response)
    {
        bool isOkay = false;
        const string unknown = "UNKNOWN";
        string errCode = unknown;
        if (response.GetType() == typeof(Response<AResponseCode>))
        {
            AResponseCode code = ((Response<AResponseCode>)response).Code;
            isOkay = code == AResponseCode.Ok;
            errCode = code.ToString();
        }
        if (response.GetType() == typeof(Response<BResponseCode>))
        {
            BResponseCode code = ((Response<BResponseCode>)response).Code;
            isOkay = code == BResponseCode.Ok;
            errCode = code.ToString();
        }
        if (response.GetType() == typeof(DataResponse<CResponseCode,string>))
        {
            CResponseCode code = ((DataResponse<CResponseCode, string>)response).Code;
            isOkay = code == CResponseCode.Ok;
            errCode = code.ToString();
        }
        if (isOkay)
        {
            return true;
        }
        string msg = "Operation resulted in error code:" + errCode;
        LogErrorCode(msg);
        return false;
    }

I am trying to figure out a way to reduce castings and imrove the method style. I have no code ownership on Response<TResponseCode>, DataResponse<TResponseCode,string>, AResponseCode, BResponseCode, CResponseCode

response parameter will be of type Response<TResponseCode> or inherit from it (DataResponse<TResponseCode,string> : Response<TResponseCode>)

All *ResponseCode are Enums and they all have an *ResponseCode.Ok entry

Answer 1

I'd swap a switch statement for all the ifs.

Answer 2

Ideally, you'd give Response<TResponseCode> a common interface with an 'OK' function on it. Seeing as you can't, your solution is going to look a bit hacky.

Given that constraint, I'd extract a couple of methods - static bool IsResponseOk(object response) and static string GetResponseError(object response) - which would result in easier to read code, but still not brilliant.

Answer 3

This replaces my old answer as it clearly bombed :) (them's the breaks)

I don't expect this'll do any better - but you might find the approach interesting.

The basic idea is to define a result type that contains your IsOkay and ErrCode values, and then define a dictionary of delegates (keyed by the type of the object that they handle) that you consult. To add new handlers you simply add another delegate to the dictionary.

public class ResponseResult{
  public bool IsOkay;
  public string ErrCode;
}

public static class ResponseExtracter
{
  //STARTING OFF HERE WITH THE NEW VERSION OF YOUR METHOD
  public static bool ProcessErrorCode(object response)
  {
    Func<object, ResponseResult> processor = null;
    ResponseResult result = new ResponseResult() 
    { 
      IsOkay = false, ErrCode = "UNKNOWN"
    };

    //try to get processor based on object's type
    //then invoke it if we find one.
    if (_processors.TryGetValue(response.GetType(), out processor))
      result = processor(response);

    if (result.IsOkay)
      return true;
    string msg = "Operation resulted in error code:" + result.ErrCode;
    LogErrorCode(msg);
    return false;
  }
  //A lot better no?

  //NOW FOR THE INFRASTRUCTURE
  static Dictionary<Type, Func<object, ResponseResult>> _processors 
    = new Dictionary<Type, Func<object, ResponseResult>>();

  static ResponseExtracter()
  {
    //this can be replaced with self-reflection over methods
    //with attributes that reference the supported type for
    //each method.
    _processors.Add(typeof(Response<AResponseCode>), (o) =>
    {
      AResponseCode code = ((Response<AResponseCode>)o).Code;
      return new ResponseResult
      {
        IsOkay = code == AResponseCode.Ok,
        ErrCode = code.ToString()
      };
    });

    _processors.Add(typeof(Response<BResponseCode>), (o) =>
    {
      BResponseCode code = ((Response<BResponseCode>)o).Code;
      return new ResponseResult
      {
        IsOkay = code == BResponseCode.Ok,
        ErrCode = code.ToString()
      };
    });

    _processors.Add(typeof(DataResponse<CResponseCode, string>),
      (o) =>
      {
        CResponseCode code = ((DataResponse<CResponseCode, string>)o).Code;
        return new ResponseResult
        {
          IsOkay = code == CResponseCode.Ok,
          ErrCode = code.ToString()
        };
      });
  }
}

Performance-wise it's better than it looks because the Dictionary<Type, TValue> is actually hashing on the integer value that underpins the type's TypeHandle. This is in fact an Int64 - so only the first 32 bits are used, but in practise it's highly unlikely that two types share the same first 32 bits of their handle.

Answer 4

From a performance perspective, I'd replace .GetType() == typeof() with the is operator, eg:

if( response is DataResponse<CResponseCode,string> )

The following benchmark shows a performance improvement of roughly 3x on my machine:

var f = new List<string>();
var dummy = 0;

Stopwatch sw = new Stopwatch();

sw.Start();
for(int i=0; i< REPS; i++)
    if(f.GetType() == typeof( List<string> )) dummy++;
sw.Stop();
Console.WriteLine(sw.Elapsed);

sw.Reset();

sw.Start();
for(int i=0; i< REPS; i++)
    if(f is List<string> )  dummy++;
sw.Stop();

Console.WriteLine(sw.Elapsed);

// Outputs
00:00:00.0750046
00:00:00.0261598

Answer 5

You could use a generic subroutine for this:

    // Mocking your classes, just to check if it compiles. You don't need this.
    class Response<T> { 
        public T Code { get { return default(T); } } 
    }
    enum AResponseCode { Ok }
    enum BResponseCode { Ok }
    enum CResponseCode { Ok }
    static void LogErrorCode(string msg) { }

    private static bool ProcessErrorCode(object response)
    {
        bool isOkay;
        string errCode;

        if (!TryProcessErrorCode(response, AResponseCode.Ok, out isOkay, out errCode))
            if (!TryProcessErrorCode(response, BResponseCode.Ok, out isOkay, out errCode))
                TryProcessErrorCode(response, CResponseCode.Ok, out isOkay, out errCode);

        if (isOkay)
        {
            return true;
        }
        string msg = "Operation resulted in error code:" + errCode;
        LogErrorCode(msg);
        return false;
    }

    // TResponseCode is automatically inferred by passing the okCode
    private static bool TryProcessErrorCode<TResponseCode>(
        object response, TResponseCode okCode, 
        out bool isOkay, out string errCode)
    {
        var resp = response as Response<TResponseCode>;
        if (resp == null)
        {
            isOkay = false;
            errCode = "UNKNOWN";
            return false;
        }
        else
        {
            isOkay = okCode.Equals(resp.Code);
            errCode = resp.Code.ToString();
            return true;
        }
    }

Answer 6

Refactored thus, and I'd have been faster if IE hadn't freaked out and reloaded the page spontaneously. :(

I agree with others that this ideally should be gutted and mocked/encapsulated/extended to get where you want, but working within the method as a point of least impact and as an instructional:

  bool isOkay = false; 
  string errCode; // you don't need to default this to anything because it won't be empty if you need to use it

  if (response is Response<AResponseCode>) // "is" is a faster and more expressive test
  { 
    var code = (response as Response<AResponseCode>).Code; // "as" is a faster conversion
    isOkay = (code == AResponseCode.Ok); // Readability is awesome!
    errCode = code.ToString(); // This still sucks
  } 
  else if (response is Response<BResponseCode>) // elsif logically exclusive tests
  { 
    var code = (response as Response<BResponseCode>).Code; 
    isOkay = code == BResponseCode.Ok; 
    errCode = code.ToString(); 
  } 
  else if (response is DataResponse<CResponseCode,string>) 
  { 
    var code = (response as DataResponse<CResponseCode, string>).Code; 
    isOkay = (code == CResponseCode.Ok); 
    errCode = code.ToString(); 
  } 

  // Test what you mean to express, i.e. if this isn't ok I need to log it
  if (!isOkay) 
  { 
    // you don't need a temp variable here...
    // and why aren't you just throwing an exception here? Is not being ok a regular unexceptional event, but one you still need to log? Really?
    LogErrorCode("Operation resulted in error code:" + errCode);
  } 

  // And try not to return bool literals, it's again unexpressive
  return isOkay;

Answer 7

You can get a slightly cleaner solution using generics:

private static void TestErrorCode<TCode>(object response, TCode ok, ref bool isOkay, ref string errCode)
{
    Response<TCode> responseTyped = response as Response<TCode>;

    if (responseTyped == null)
    {
        return;
    }

    TCode code = responseTyped.Code;
    isOkay = code.Equals(ok);
    errCode = code.ToString();
    return;
}

private static bool ProcessErrorCode(object response)
{
    bool isOkay = false;
    string errCode = "UNKNOWN";

    TestErrorCode(response, AResponseCode.Ok, ref isOkay, ref errCode);
    TestErrorCode(response, BResponseCode.Ok, ref isOkay, ref errCode);
    TestErrorCode(response, CResponseCode.Ok, ref isOkay, ref errCode);

    if (isOkay)
    {
        return true;
    }

    LogErrorCode("Operation resulted in error code:" + errCode);
    return false;
} 

Answer 8

I can't imagine to do something with this without finding person responsible for writing this XResponseZZZ. Maybe others are smarter than me. But im convinced that you should find the guy and presuade to him that it should be XResponse responsibility to known if specified code is okay and what error message should be thrown. This is C coding style and it is done bad.

I looked at the Heinzi or Daniel generic solutions and I like them most.

Answer 9

Here's a solution with dynamic (can be edited to use reflection instead), untested and not recommended, but short.

It should work identically to your original function:

• Since you rely on response.GetType() == typeof(XXX), which is not the same as response is XXX (inheritance is excluded), types.Contains(response.GetType()) is equivalent.
• Since you guarantee that all these types have a property Code which is (presumably) an enum type that contains a value Ok, the dynamic portion should always succeed.
• The fact that only the eligible types are included means that 'coincidences' cannot happen.

Cons:

• Performance?
• If the underlying types change, you will not be protected at compile-time.

---

private static bool ProcessErrorCode(object response)
{
    var types = new[] { typeof(Response<AResponseCode>), typeof(Response<BResponseCode>), typeof(DataResponse<CResponseCode, string>)};
    var errCode = !types.Contains(response.GetType())
                  ?"UNKNOWN"
                  :(string)(((dynamic)response).Code.ToString());

    if(errCode == "Ok") return true;

    LogErrorCode("Operation resulted in error code:" + errCode);
    return false;
}

Answer 10

I would be inclined to try to get the main body of code quite simple like this:

/// <summary>
/// Calls matching process error code on response.Code
/// </summary>
/// <param name="response">Actually will be of type Response or extend it</param>
/// <returns>true for successful response, false otherwise</returns>
private static bool ProcessErrorCode(object response)
{
    Func<Type, Func<object, string>> process = ...;
    var errCode = process(response.GetType())(response);
    if (errCode != "Ok")
    {
        LogErrorCode("Operation resulted in error code:" + errCode);
    }
    return errCode == "Ok";
}

Then it just becomes a matter of defining the Func<Type, Func<object, string>>. This could be done using a separate method or by dependency injection.

The separate method would look like this:

private static Func<Type, Func<object, string>> _process = null;
private static Func<Type, Func<object, string>> GetProcessFunc()
{
    if (_process == null)
    {
        var d = new Dictionary<Type, Func<object, string>>()
        {
            { typeof(Response<AResponseCode>), r => ((Response<AResponseCode>)r).Code.ToString() },
            { typeof(Response<BResponseCode>), r => ((Response<BResponseCode>)r).Code.ToString() },
            { typeof(DataResponse<CResponseCode,string>), r => ((DataResponse<CResponseCode,string>)r).Code.ToString() },
        };
        _process = t =>
        {
            if (d.Contains(t))
            {
                return o => d[t];
            }
            return o => "UNKNOWN";
        };
    }
    return _process;
}

This still has the same code, per se, but it is now better separated and can be replaced with a dependency injection approach more easily. :-)