WorkGroup Data Service with JSON / Web based API

Question

I'm wondering if this code can be made more clear and fluent:

using System;
using System.Threading.Tasks;
using Windows.Data.Json;
using System.Collections.ObjectModel;
using System.Net.Http;
using System.Collections.Generic;
using Newtonsoft.Json;
using System.Linq;
using eTest.Space.Log;
using eTest.Space.Interface;
using eTest.Objects;

namespace eTest.Space.Services
{

    public class WorkGroupDataService : IWorkgroupDataService
    {


        public string WebApiUri
        {
            get { return Constants.ServerAddress; }
        }

        private ObservableCollection<IWorkgroup> Groups;// = new ObservableCollection<IWorkgroup>();

        public async Task<ObservableCollection<IWorkgroup>> GetAllWorkGroups()
        {
            Groups = new ObservableCollection<IWorkgroup>();
            //if (Groups.Count == 0)
            {
                Uri dataUri = new Uri(WebApiUri + "api/workgroup/");

                using (var client = new Windows.Web.Http.HttpClient())
                {
                    var response = await client.GetAsync(dataUri).AsTask(); ;

                    string jsonText = response.Content.ReadAsStringAsync().GetResults();
                    JsonArray jsonArray = JsonArray.Parse(jsonText);

                    foreach (JsonValue groupValue in jsonArray)
                    {
                        JsonObject groupObject = groupValue.GetObject();
                        IWorkgroup group = new Workgroup();
                        group.GroupState = (int)groupObject["GroupState"].GetNumber();
                        group.WorkgroupName = groupObject["WorkgroupName"].GetString();
                        group.WorkgroupSecretary = groupObject["WorkgroupProba"].GetString();
                        group.ObjectID = groupObject["ObjectID"].GetString();
                        Uri dataUri1 = new Uri(WebApiUri + "api/etest/" + group.ObjectID);
                        using (var client1 = new Windows.Web.Http.HttpClient())
                        {
                            var response1 = await client.GetAsync(dataUri1).AsTask();
                            if (response1.StatusCode == Windows.Web.Http.HttpStatusCode.Ok)
                            {
                                string jsonText1 = response1.Content.ReadAsStringAsync().GetResults();
                                List<TestSession> Test = JsonConvert.DeserializeObject<List<TestSession>>(jsonText1);
                                group.Test = Test;
                            }
                        }
                        if (group.Test.Count > 0)
                        {
                            Groups.Add(group);
                        }

                    }
                }
            }
            Groups = new ObservableCollection<IWorkgroup>(Groups.OrderByDescending((x => x.Test.Count)));
            return Groups;
        }
    }
}

Answer 1

Naming Conventions

Private instance fields should be lowercase:

private ObservableCollection<IWorkgroup> groups;

Local variables should also be lowercase:

List<TestSession> test = JsonConvert.DeserializeObject<List<TestSession>>(jsonText1);

Also, where you are doing the same thing with the clients and URIs, you are adding numbers to the second of those variables, e.g. client1. Try to think of something more descriptive, those two clients are serving two different purposes so try something like purposeClient where purpose is replaced by a word for it's purpose. At this point, extracting the functionality out into separate methods is probably a good idea.

---

Improvements

If you're going to use an async method, then you should await it:

var jsonText = await response.Content.ReadAsStringAsync().GetResults();

---

Structure

Similarly, given that this variable doesn't look like it needs to be shared amongst methods (unless you haven't included the rest of your class), I'm not really sure why you've made it an instance field and can quite comfortably be declared inside your GetAllWorkGroups() method.

You seem to assign a value to Groups twice in that method, you don't need to do the second one:

groups = groups.OrderByDescending(x => x.Test.Count);

You also seems to double wrap the OrderByDescending clause in brackets which was unnecessary

You seem to be switching between using var and, for example string. A lot of people do switch, using var where the type inferred is obvious and using the type when it is not immediately obvious to make it more readable (this is what I do), but a lot of people will exclusively use var. In terms of style, I'd definitely go one way or the other because currently your switching just seems messy.

I find that object initializers make the code more readable so here:

IWorkgroup group = new Workgroup();
group.GroupState = (int)groupObject["GroupState"].GetNumber();
group.WorkgroupName = groupObject["WorkgroupName"].GetString();
group.WorkgroupSecretary = groupObject["WorkgroupProba"].GetString();
group.ObjectID = groupObject["ObjectID"].GetString();

Can be:

IWorkgroup group = new Workgroup 
{
    GroupState = (int)groupObject["GroupState"].GetNumber(),
    WorkgroupName = groupObject["WorkgroupName"].GetString(),
    WorkgroupSecretary = groupObject["WorkgroupProba"].GetString(),
    ObjectID = groupObject["ObjectID"].GetString()
};

After this point, you then start a new using, but from the code it seems that you are in fact finished using your previous client so you can move it around to be more like this:

