9
\$\begingroup\$

I wrote an extension method that takes a custom object and converts its properties to a query string for use in a URL. You can specify what properties you want included by using a [QueryString] attribute. For example:

class Thing 
{
    [QueryString]
    public int Id {get; set;}
    [QueryString]
    public string Name {get; set;}
    public object MetaData {get; set;}
}

var thing = new Thing { Id = 1, Name = "Test", MetaData = someObj };
thing.ToQueryString(); // outputs `?Id=1&Name=Test

Here's the code for the extension method:

public static string ToQueryString(this object obj)
{

    StringBuilder queryStringBuilder = new StringBuilder("?");

    Type objType = obj.GetType();
    // Get properties marked with `[QueryString]`
    List<PropertyInfo> props = objType.GetProperties()
        .Where(p => Attribute.IsDefined(p, typeof(QueryStringAttribute)))
        .ToList();

    foreach (var prop in props)
    {
        string name = prop.Name;
        var value = prop.GetValue(obj, null);

        if (value != null)
        {
            // Check's if this is the last property, if so, don't add an '&'
            if (props.IndexOf(prop) != (props.Count - 1))
            {
                queryStringBuilder.Append($"{Uri.EscapeDataString(name)}={Uri.EscapeDataString(value.ToString())}&");
            }
            else
            {
                queryStringBuilder.Append($"{Uri.EscapeDataString(name)}={Uri.EscapeDataString(value.ToString())}");
            }
        }
    }

    return queryStringBuilder.ToString();
}

The code works just fine, but it seems a bit messy to me. Specifically, checking if we are on the last property seems a bit messy, because we end up doing the same thing, just nixing one character. Is there a way to make that a little cleaner? Are there any other way's this method could be simplified?

\$\endgroup\$
  • \$\begingroup\$ Also to force caller to use an attribute is problematic (IMO) because she can't use anonymous types (which might be very handy). \$\endgroup\$ – Adriano Repetti Mar 8 '18 at 15:55
  • \$\begingroup\$ @AdrianoRepetti the reason I did that, is because I figured, you may have certain properties you don't want added in the string. \$\endgroup\$ – an earwig Mar 8 '18 at 15:57
  • 2
    \$\begingroup\$ Of course it depends on the use case(s) but I expect query strings to be small objects (also because I do not want to pollute model with extraneous attributes!) then I'd go the other way (but that's just my opinion, obviously) \$\endgroup\$ – Adriano Repetti Mar 8 '18 at 15:59
  • 1
    \$\begingroup\$ I agree with @AdrianoRepetti, adding attributes is in this case unnecessary. You might also want to create different query strings from the same object. I suggest taking a glimpse at something that I wrote a couple of days ago that can easly be adjusted to produce query strings instead. \$\endgroup\$ – t3chb0t Mar 8 '18 at 16:10
  • \$\begingroup\$ See also: How do I serialize an object into query-string format? \$\endgroup\$ – Ian Kemp Aug 12 at 12:21
8
\$\begingroup\$

In places where the type is obvious (or irrelevant) then feel free to use var instead of the type name. Also, consider not naming your variable after the type; this is also generally not necessary.

I also don't think that you need to make props a list.

A trailing ampersand in the querystring is perfectly legal, so no need to special case it. You can handle it with String.Join though (see my last version below).

Lastly, since you're already using Linq I figure you can go all of the way. You can merge some of your checks into the Where check as well to simplify this.

public static string ToQueryString(this object obj)
{

    var qs = new StringBuilder("?");

    var objType = obj.GetType();

    objType.GetProperties()
           .Where(p => Attribute.IsDefined(p, typeof(QueryStringAttribute)) && p.GetValue(obj, null) != null)
           .ForEach(p => qs.Append($"{Uri.EscapeDataString(p.Name)}={Uri.EscapeDataString(p.GetValue(obj).ToString())}&"));

    return qs.ToString();
}

