0
\$\begingroup\$

I'm creating a wrapper library for an API, and I hit a block on how to redesign it. The entire library is static, and I'd like to change it to make it more testable.

The library's goal is to make rest api requests and return C# objects. There is a networking class, which makes the requests. And there is a class for each endpoint which abstracts the parameters for the request and supplies the relative url.

Networking class: build url, create headers, and return results from request

public class APIClient{

private static string url = null;
private static string apiKey = null;

public static void Initialize(string key,string url)
    {
        APIClient.url = url;
        APIClient.apiKey = key;
    }

public static async Task<T> GetAsync<T>(string path, Dictionary<string, string> parameters)
    {
        var client = CreateHttpClientFromApiCredentials();

        //build url with parameters from the dictionary
        var builder = new UriBuilder("", path);
        builder.Port = -1;

        if (parameters != null)
        {
            var query = HttpUtility.ParseQueryString("");
            foreach (var key in parameters.Keys)
                query[key] = parameters[key];

            builder.Query = query.ToString();
        }

        var response = await client.GetAsync(builder.ToString());

        if (!response.IsSuccessStatusCode)
            throw new HttpException((int)response.StatusCode, response.ReasonPhrase);

        //evaluate API specific data
        return await EvaluateResponseContent<T>(response);
    }
}

Sample endpoint class: provide relative url and arguments to networking class

public class APIObject
{
    public static async Task<List<APIObject>> Get()
    {
        return await APIClient.GetAsync<List<APIObject>>("objectendpoint",null);
    }

}

This design allows for very simple use, with calls like APIObject.Get().

With that said, I have a redesign in mind that will maintain the simplicity of the static functions while also allowing for easy testing. The APIClient will be refactored to be an object, and each of the API object classes will be a nested class inside the APIClient.

A brief example (untested):

public class APIClient
{
    public ApiObjectSender ApiObjectEndpoint;

    public APIClient() {
        ApiObjectEndpoint = new ApiObjectSender(this);
    }

    public class ApiObjectSender
    {
        APIClient apiClient;

        public ApiObjectSender(APIClient cl)
        {
            apiClient = cl;
        }

        public List<APIObject> Get()
        {
            return apiClient.Get("apiobjectendpoint");
        }
    }
}

This allows for similar function calls, like clientVar.ApiObjectEndpoint.Get(). However, I can't help but feel like this approach is... janky.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to StackExchange Code Review! Please review How do I ask a good Question? Specifically, it is best to explain what the code does. This is especially true in the title. \$\endgroup\$ – Stephen Rauch Jul 19 '17 at 18:37
  • \$\begingroup\$ @StephenRauch made some changes to be more descriptive \$\endgroup\$ – SharpLizard Jul 19 '17 at 19:06
  • 1
    \$\begingroup\$ A brief example (untested): This would work on Software Engineering but I don't think it's a good question for Code Review. \$\endgroup\$ – t3chb0t Jul 21 '17 at 4:23
  • \$\begingroup\$ It is probably better to leave out that part of the question and see what kind of improvements people come up with here. \$\endgroup\$ – Graipher Jul 21 '17 at 7:32
  • \$\begingroup\$ How should I phrase a question if I'm asking about a specific redesign? \$\endgroup\$ – SharpLizard Jul 21 '17 at 15:48
2
\$\begingroup\$
public static void Initialize(string key,string url)
    {
        APIClient.url = url;
        APIClient.apiKey = key;
    }

This is a dead give away that this class shouldn't be a bunch of static methods. I have to call this initialize method before using it or it'll blow up with null reference exceptions.

Worse, let's say I parallelize the main process and have multiple processes using this class. What happens when one of processes changes the uri? That's right, now all the processes are hitting that other uri. You'll get results, they'll just be the wrong one.

Thankfully, it's a simple fix. Just turn your Initialize method into a constructor and remove all instances of the static keyword.

public class APIClient
{
    private readonly string url;
    private readonly string apiKey;

    public APIClient(string key,string url)
    {
        this.url = url;
        this.apiKey = key;
    }
\$\endgroup\$
  • \$\begingroup\$ +1. And if you need to have only one instance of the APIClient you can use singleton pattern. \$\endgroup\$ – Maxim Jul 20 '17 at 3:45
  • \$\begingroup\$ @Maxim I don't see anything that suggests a singleton would be appropriate here. \$\endgroup\$ – RubberDuck Jul 20 '17 at 15:52
  • \$\begingroup\$ Yep! It seems like being object oriented is the way to go, but what do you think of the nested classes? \$\endgroup\$ – SharpLizard Jul 20 '17 at 18:27
  • \$\begingroup\$ I personally don't see what benefit they'd give you @SharpLizard. What do you hope to achieve with the inner class? \$\endgroup\$ – RubberDuck Jul 20 '17 at 19:19
  • \$\begingroup\$ I want it to be really simple to use and I like the simplicity of a call like MyObject.Get(), so nested classes would allow for that. I wanted something like this answer only more abstract stackoverflow.com/a/40538653/7691601 \$\endgroup\$ – SharpLizard Jul 20 '17 at 19:38

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.