9
\$\begingroup\$

So having used the SE API multiple times, I like how all responses are in a Wrapper object, and for my own API design I figured it was a good method to follow.

Of course, I also wanted to abstract everything out so that common actions could be handled in a simple manner, so that I wouldn't need to write the same boilerplate code multiple times. (Wrap the entire response generation in a try/catch block then serialize exceptions to JSON, etc.)

Of course, since the inside of the try/catch block differs for each request, I needed a way to deal with that.

I got the idea to use a Request class which had a ProcessRequest() method that would call a DoWork() method which was implemented differently for each request.

Let's start with Request<T>:

public abstract class Request<T>
    where T : IBaseModel
{
    private ResponseType _responseType;

    public Request(HttpContext context)
    {
        var responseTypeString = context.Request.QueryString["FileType"] ?? string.Empty;
        _responseType = Utilities.Extensions.ResponseTypeExtensions.FromString(responseTypeString);
    }

    public string ProcessRequest()
    {
        try
        {
            var response = DoWork();
            var responseWrapped = BuildWrapper(response);
            var responseString = "";

            switch (_responseType)
            {
                // For the Delimited Serializer types, serialize ONLY the Items. The Delimited Serializer doesn't support serializing graph objects like JSON and XML do.
                case ResponseType.Csv:
                    responseString = DelimitedSerializer.CsvSerializer.Serialize(responseWrapped.Items);
                    break;
                case ResponseType.Psv:
                    responseString = DelimitedSerializer.PsvSerializer.Serialize(responseWrapped.Items);
                    break;
                case ResponseType.Tsv:
                    responseString = DelimitedSerializer.TsvSerializer.Serialize(responseWrapped.Items);
                    break;
                // For the JSON and XML types, serailize the entire response.
                case ResponseType.Json:
                    JsonSerialization.Serialize(responseWrapped, ref responseString);
                    break;
                case ResponseType.Xml:
                    XmlSerialization.Serialize(responseWrapped, ref responseString);
                    break;
            }

            return responseString;
        }
        catch (Exception e)
        {
            return Exception(e);
        }
    }

    private static string Exception(Exception exception)
    {
        if (exception is ArgumentException)
        {
            return ArgumentException((ArgumentException)exception);
        }

        return SerializeWrapper(BuildErrorWrapper(new ExceptionResponse(exception)));
    }

    private static string SerializeWrapper<TItem>(ApiResponseWrapper<TItem> wrapper)
        where TItem : IBaseModel
    {
        string response = "";
        JsonSerialization.Serialize(wrapper, ref response);
        return response;
    }

    private static ApiResponseWrapper<TItem> BuildErrorWrapper<TItem>(TItem item)
        where TItem : IBaseModel
    {
        var wrapper = new ApiResponseWrapper<TItem>();
        wrapper.Items.Add(item);
        wrapper.IsError = true;

        AddRateLimits(wrapper);

        return wrapper;
    }

    private static ApiResponseWrapper<TItem> BuildWrapper<TItem>(IEnumerable<TItem> items)
        where TItem : IBaseModel
    {
        var wrapper = new ApiResponseWrapper<TItem>();
        wrapper.Items.AddRange(items);
        wrapper.IsError = false;

        AddRateLimits(wrapper);

        return wrapper;
    }

    private static void AddRateLimits<TItem>(ApiResponseWrapper<TItem> wrapper)
        where TItem : IBaseModel
    {
        wrapper.QuotaMax = int.MaxValue;
        wrapper.QuotaRemaining = int.MaxValue;
    }

    private static string ArgumentException(ArgumentException exception) => SerializeWrapper(BuildErrorWrapper(new ArgumentExceptionResponse(exception)));

    protected abstract IEnumerable<T> DoWork();
}

The idea is to allow a derived class to implement it's own DoWork() method which will be run by the Request<T>.ProcessRequest() method.

To use it, an IHttpHandler has to simply do the following:

public class SiteHistory : IHttpHandler
{
    public void ProcessRequest(HttpContext context)
    {
        var request = new SiteHistoryRequest(context);
        var response = request.ProcessRequest();

        context.Response.ContentType = "text/plain";
        context.Response.Write(response);
    }

