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\$
5
  • 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\$ Jul 19, 2017 at 18:37
  • \$\begingroup\$ @StephenRauch made some changes to be more descriptive \$\endgroup\$ Jul 19, 2017 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, 2017 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, 2017 at 7:32
  • \$\begingroup\$ How should I phrase a question if I'm asking about a specific redesign? \$\endgroup\$ Jul 21, 2017 at 15:48

1 Answer 1

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\$
5
  • \$\begingroup\$ +1. And if you need to have only one instance of the APIClient you can use singleton pattern. \$\endgroup\$
    – Maxim
    Jul 20, 2017 at 3:45
  • \$\begingroup\$ @Maxim I don't see anything that suggests a singleton would be appropriate here. \$\endgroup\$
    – RubberDuck
    Jul 20, 2017 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\$ Jul 20, 2017 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, 2017 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\$ Jul 20, 2017 at 19:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.