2
\$\begingroup\$

I've written a variety of libraries that query and work with data from APIs. While the classes and objects being passed around vary from project to project, I'm generating GET requests in the same manner in each project.

I have a basic method to generate a GET request:

private string GenerateWebRequest(string conn)
{
    try
    {
        using (var webClient = new WebClient())
        {
            return webClient.DownloadString(conn);
        }
    }
    catch (Exception e)
    {
        Console.WriteLine("There was a problem generating the web request to the API server.");
        Console.WriteLine(e.StackTrace);
        return string.Empty;
    }
}

To make life easier and avoid code repetition, I create a Config object in the primary constructor for each library, which looks something like:

public class Config
{
    public Config()
    {
        localhostNearest = string.Empty;
        localhostRoute = string.Empty;
        localhostTrip = string.Empty;
        processLocation = string.Empty;
        workingDirectory = string.Empty;
        datasource = string.Empty;
    }

    public string localhostNearest { get; set; }

    public string localhostRoute { get; set; }

    public string localhostTrip { get; set; }

    public string processLocation { get; set; }

    public string workingDirectory { get; set; }

    public string datasource { get; set; }
}

And the instantiation of the Config object:

Config cfg = new Config
{
    localhostNearest = "http://localhost:5000/nearest/v1/driving/",
    localhostRoute = "http://localhost:5000/route/v1/driving/",
    localhostTrip = "http://localhost:5000/trip/v1/driving/",
    processLocation = installationFolder + "\\osrm-routed.exe",
    workingDirectory = workingFolder + @"data\"
};

The string conn that gets passed as a parameter to the GenerateWebRequest() method is generated in individual methods, dependent on what should be queried and then deserialized (each response is a JSON string). A sample conn generation would be:

string response = GenerateWebRequest($"{_config.localhostNearest}{lat},{lon}?apiKey={key}");

which would come out as:

http://localhost:5000/route/v1/nearest/-3.3431053161621094,52.50671376947057?apiKey=XXXXX

The connection string is typically made up of three parts:

  1. Base URL. Such as http://localhost:5000/api/
  2. User query, which part of the API is being queried.
  3. API key appended to the end of the URL.

I feel like there is a cleaner way of doing this in each project, and some methods end up being overly verbose due to the creation of the URL.

Are there any recommendations that could be made to improve this?

\$\endgroup\$
3
  • \$\begingroup\$ @Vogel612 I use this on a daily basis. Presumably you're referencing the WebClient, in which case it's recreated for each request. I could, for instance, instantiate it elsewhere and open/close it, but the using statement does that for me. \$\endgroup\$ Feb 7, 2017 at 11:07
  • \$\begingroup\$ the method name thoroughly threw me off there ... ~blush \$\endgroup\$
    – Vogel612
    Feb 7, 2017 at 11:12
  • \$\begingroup\$ @Vogel612 Ha, no problem. BCdotWEB pointed out it's poorly named so feel free to blame me, not yourself. \$\endgroup\$ Feb 7, 2017 at 11:13

1 Answer 1

3
\$\begingroup\$

Why are there Console.WriteLine calls? Use a logging library instead of binding this to a specific usage.


Also, returning string.Empty; instead of throwing an exception seems a bit of a bad idea. You wouldn't know there's an issue unless you'd look at your logs.


Properties should be PascalCase: localhostNearest, etc.


Why bother with this in the constructor?

localhostNearest = string.Empty;

Why can't it be null?


Do not concatenate when working with paths:

processLocation = installationFolder + "\\osrm-routed.exe",
workingDirectory = workingFolder + @"data\"

Instead, use Path.Combine().


GenerateWebRequest() does more than what its name implies.


What is conn? Use descriptive names for variables, parameters, properties, etc.


There is a lot of repetition here:

localhostNearest = "http://localhost:5000/nearest/v1/driving/",
localhostRoute = "http://localhost:5000/route/v1/driving/",
localhostTrip = "http://localhost:5000/trip/v1/driving/",

If all URLs follow that pattern, why not work with a "template" -- e.g. $"http://localhost:5000/{type}/{version}/driving/" -- and use string interpolation or String.Format()?

Also, why are these hardcoded in your code? Shouldn't these URLs come from a .config file?

\$\endgroup\$
0

Your Answer

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

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