Checking for new software updates to the client

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(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?

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 wpf 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.

`

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(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, and have an Async` suffix in its name, to follow convention.

StackExchange Code Review Q#57443, answer score: 6