4
\$\begingroup\$

I'm looking for a code correctness and best practices review on the following. I created the following in order to prevent using the same code to convert serialized JSON to something more accessible in my HttpHandlers without incorporating this directly into my base handler. I don't always need to post over full entities when only a few values are needed.

I'm aware the empty catch statements should be avoided -- my version has additional content.

/// <summary>
/// Returns a collection of parameters passed via JSON 
/// </summary>
/// <param name="request">HttpRequest</param>
/// <returns>Dictionary<typeparamref name="string"/>,<typeparamref name="object"/> of key, value (string, object) pairs.</returns>
public static Dictionary<string, object> Json(this HttpRequest request)
{
    // Per request caching.
    var jsonCollection = HttpContext.Current.Items["jsonData"] as Dictionary<string, object>;

    if (request.ContentType.StartsWith("application/json") || request.ContentType.StartsWith("text/json"))
    {
        if (jsonCollection == null)
        {
            string jsonData = string.Empty;
            using (StreamReader reader = new StreamReader(request.InputStream))
            {
                if (!reader.EndOfStream)
                {
                    try
                    {
                        jsonData = reader.ReadToEnd();
                    }
                    catch (OutOfMemoryException ex)
                    {

                    }
                    catch (IOException ex)
                    {

                    }
                    finally
                    {
                        jsonCollection = JsonConvert.DeserializeObject<Dictionary<string, object>>(jsonData);
                        HttpContext.Current.Items["jsonData"] = jsonCollection;
                    }

                }
            }
        }
    }
    return jsonCollection;
}

/// <summary>
/// Get Element by index
/// </summary>
/// <param name="request"></param>
/// <param name="index"></param>
/// <returns></returns>
public static object Json(this HttpRequest request, int index)
{
    object obj = null;
    try
    {
        obj = request.Json().ElementAt(index);
    }
    catch (KeyNotFoundException ex)
    {
        obj = null;
    }

    return obj;
}

/// <summary>
/// Get element by key
/// </summary>
/// <param name="request"></param>
/// <param name="key"></param>
/// <returns></returns>
public static object Json(this HttpRequest request, string key)
{
    object obj = null;
    try
    {
        obj = request.Json()[key];
    }
    catch (KeyNotFoundException ex)
    {
        obj = null;
    }

    return obj;
}

07/22/2014 Quick (Read: Untested) Generic Typed HttpRequest.Json

public static T GetValue<T>(this IDictionary collection, string key)
{
    if (collection == null)
    {
        return default(T);
    }

    var value = collection[key];

    if (value == null)
    {
        return default(T);
    }

    var type = typeof(T);

    if (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>))
    {
        type = Nullable.GetUnderlyingType(type);
    }

    var converter = TypeDescriptor.GetConverter(type);

    if (!converter.CanConvertTo(value.GetType()))
    {
        return default(T);
    }

    return (T)converter.ConvertTo(value, type);
}


public static T Json<T>(this HttpRequest request)
{
    // Per request caching -- See GetValue<T> above.
    T jsonCollection = HttpContext.Current.Items.GetValue<T>("jsonData");

    if (request.ContentType.StartsWith("application/json") || request.ContentType.StartsWith("text/json"))
    {
        if (jsonCollection == null)
        {
            string jsonData = string.Empty;
            using (StreamReader reader = new StreamReader(request.InputStream))
            {
                if (!reader.EndOfStream)
                {
                    try
                    {
                        jsonData = reader.ReadToEnd();
                    }
                    catch (OutOfMemoryException ex)
                    {

                    }
                    catch (IOException ex)
                    {

                    }
                    finally
                    {
                        jsonCollection = JsonConvert.DeserializeObject<T>(jsonData);
                        HttpContext.Current.Items["jsonData"] = jsonCollection;
                    }

                }
            }
        }
    }
    return jsonCollection;
}
\$\endgroup\$
  • \$\begingroup\$ What does this mean: "I don't always need to post over full entities when only a few values are needed."? Why wouldn't this go in your base handler? \$\endgroup\$ – Pierre Menard Jul 18 '14 at 17:22
  • \$\begingroup\$ @PierreMenard Assuming I had a collection of employees where the model is as follows: public class Employee { public int ID {get;set;} public string Name {get;set;} public int Age {get;set;} // Other properties below } I wouldn't need to submit the entire view model when all I need is the ID, thus I'll stringify the following: { ID: employee.ID } where 'employee' is the entity selected. \$\endgroup\$ – Code.Combustion Jul 21 '14 at 18:57
  • \$\begingroup\$ @PierreMenard I could in theory put this in my base handler however it makes more since to me to have this as a possibility on all HttpRequest objects similarly in function to HttpRequest.Form and HttpRequest.QueryString. This leaves the option of accessing the collection from other sources that don't use my base handler. I have the base handler setup to follow an MVC convention in an existing Web Forms application as in the future, we intend to transition to a full MVC implementation. \$\endgroup\$ – Code.Combustion Jul 21 '14 at 18:57
  • \$\begingroup\$ I get what you mean by not posting everything over, but how does that impact the design here? \$\endgroup\$ – Pierre Menard Jul 21 '14 at 23:36
  • \$\begingroup\$ I toyed with adding a type parameter so I could specify a view model type instead of object and then would have the JSON string de-serialized into the view model/s as needed. My comment may not make as much since without the context. \$\endgroup\$ – Code.Combustion Jul 22 '14 at 17:42
1
\$\begingroup\$

I'll leave the meat for others, but here are some quick improvements on your indexing methods.
Try-catch is slow and to be avoided if possible. Here is how I would do it.

/// <summary>
/// Get Element by index
/// </summary>
/// <param name="request">HttpRequest</param>
/// <param name="index"></param>
/// <returns></returns>
public static object Json(this HttpRequest request, int index)
{
    var json = request.Json();
    if (json.Count() < index + 1)
    {
        return null;
    }

    return json.ElementAt(index);
}


/// <summary>
/// Get element by key
/// </summary>
/// <param name="request">HttpRequest</param>
/// <param name="key"></param>
/// <returns></returns>
public static object Json(this HttpRequest request, string key)
{
    object obj = null;
    request.Json().TryGetValue(key, out obj);
    return obj;
}
\$\endgroup\$
0
\$\begingroup\$

The implementation is fine but in my opinion the design is questionable. Any time you return a poorly-typed bag like a Dictionary<String, Object> you should think about how well you've encapsulated. The handler object is the only one that knows about the argument types it's expecting, right? So it should deserialize the arguments into some business object like Employee[] and not a dictionary that holds an array of dictionaries.

It's hard to say what kind of an interface you should have, since you haven't provided any use cases. Maybe the deserialize function could be generic and each subclass could specify a type that it generates.

\$\endgroup\$
  • \$\begingroup\$ Please see my above comment -- I think it'll clarify things. The Dictionary<String, Object> is for partial entities to which I may not have an appropriate view model. I haven’t had the time to continue my typed implementation but I do intend to as I find the approach much more palatable than a using statement with a stream reader obtaining data from the HttpRequest.InputStream then continuing to ReadLine and de-serializing the result. \$\endgroup\$ – Code.Combustion Jul 22 '14 at 17:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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