5
\$\begingroup\$

I have written a very simple web service with MVC and WebApi. Now I'm working on the client code which will be a WPF application (and soon Windows 8 Store/Phone app). What I have done works, but I'm not sure I'm doing it the "right" way. The purpose of the service is to check if there are any new software updates to the client.

My server code looks like this (simplified):

public class ProductVersionsController : ApiController
{
    private ApplicationDbContext db = new ApplicationDbContext();

    [HttpGet]
    public CheckVersionResult CheckVersion(string product, string platform, string version)
    {
        CheckVersionResult result = new CheckVersionResult();

        //Logic removed...

        return result;
    }
}

My client code looks like this (simplified):

string parameters = "product=myproduct&platform=wpf&version=1.2.3.4";
string CheckUrl = "http://localhost:61933/api/ProductVersions/CheckVersion";

var url = new Uri(CheckUrl + "?" + parameters);
using (var client = new System.Net.WebClient())
{
    var json = await client.DownloadStringTaskAsync(url);

    CheckVersionResult data = JsonConvert.DeserializeObject<CheckVersionResult>(json);

    //Logic removed...
}

The deserializing is done with Json.net.

Should I use HttpGet for a service like this, or should I use Post to send the parameters? How the parameters are sent feels a bit clumsy. If the parameters were encapsulated in a class, how should that be solved on the client side? Is there any good practice I have missed?

\$\endgroup\$
  • \$\begingroup\$ Have you considered using something like WCF, where a method invocation is translated into a network request automatically? \$\endgroup\$ – svick Jul 19 '14 at 23:20
  • \$\begingroup\$ I know barely anything about WCF but I guess there is no support for that in Windows 8 store apps (I missed to mention that - question updated). But I'm gladly wrong about this :-). \$\endgroup\$ – PEK Jul 20 '14 at 6:41
  • \$\begingroup\$ What did you simplify exactly? Is the parameters string usually built up in code rather than hard-coded like that? \$\endgroup\$ – Ben Aaronson Jul 20 '14 at 11:01
  • \$\begingroup\$ For now the parameters string is build like that, which I don't like an plan to improve. Otherwise my only simplification is that I have removed code that isn't directly related to the communication. \$\endgroup\$ – PEK Jul 20 '14 at 19:45
  • \$\begingroup\$ @PEK I would use GET as your not changing anything on the serverside. \$\endgroup\$ – dreza Jul 21 '14 at 1:18
6
\$\begingroup\$

Simplified, examplified, trimmed code is frowned upon on this site; I'm surprised this question hasn't received any close votes yet.

I don't see anything blatantly done wrong here - I like . On the other hand if the client code you've shown is cohesively written in a specialized service (class) with a clear, focused interface that your ViewModel receives as a constructor parameter and assigns to a private readonly field, then you've probably done the part right.

If that very same code is written in some Button1_Click handler in your View's code-behind, it could be the most cleverly written, beautifully crafted little piece of code, it would still be horribly misplaced.

</rant>


I still have a couple of things to say about your code:

private ApplicationDbContext db = new ApplicationDbContext();

The db identifier is misleading. I prefer to use context over db, because db refers to a database; a context, or data context, is more accurate. That said, by newing up your context inside your controller, you have tightly coupled your controller with the specific ApplicationDbContext class.

The Controller class implements IDisposable; you should override the Dispose method and properly dispose of your ApplicationDbContext instance.

An alternative would be to depend on an abstraction instead:

private readonly IDbContext context;

The dependency can then be constructor-injected, and the Controller doesn't care what the actual implementation is, it's simply calling methods defined by some interface.

This would greatly improve the controller's testability.


CheckVersionResult result = new CheckVersionResult();

I'm not a fan of this verbose notation; I find result is screaming its type at me, and I don't need that.

var result = new CheckVersionResult();

I find var makes more concise code that literally reads as "there's a variable here called result that we're going to assign to a new instance of the CheckVersionResult type" - as opposed to "there's a variable here of type CheckVersionResult called result that we're going to assign to a new instance of the CheckVersionResult type". But it boils down to personal preference I guess.

What isn't about personal preference, is consistency. Why is the client code different?