I ended up keeping the ToList here so I could use the ForEach, but removing that and just looping is fine too.

public static string ToQueryString(this object obj)
{

    var qs = new StringBuilder("?");

    var objType = obj.GetType();

    var properties = objType.GetProperties()
                            .Where(p => Attribute.IsDefined(p, typeof(QueryStringAttribute)) && p.GetValue(obj, null) != null);

    foreach (var prop of properties)
    {
        var name = prop .Name;
        var value = prop .GetValue(obj);

        qa.Append($"{Uri.EscapeDataString(name)}={Uri.EscapeDataString(value)}&"));
    }
    return qs.ToString();
}

Ultimately, I think the cleanest method is to use String.Join and pass the enumerable as the argument. It gets a little long, but there are plenty of ways to change the styling if you don't like that.

public static string ToQueryString(this object obj)
{

    return string.Join("&", obj.GetType()
                               .GetProperties()
                               .Where(p => Attribute.IsDefined(p, typeof(QueryStringAttribute)) && p.GetValue(obj, null) != null)
                               .Select(p => $"{Uri.EscapeDataString(p.Name)}={Uri.EscapeDataString(p.GetValue(obj).ToString())}"));
}
\$\endgroup\$
7
\$\begingroup\$

I wouldn't go the StringBuilder route, simply because you need to handle the "what if I'm at the start/end" problem. Instead, simply collect the "URL_translated" key-value pairs into an IEnumerable<string> and then apply string.join().

If you're working in .NET Core, Microsoft does all the lifting for you. Here's how they do it.

\$\endgroup\$
  • \$\begingroup\$ Ah cool, unfortunately not on .NET Core, but it's nice to know it has such a utility \$\endgroup\$ – an earwig Mar 8 '18 at 15:52
4
\$\begingroup\$

A few ideas

  1. Consider reversing the attribute logic so that you opt OUT of including rather than opt in. If you do it that way, your function will also work with anonymous types and you'll be able to create URLs on the fly very easily. You could even use an existing framework attribute such as IgnoreDataMemberAttribute, which could work well if you plan to use this function on models.

  2. Consider breaking the function up into two different functions. In one function, convert the properties into a dictionary. In the second function, convert a dictionary into a query string. This will give you much more flexibility. With your current setup, you are unable to get the querystring values without going to the trouble of creating the string too.

    static Dictionary<string,object> ToPropertyDictionary(this object o)
    {
        return o.GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance)
            .Where( p => !Attribute.IsDefined(p, typeof(IgnoreDataMemberAttribute))
            .ToDictionary
            (
                p => p.Name,
                p => p.GetValue(o)
            );
    }
    
  3. If you do as I suggest in #2 and break it into two stages, you can easily generate the URL (without the weird logic to add the &) by using String.Join over the dictionary, e.g.

    static string ToQueryString(this object o)
    {
        return string.Join
        (
            "&", 
            o.ToPropertyDictionary()
             .Where( e => e.Value != null)
             .Select
             ( 
                 e => string.Format
                 (
                     "{0}={1}",
                     e.Key, 
                     UrlEncode(e.Value)
                 )
             )
        )
    }
    
  4. Do not include the ? in the results. The caller should be responsible for that. If you do it this way, you can use the results of your function to append to an existing URL that may already have querystring parameters.

  5. You probably do not need to URL-escape the property name, since c# property names can't contain &, space, + or =.

\$\endgroup\$
3
\$\begingroup\$

Several people have suggested turning the loop in to a linq query and using string.join, I wholeheartedly endorse them, but if you’re not ready for that there is a minor change that addresses your concern.

// Check's if this is the last property, if so, don't add an '&'
if (props.IndexOf(prop) != (props.Count - 1))
{
  queryStringBuilder.Append($"{Uri.EscapeDataString(name)}={UriEscapeDataString(value.ToString())}&");
}
else
{
  queryStringBuilder.Append($"{Uri.EscapeDataString(name)}={Uri.EscapeDataString(value.ToString())}");
}