    public bool IsReusable { get { return false; } }
}

Where SiteHistoryRequest would do all the major work and such.

This also means each type of request can be reused for multiple endpoints, with different parameters specified. (The base Request<T> cares not about any of this, it only cares about the core processing.)

Finally, here's what a sample implementation might look like (this endpoint isn't fully implemented, don't review it yet):

public class SiteHistoryRequest : Request<SiteHistoryResponseItem>
{
    private const string _defaultFields = "Date,TotalQuestions,TotalAnswers,QuestionAnswerRate,AnswerAcceptRate,AnswersPerQuestion";

    private string _fields;
    private string _site;

    public SiteHistoryRequest(HttpContext context)
        : base(context)
    {
        _fields = context.Request.QueryString["Fields"] ?? _defaultFields;
        _site = context.Request.QueryString["Site"];
    }

    protected override ApiResponseWrapper<SiteHistoryResponseItem> DoWork()
    {
        if (string.IsNullOrWhiteSpace(_site))
        {
            throw new ArgumentException("The 'Site' parameter is required and cannot be empty.", "Site");
        }

        return new ApiResponseWrapper<SiteHistoryResponseItem>();
    }
}

This satisfies two of my requirements:

  1. All endpoints can use the same code and general implementation, and satisfy the file-type serialization requirements.
  2. All errors will be gracefully handled and returned to the user as a JSON object.

Any comments welcome, though I would greatly appreciate any comments/advice on the design pattern itself.

\$\endgroup\$
2
  • \$\begingroup\$ Well, you should actually already know how to write a good title :-P \$\endgroup\$
    – t3chb0t
    Commented Aug 2, 2016 at 17:04
  • 1
    \$\begingroup\$ @t3chb0t Better? ;) \$\endgroup\$ Commented Aug 2, 2016 at 17:10

2 Answers 2

4
\$\begingroup\$

Using the is operator and if it evaluates to true and then you cast the type the code behind is doing it twice. A better way would involve a soft cast using as and a null check like so

private static string Exception(Exception exception)
{
    var argumentException = exception as ArgumentException;
    if (argumentException != null)
    {
         return ArgumentException(argumentException );
    }

    return SerializeWrapper(BuildErrorWrapper(new ExceptionResponse(exception)));
}  

As mentioned by you is would be faster than as I measured this myself, using the following two methods

static int FindSumWithCast(object[] values)
{

    int sum = 0;
    foreach (object o in values)
    {
        if (o is TestClass)
        {
            int x = ((TestClass)o).Value;
            sum += x;
        }
    }
    return sum;
}

static int FindSumWithAs(object[] values)
{

    int sum = 0;
    foreach (object o in values)
    {
        TestClass x = o as TestClass;
        if (x != null)
        {
            sum += x.Value;
        }
    }
    return sum;
}  

with TestClass looking like so

class TestClass
{
    public int Value { get; }
    public TestClass(int value)
    {
        Value = value;
    }
}

and called both methods in my timing soulution with an object[] created like so

const int Size = 30000000;
object[] values = new object[Size];
for (int i = 0; i < Size - 1; i += 2)
{
    values[i] = "";
    values[i + 1] = new TestClass(1);
}  

In your quote Jon Skeet stated that is would be 20 times faster than as but this seems only if value types are involved.

My mesurements resulted in:

is: 0.095 ms
as: 0.091 ms

but now enough with that.

I thought about this some time and just thought, why don't you just catch the correct exception ?

Like so