var url = new Uri(CheckUrl + "?" + parameters);
using (var client = new System.Net.WebClient())
{
    var json = await client.DownloadStringTaskAsync(url);

..and the next line goes:

CheckVersionResult data = JsonConvert.DeserializeObject<CheckVersionResult>(json);

I really prefer var I don't mind either notation, but using both interchangeably is wrong.


I like that you're using await. This means you've written this code in an async method. That method's name should return a Task or a Task<T>, and have an Async suffix in its name, to follow convention.

| improve this answer | |
\$\endgroup\$
4
\$\begingroup\$

Now I will answer myself, hopefully someone else has use of this.

When it comes to the server side, I'll let it be like it is. Using HttpGet could be useful for caching so that's a good thing.

The client code works but it's a bit messy so decided to clean it up with some simple helper classes. In these classes I'm using HttpClient instead of WebClient. WebClient isn't available on Windows Phone/Store apps, but HttpClient could be used if in wpf if you put a reference to System.Net.Http.

I have also replaced Json.net with System.Runtime.Serialization.Json.DataContractJsonSerializer. The benefits with this is that this is supported in the framework in all platforms. The downside is that some variable types (like DateTime) can't be parsed directly. But in my application this is fairly easy to deal with, and I prefer to add some extra lines of code than a large library.

This is my replacement for Json.net:

public class JsonHelper
{
    public static string Serialize(object obj)
    {
        EnsureHasDataContractAttribute(type);

        System.Runtime.Serialization.Json.DataContractJsonSerializer serializer =
                        new System.Runtime.Serialization.Json.DataContractJsonSerializer(obj.GetType());

        using (MemoryStream stream = new MemoryStream())
        {
            serializer.WriteObject(stream, obj);

            byte[] rawData = stream.ToArray();

            return System.Text.UTF8Encoding.UTF8.GetString(rawData, 0, rawData.Length);
        }
    }

    public static object Deserialize(string text, Type type)
    {
        EnsureHasDataContractAttribute(type);

        System.Runtime.Serialization.Json.DataContractJsonSerializer a =
                        new System.Runtime.Serialization.Json.DataContractJsonSerializer(type);

        using (MemoryStream stream = new MemoryStream(System.Text.UTF8Encoding.UTF8.GetBytes(text)))
        {
            return a.ReadObject(stream);
        }
    }

    public static T DeserializeObject<T>(string text)
    {
        return (T)Deserialize(text, typeof(T));
    }

    private static void EnsureHasDataContractAttribute(Type attributeHolder)
    {
        // I have had problems with classes where [DataContract]/[DataMember] is
        // is missing. This has caused DataContractJsonSerializer to crash
        // randomly. This method make a simple check that the
        // [DataContract] attribute is added to the class. Not perfect
        // but should catch mose problems I hope.

        //String is always safe
        if (attributeHolder == typeof(string))
            return;

        //decimal is always safe
        if (attributeHolder == typeof(decimal))
            return;

        //Primtives is always safe
        if (attributeHolder.GetTypeInfo().IsPrimitive)
            return;

        //Enums is always safe
        if (attributeHolder.GetTypeInfo().IsEnum)
            return;

        //byte[] could cause problems.
        if (attributeHolder == typeof(byte[]))
        {
            System.Diagnostics.Debug.WriteLine("Type byte[] is behaving differently in DataContractJsonSerializer and JSon.Net. You should probably use string and Convert.FromBase64String instead");
            return;                    
        }

        //DateTime/DateTimeOffset could cause problems.
        if (attributeHolder == typeof(DateTime) || attributeHolder == typeof(DateTimeOffset))
        {
            System.Diagnostics.Debug.WriteLine("Type DateTime/DateTimeOffset is behaving differently in DataContractJsonSerializer and JSon.Net. You should probably use string and DateTime/DateTimeOffset.Parse instead");
            return;
        }

        //TimeSpan could cause problems.
        if (attributeHolder == typeof(TimeSpan))
        {
            System.Diagnostics.Debug.WriteLine("Type TimeSpan is behaving differently in DataContractJsonSerializer and JSon.Net. You should probably use string and DateTime/DateTimeOffset.Parse instead");
            return;
        }

        // If this is a collection, check the elemnt type instead.
        Type subType = attributeHolder.GetElementType();

        if( subType != null )
        {
            EnsureHasDataContractAttribute(subType);
            return;
        }

        // Check that DataContractAttribute is added to the type.
        // Note: using System.Reflection; is needed for GetTypeInfo.
        if (attributeHolder.GetTypeInfo().GetCustomAttribute(typeof(System.Runtime.Serialization.DataContractAttribute)) != null)
            return;

        // Oh no! [DataContract] is missing on the type that should de deserialized!
        System.Diagnostics.Debugger.Break();

        throw new Exception("Missing [DataContract] for " + attributeHolder.ToString());
    }
}

I have also written a small class to build the Uri with all parameters:

public class UriQueryBuilder
{
    public UriQueryBuilder()
        : this(null, null)
    {

    }

    public UriQueryBuilder(string baseurl)
        : this(baseurl, null)
    {
    }

    public UriQueryBuilder(string baseurl, string action)
    {
        Parameters = new Dictionary<string, string>();
        BaseUrl = baseurl;
        Action = action;
    }

    public string BaseUrl { get; set; }
    public string Action { get; set; }
    public Dictionary<string, string> Parameters { get; set; }

    public string QueryString
    {
        get
        {
            var array = (from key in Parameters.Keys
                         select string.Format("{0}={1}", System.Uri.EscapeDataString(key), System.Uri.EscapeDataString(Parameters[key])))
                            .ToArray();

            return string.Join("&", array);
        }
    }

    public string FormattedUri
    {
        get
        {
            string formattedString = BaseUrl;

            if (!String.IsNullOrWhiteSpace(Action))
            {
                if (!BaseUrl.EndsWith("/"))
                    formattedString += "/";

                formattedString += Action;
            }

            string query = QueryString;

            if (!String.IsNullOrWhiteSpace(query))
            {
                formattedString += "?";

                formattedString += query;
            }

            return formattedString;
        }
    }
}

Finally I wrote HttpHelper that takes care of the downloading part. Note that there is support of CancelleationTokenSource. I want that the user should be able to stop everything immediately if needed.

public class HttpHelper
{
    public static async Task<string> DownloadStringAsync(UriQueryBuilder builder, CancellationTokenSource cancelHandler = null)
    {   
        var url = new System.Uri(builder.FormattedUri);
        using (var client = new System.Net.Http.HttpClient())
        {
            if (cancelHandler != null)
                cancelHandler.Token.Register(client.CancelPendingRequests);

            return await client.GetStringAsync(url);
        }
    }

    public static async Task<T> DownloadJsonObjectAsync<T>(UriQueryBuilder builder, CancellationTokenSource cancelHandler = null)
    {
        string text = await DownloadStringAsync(builder, cancelHandler);

        return JsonHelper.DeserializeObject<T>(text);
    }
}

Example of usage

First, define a class that will hold the parsed json data:

[DataContract]
public class CheckVersionResult
{
    [DataMember]
    public bool HasUpdate { get; set; }

    [DataMember]
    public string DownloadUrl { get; set; }

    [DataMember]
    public string ReleaseDate { get; set; }

    public DateTime ReleaseDateParsed
    { 
        get
        {
            try
            {
                return DateTime.Parse(ReleaseDate);
            }
            catch(Exception )
            {
                return new DateTime(2000, 1, 1);
            }
        }
    }
}

[DataContract] and [DataMember] is needed for the DataContractJsonSerializer. ReleaseDateParsed is dirty but acceptable in my case.

Finally some code to build up the Uri and download and parse the data:

UriQueryBuilder builder = new UriQueryBuilder("http://localhost:61933/api/ProductVersions", "CheckVersion");
builder.Parameters.Add("product", "myproduct");
builder.Parameters.Add("platform", "wpf");
builder.Parameters.Add("version", "1.2.3.4");

CheckVersionResult data = await HttpHelper.DownloadJsonObjectAsync<CheckVersionResult>(builder, null);

Quite small and elegant I think. The best thing is that all code works in Wpf, Windows Store and Windows Phone apps.

| improve this answer | |
\$\endgroup\$

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.