Can easily be rewritten as:

queryStringBuilder.Append($"{Uri.EscapeDataString(name)}={Uri.EscapeDataString(value.ToString())}");
// Check's if this is the last property, if not add an '&'
if (props.IndexOf(prop) != (props.Count - 1))
{
  queryStringBuilder.Append(“&”);
}

But really, if you are going to do this in an explicit loop, it’s a great place to use a ternary assignment:

var queryDelimeter = props.IndexOf(prop) != (props.Count - 1) ? “&” : “”;
queryStringBuilder.Append($"{Uri.EscapeDataString(name)}={Uri.EscapeDataString(value.ToString())}{queryDelimeter}");
\$\endgroup\$
2
\$\begingroup\$

I've found trying to use attributes on a complex model to designate what you want to turn into a query string begins to break down the more that model is used elsewhere. Because not everywhere it is translated to query strings will you ALWAYS want to same set of query strings. So generally you create a model-per-querystring-context. These models make attributes moot as the models are only going to have what they need anyways.

var MyObject = new { 
                FirstName = "John",
                LastName = "Doe",
                MySSN = "111-11-1111",
                MyCC = "4820895069259441",
                MyCVV = "4820895069259441",
                MyDOB = DateTime.Now.AddYears(-20),
                MyImageArray = new byte[]{1,2,3,4,5,6,7,8,9} 
                };

var MyQueryGraph = new {
                                MyObject.FirstName,
                                MyObject.LastName,
                 DOB = MyObject.MyDOB.ToString("MM/dd/yyyy"),
            Image = Convert.ToBase64String(MyObject.MyImageArray)
            };

As far as turning an object to query string, I've used:

using System;
using System.Linq;
using System.Collections.Generic;
using Newtonsoft.Json;

static class Helper
{
    public static string ToQueryString(this object model)
        {
            var serialized = JsonConvert.SerializeObject(model);
            var deserialized = JsonConvert.DeserializeObject<Dictionary<string,string>>(serialized);        
            var result = deserialized.Select((kvp) => kvp.Key.ToString() + "=" + Uri.EscapeDataString(kvp.Value)).Aggregate((p1,p2) => p1 + "&" + p2);
            return result;
        }

}

and then:

MyQueryGraph.ToQueryString();
\$\endgroup\$
  • 2
    \$\begingroup\$ If you're going to down vote, have the courage to say something about it. \$\endgroup\$ – Rex Henderson Mar 9 '18 at 1:09
  • 1
    \$\begingroup\$ It's not my DV but I can imagine it's because of the As far as turning an object to query string, I've used and no further explanation at all. At least this is what I would DV it for if it wasn't already DV. \$\endgroup\$ – t3chb0t Mar 9 '18 at 7:27
  • 1
    \$\begingroup\$ Bringing in a dependency on Newtonsoft JSON (as common as it may be) and then using a serialize/deserialize for converting to a dictionary instead of using reflection is red flag to me. Whether you like the attribute method or not, using reflection like in @Dannnno's last example is far better in my opinion. \$\endgroup\$ – Matthew FitzGerald-Chamberlain Mar 9 '18 at 14:53
  • \$\begingroup\$ I appreciate the responses. I can learn from everybody's perspective and opinion as well :) Well, if you do not already have Newsoft.Json or similar serialization library in your web project, then I would say you are overlooking a powerful capability that makes web communications much more simple. I'm just leveraging what you should already have included and allowing its strength to perform for me what your reflection and enumerations are doing. \$\endgroup\$ – Rex Henderson Mar 9 '18 at 19:18
  • 1
    \$\begingroup\$ I wouldn’t say courage, I would say courtesy. This is Code Review, criticism is expected and should be encouraged. But a downvote without a comment isn’t really helpful. This is the one stack where I wish comments were required before downvotes. \$\endgroup\$ – jmoreno Dec 30 '18 at 3:31

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.