7
\$\begingroup\$

I recently created a custom static class in C# to encode, hopefully, any object it is given (or collection of objects), because I was, at the time, unaware of a native C# library that did the same thing. I am using Generics instead of System.Object because I am aware of the overhead that comes with boxing/unboxing.

After discovering System.Web.Helpers, which had to be installed via NuGet, I discovered their methods usually take in an object and Type, so I assume they're using boxing/unboxing:

public static dynamic Decode(string value);

public static T Decode<T>(string value);

public static dynamic Decode(string value, Type targetType);

public static string Encode(object value);

public static void Write(object value, TextWriter writer);

Here is my code:

public static string ToJson<U, T>(U arg) where U : IEnumerable<T>
{
    StringBuilder json = new StringBuilder();
    json.Append(@"{""data"" : [");

    foreach (var item in arg)
    {// iterate through the IEnumerable

        json.Append("{");
        foreach (var property in typeof(T).GetProperties())
        {// iterate over the properties of the class stored in the IEnumerable argument
            var value = property.GetValue(item, null).ToString();
            if (value == null)// make sure value is not null or empty
                json.AppendFormat(@"""{0}"": """",", property.Name);
            else
            {
                if (value.Contains("\r\n"))
                {// convert the new lines into an array of strings, to be joined when read
                    json.AppendFormat(@"""{0}"": [""{1}""],", property.Name,
                        value.Replace("\"", "\\\"").Replace("\r\n", @""","""));
                    // can be read back by join('\n');
                }
                else
                    json.AppendFormat(@"""{0}"": ""{1}"",", property.Name,
                        value.Replace("\"", "\\\""));
            }
        }
        // remove the trailing comma
        json.Remove(json.Length - 1, 1);
        json.Append("},");// add another row in the array (another dictionary)
    }

    if (json.Length > 11)
    {
        // remove the trailing comma
        json.Remove(json.Length - 1, 1);
        json.Append("]}");
    }
    else
        // no results from sql query:
        json.Replace("[", @"""none""}");

    return json.ToString();
}

Is there something that I'm missing out on, or is my code not optimal/pretty?

\$\endgroup\$
5
  • \$\begingroup\$ ToJson<IEnumerable<string>, string>(new string[] { "Hello" }); throws an exception. \$\endgroup\$
    – RobH
    May 29, 2015 at 13:43
  • \$\begingroup\$ It's a fairly naive implementation, I have to admit. It fits my needs currently, though, and I think if I really want an extensive Json library, I have a few good 3rd party options out there. \$\endgroup\$ May 29, 2015 at 13:48
  • 1
    \$\begingroup\$ I think you should just pick a new one now. It will be a lot easier in the long run as it will be faster and handle correct escaping of all characters. \$\endgroup\$
    – RobH
    May 29, 2015 at 13:57
  • \$\begingroup\$ Yeah, you're probably right. That will probably be much better for the future. \$\endgroup\$ May 29, 2015 at 14:09
  • \$\begingroup\$ 1. The overhead of boxing/unboxing is pretty tiny in comparison to everything else when serialization is concerned. 2. There's nothing wrong with rolling your own (I do it all the time) but if you really need a great JSON serializer Json.NET is the best out there newtonsoft.com/json \$\endgroup\$ May 30, 2015 at 13:25

1 Answer 1

6
\$\begingroup\$

Nice try, however there are potential optimization and code quality improvements.

First obvious thing is handling of the trailing comma.

If you try to generalize your code for the other possible underlying string output component (Stream, for example), you would discover that a string editing operation is not readilly available. Not all the streams support positioning, for example. In a good implementation, i would not expect the trailing comma at all:

Following is a pseudocode with an abstracted Append method.

using (var iterator = items.GetEnumerator())
{
    if (iterator.MoveNext())
        Append(json, iterator.Current);
    while (iterator.MoveNext())
    {
        Append(json, ',');
        Append(json, iterator.Current);
    }
}

In this code, json could be a StringBuilder or a Stream, or whatever your implementation would use.

Using formatting is a performance killer.

Append format creates at least one excess array object on heap (as for the current implementation of NET 4.5), and parses the format string, which is suboptimal. If you want your code to be performant, use just the append method of the string builder:

json.Append('[').Append(value).Append(']');

Following part makes feeling like i would never rely on such an implementation:

if (json.Length > 11)

It's a bad way of checking for an empty collection and a bad usage of magic constant in your code. Solve the former, and get rid of the latter.

Using reflection is another performance killer. There are many techniques to do all the reflection on a type beforehand, and then use a compiled delegate. You can find information on Expression.Compile, which uses Assembly.Emit to generate code dynamically at runtime only the first time a particular type serialization is used.

I would say code comments inside methods lead to unmaintainable code, and the comments like:

// iterate through the IEnumerable

..

if (value == null)// make sure value is not null or empty

..

json.Append("},");// add another row in the array (another dictionary)

...are pretty much useless.

Just assume that every person reading your code knows that foreach means iterating an ienumerable, if means a conditional statement, and } in json means the end of dictionary.

Using comments on parts of the code blocks inside a method indicates that the method could be composed of smaller ones with descriptive names and possibly add xmldoc comments:

    /// <summary>
    /// Appends multiline value <paramref name="value"/> with special formatting based on newline charactes.
    /// </summary>
    /// <param name="stringBuilder">Output builder.</param>
    /// <param name="value"></param>
    public void AppendMultiline(StringBuilder stringBuilder, string value)
    {
        // ...
    }
\$\endgroup\$
3
  • \$\begingroup\$ Thanks a lot for your answer! That was very informative, and I'm happy to have the criticism. Are these things that you have just learned with time, or is there something I can read up on to enhance my programming skills? \$\endgroup\$ May 29, 2015 at 19:36
  • \$\begingroup\$ "Using comments on parts of the code blocks inside a method indicates that the method could be composed of smaller ones". That's a really good point; I'll have to be more aware of that in the future. \$\endgroup\$ May 29, 2015 at 19:37
  • \$\begingroup\$ I'd recomment 'Refactoring to Patterns' by Joshua Kerievsky martinfowler.com/books/r2p.html for code quality questions and 'CLR via C#' by Jeffrey Richter amazon.com/CLR-via-Edition-Developer-Reference/dp/0735667454 to understand CLR performance. \$\endgroup\$ May 29, 2015 at 19:44

Your Answer

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

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