public async Task<ObservableCollection<IWorkgroup>> GetAllWorkGroups()
{
    var groups = new ObservableCollection<IWorkgroup>();
    JsonArray jsonArray;

    using (var client = new Windows.Web.Http.HttpClient())
    {
        var dataUri = new Uri(WebApiUri + "api/workgroup/");
        var response = await client.GetAsync(dataUri).AsTask();

        var jsonText = await response.Content.ReadAsStringAsync().GetResults();
        jsonArray = JsonArray.Parse(jsonText);
    }

    foreach (var groupValue in jsonArray)
    {
        var groupObject = groupValue.GetObject();
        IWorkgroup group = new Workgroup
        {
            GroupState = (int)groupObject["GroupState"].GetNumber(),
            WorkgroupName = groupObject["WorkgroupName"].GetString(),
            WorkgroupSecretary = groupObject["WorkgroupProba"].GetString(),
            ObjectID = groupObject["ObjectID"].GetString()
        };

        var dataUri = new Uri(WebApiUri + "api/etest/" + group.ObjectID);
        using (var client = new Windows.Web.Http.HttpClient())
        {
            var response = await client.GetAsync(dataUri).AsTask();
            if (response.StatusCode == Windows.Web.Http.HttpStatusCode.Ok)
            {
                var jsonText = await response.Content.ReadAsStringAsync().GetResults();
                var test = JsonConvert.DeserializeObject<List<TestSession>>(jsonText);
                group.Test = test;
            }
        }

        if (group.Test.Count > 0)
        {
            groups.Add(group);
        }
    }

    return groups.OrderByDescending(x => x.Test.Count);
}

Answer 2

Based on danrhul's good answer

If group.Test.Count == 0 all the done work with IWorkgroup group will be skipped. So you better change the execution flow.

foreach (var groupValue in jsonArray)
{
    var groupObject = groupValue.GetObject();
    String objectId = groupObject["ObjectID"].GetString();

    var test = GetTestSessions(objectId);
    if (test.Count == 0) { continue; }

    IWorkgroup group = new Workgroup
    {
        GroupState = (int)groupObject["GroupState"].GetNumber(),
        WorkgroupName = groupObject["WorkgroupName"].GetString(),
        WorkgroupSecretary = groupObject["WorkgroupProba"].GetString(),
        ObjectID = objectId,
        Test = test
    };

    groups.Add(group);
}

With calling the extracted overloaded methods

private List<TestSession> GetTestSessions(String objectId)
{
    return GetTestSessions(new Uri(WebApiUri + "api/workgroup/" + objectId));
}

private List<TestSession> GetTestSessions(Uri dataUri)
{
    using (var client = new Windows.Web.Http.HttpClient())
    {
        var response = await client.GetAsync(dataUri).AsTask();
        if (response.StatusCode == Windows.Web.Http.HttpStatusCode.Ok)
        {
            var jsonText = await response.Content.ReadAsStringAsync().GetResults();
            return JsonConvert.DeserializeObject<List<TestSession>>(jsonText);
        }

        return new List<TestSession>();
    }

}

Answer 3

Absolutely! There's loads of cool shortcuts in C#. I'll apply them to the code with comments on what they are:

namespace eTest.Space.Services
{
    public class WorkGroupDataService : IWorkgroupDataService
    {
        public string WebApiUri
        {
            get { return Constants.ServerAddress; }
        }

        public async Task<ObservableCollection<IWorkgroup>> GetAllWorkGroups()
        {
            // WARNING: You have a private member that has the potential to be 
            // shared across threads if GetAllWorkGroups is used asynchronously!
            // Swapped for a local variable instead.
            var groups = new ObservableCollection<IWorkgroup>();
            //if (groups.Count == 0)
            {
                // Love the var keyword, means typing the Type once instead of twice
                var dataUri = new Uri(WebApiUri + "api/workgroup/");

                string jsonText;
                using (var client = new Windows.Web.Http.HttpClient())
                {
                    // AsTask redundant, already a task
                    // ConfigureAwait to avoid unnecessary context switch (you'll await again outside this and can choose to keep context or not depending on if you do work in the UI
                    var response = await client.GetAsync(dataUri).ConfigureAwait(false);

                    // we're using async, await when getting results to trigger thread release while waiting
                    jsonText = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
                // We close the using as early as possible to release the object as early as possible
// Also, the less nesting of scopes, the easier your code is to read
                }

                foreach (var groupValue in jsonArray)
                {
                    var groupObject = groupValue.GetObject();

                    // Moved body of foreach to its own method
                    var group = GetGroupFromGroupObject(groupObject);

                    if (group.Test.Count > 0)
                    {
                        groups.Add(group);
                    }
                }
            }

            // Removed the private member assign, returned the local variable
            return groups;
        }

        private Group GetGroupFromGroupObject(GroupObject groupObject)
        {
            // Object initializer for cleaner syntax
            var group = new Workgroup
            {
                GroupState = (int)groupObject["GroupState"].GetNumber();
                WorkgroupName = groupObject["WorkgroupName"].GetString();
                WorkgroupSecretary = groupObject["WorkgroupProba"].GetString();
                ObjectID = groupObject["ObjectID"].GetString();
            }

            // string.format reads better
            // dropping args to their own lines can improve readability
            var uriString = string.Format(
                                "{0}api/etest/{1}", 
                                WebApiUri, 
                                group.ObjectID);

            // Avoiding nested calls can make for easier reading
            var dataUri1 = new Uri(uriString);

            using (var client1 = new Windows.Web.Http.HttpClient())
            {
                var response1 = await client.GetAsync(dataUri1).ConfigureAwait(false);
                if (response1.StatusCode == Windows.Web.Http.HttpStatusCode.Ok)
                {
                    var jsonText1 = await response1.Content.ReadAsStringAsync().ConfigureAwait(false);

                    // No need to create a separate var just to assign
                    // Type T will be inferred from the object, no need to write it
                    group.Test = JsonConvert.DeserializeObject(jsonText1);
                }
            }

            return group;
        }
    }
}