public string ProcessRequest()
{
    try
    {
        var response = DoWork();
        var responseWrapped = BuildWrapper(response);
        var responseString = "";

        switch (_responseType)
        {
            // For the Delimited Serializer types, serialize ONLY the Items. The Delimited Serializer doesn't support serializing graph objects like JSON and XML do.
            case ResponseType.Csv:
                responseString = DelimitedSerializer.CsvSerializer.Serialize(responseWrapped.Items);
                break;
            case ResponseType.Psv:
                responseString = DelimitedSerializer.PsvSerializer.Serialize(responseWrapped.Items);
                break;
            case ResponseType.Tsv:
                responseString = DelimitedSerializer.TsvSerializer.Serialize(responseWrapped.Items);
                break;
            // For the JSON and XML types, serailize the entire response.
            case ResponseType.Json:
                JsonSerialization.Serialize(responseWrapped, ref responseString);
                break;
            case ResponseType.Xml:
                XmlSerialization.Serialize(responseWrapped, ref responseString);
                break;
        }

        return responseString;
    }
    catch (ArgumentException e)
    {
        return ArgumentException(e);
    }
    catch (Exception e)
    {
        return Exception(e);
    }
}

private static string Exception(Exception exception)
{
    return SerializeWrapper(BuildErrorWrapper(new ExceptionResponse(exception)));
}
\$\endgroup\$
7
  • 1
    \$\begingroup\$ @EBrown the difference seems to be marginal and barely noticable even with 50.000.000 iterations C# 'is' operator performance - I think next time I use the is because it looks nicer ;-) how did you test the performance? \$\endgroup\$
    – t3chb0t
    Commented Aug 2, 2016 at 19:22
  • \$\begingroup\$ @EBrown In my own testing as is faster than is (though marginally as mentioned). I'd be interested in knowing how you determined the opposite. \$\endgroup\$
    – 404
    Commented Aug 2, 2016 at 23:28
  • \$\begingroup\$ @eurotrash "I'd always assumed that doing one check would be faster than two, but it appears that's not the case, ... When writing a quick benchmark that summed all the integers within an array of type object[], where only a third of the values were actually boxed integers, using is and then a cast ended up being 20 times faster than using the as operator." C# In Depth by Jon Skeet, Page 123. \$\endgroup\$ Commented Aug 3, 2016 at 5:10
  • \$\begingroup\$ @t3chb0t See the above comment. \$\endgroup\$ Commented Aug 3, 2016 at 5:10
  • \$\begingroup\$ @EBrown I'd say this is a good news because I like the is better then then as. \$\endgroup\$
    – t3chb0t
    Commented Aug 3, 2016 at 6:13
6
\$\begingroup\$

Serialization

Where there's a switch there is usually an indication of a missing interface. In your case you could perfectly define a serializer interface:

public interface ISerializer
{
    public ResponseType ResponseType { get; }
    public string Serialize(object items);
}

and make the Request class extendable:

class Request
{
    // or a dictionary or whatever you like
    // you could also pass a collection to the constructor and make it immutable
    public List<ISerializer> Serializers { get; set; }

    public string ProcessRequest()
    {
        return Serializers
            .Where(x => x.ResponseType == abc)
            .FirstOrDefault(x => x.Serialize(input));
    }
}

Constructor

public Request(HttpContext context)

We don't normally use public constructors in abstract classes. It should be protected.


Exception formatting

I don't like the exception formatting that the Request is currently responsible for. It doesn't look like it belongs there. The Request should take an exception formatter via DI. Formatting exceptions is a different matter and it doesn't fit well into a Request.


Naming

return Exception(e);

This is a very misleading and unclear name for a method. To understand what it does I had to first look at its source code. FormatException would be much better because its what it actually does.

IBaseModel

I'm not happy with this name either. It looks like it should rather be the name of the class RequestBase and not of the interface. For an interface a name like IRequest would be fine.

\$\endgroup\$
4
  • \$\begingroup\$ IBaseModel is used in a lot of other places for other things. \$\endgroup\$ Commented Aug 2, 2016 at 17:37
  • \$\begingroup\$ @EBrown could you post it too? I'm curious about what it does. I still think it might be too general. \$\endgroup\$
    – t3chb0t
    Commented Aug 2, 2016 at 17:39
  • \$\begingroup\$ It's literally an empty interface. I only use it for generic restrictions. \$\endgroup\$ Commented Aug 2, 2016 at 17:39
  • 1
    \$\begingroup\$ @EBrown this is a very questionable design ;-] \$\endgroup\$
    – t3chb0t
    Commented Aug 2, 2016 at 17:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.