4
\$\begingroup\$

I have create one class and interface for finding SuburbName based on Location or LatLong coordinates.

My interface

public interface IGeoCodeService
{
    string GetSuburbName(double latitude, double longitude);
    string GetSuburbName(string location);
    string GetGoogleStaticMapImageUrl(string location);
}

My Class

public class GeoCodeService : IGeoCodeService
{
    private static readonly string GoogleApiKey = "*****MyAPI KEY*****";
    private static readonly string GoogleStaticMapUrl = "http://maps.googleapis.com/maps/api/staticmap";

    public string GetSuburbName(string location)
    {
        if (!string.IsNullOrEmpty(location))
        {
            using (var clientService = new HttpClientService<GeoCodeResponse>("https://maps.googleapis.com"))
            {
                var param = new Dictionary<string, string>();
                param.Add("address", location);
                param.Add("components", "country:AU");
                param.Add("result_type", "locality");
                param.Add("key", GoogleApiKey);
                var apiResult = clientService.GetAPI("maps/api/geocode/json", param);
                return FindSuburbName(apiResult);
            }
        }
        return string.Empty;
    }

    public string GetSuburbName(double latitude, double longitude)
    {
        using (var clientService = new HttpClientService<GeoCodeResponse>("https://maps.googleapis.com"))
        {
            var param = new Dictionary<string, string>();
            param.Add("latlng", $"{latitude},{longitude}");
            param.Add("result_type", "locality");
            param.Add("key", GoogleApiKey);

            var apiResult = clientService.GetAPI("maps/api/geocode/json", param);
            return FindSuburbName(apiResult);
        }
    }

    private string FindSuburbName(GeoCodeResponse result)
    {
        var response = string.Empty;
        if (result != null || result.results != null && result.results.Count > 0)
        {
            var addressObj = result.results.FirstOrDefault();
            if (addressObj != null)
            {
                var component = addressObj.AddressComponents.Where(x => x.Types.Contains("locality")).FirstOrDefault();
                if (component != null)
                {
                    return component.LongName;
                }
            }
        }
        return response;
    }

    public string GetGoogleStaticMapImageUrl(string location)
    {
        return $"{GoogleStaticMapUrl}?size=350x200&markers={location}";
    }
}

And lastly the GeoCodeResponse class to store response from the google api call.

public class GeoCodeResponse
{
    [JsonProperty("results")]
    public List<Results> results { get; set; }
    [JsonProperty("status")]
    public string status { get; set; }
}

public class Results
{
    public Results()
    {
        AddressComponents = new List<AddressComponent>();
    }
    [JsonProperty("address_components")]
    public List<AddressComponent> AddressComponents { get; set; }
}
public class AddressComponent
{
    [JsonProperty("long_name")]
    public string LongName { get; set; }
    [JsonProperty("short_name")]
    public string ShortName { get; set; }
    [JsonProperty("types")]
    public string[] Types { get; set; }
}

Can you please suggest me if what I have implemented is right or require any change. Your help will improve my code quality.

The HttpClientService class is used for call api

\$\endgroup\$
  • \$\begingroup\$ Please include the HttpClientService class even if it's already been reviewed. You never know what someone else can find :) \$\endgroup\$ – IEatBagels Aug 23 '18 at 14:42
3
\$\begingroup\$

There are lots of nice things in your code for a beginner, congratulations.

Configuration file

Right now, you obfuscated your API key so that we don't have it, that's logical.

But it shouldn't be hardcoded in your code either. This is a bad practice for multiple reasons

  1. What if I want to use your code? I'd be using you API key, this is danngerous. I should be able to set mine without having to recompile
  2. Your API key would be stored in your source control (if you have one) and could be accessible to anyone decompiling your code. If there are costs related to Maps API, things could go wrong very quickly.

An API key should always be in some sort of configuration file. Either an app.config, web.config or some other external configuration manager.

Code repetition

Between the two overrides of GetSuburbName, there are parts of code that are duplicated. You did a good job of extracting FindSuburbName(GeoCodeResponse result) as a private method though.

I would go a little further. The only difference between the two methods is the parameters list so, as an example, I'd change :

if (!string.IsNullOrEmpty(location))
{
    using (var clientService = new HttpClientService<GeoCodeResponse>("https://maps.googleapis.com"))
    {
        var param = new Dictionary<string, string>();
        param.Add("address", location);
        param.Add("components", "country:AU");
        param.Add("result_type", "locality");
        param.Add("key", GoogleApiKey);
        var apiResult = clientService.GetAPI("maps/api/geocode/json", param);
        return FindSuburbName(apiResult);
    }
}
return string.Empty;

To :

if (!string.IsNullOrEmpty(location))
{
    var param = new Dictionary<string, string>();
    param.Add("address", location);
    param.Add("components", "country:AU");
    param.Add("result_type", "locality");
    param.Add("key", GoogleApiKey);

    return FindSuburbName(param);
}
return string.Empty;

Then, you could have your API call in the common method.

C# tips

You can use the Dictionary Initializer if you have a recent version of C#.

As an example :

var param = new Dictionary<string, string>();
param.Add("address", location);

becomes :

var param = new Dictionary<string, string>
{
    {"address", location},
    //etc
}

Regarding this line :

var component = addressObj.AddressComponents.Where(x => x.Types.Contains("locality")).FirstOrDefault();

There is an override for FirstOrDefaut that takes a Predicate (a condition), which means your code could be :

var component = addressObj.AddressComponents.FirstOrDefault(x => x.Types.Contains("locality"));

@FabienH. @FabianH. already pointed out the other points I wanted to cover regarding C#.

\$\endgroup\$
  • \$\begingroup\$ Nice Answer and thanks for mentioning my own. I even forgive you writing my name wrong :-) \$\endgroup\$ – Fabian H. Aug 23 '18 at 14:49
  • \$\begingroup\$ @FabianH. Oops my bad, fixed :p \$\endgroup\$ – IEatBagels Aug 23 '18 at 14:58
2
\$\begingroup\$

I would reccomend using string.IsNullOrWhitespace(..) instead of string.IsNullOrEmpty(..), but this depends on how you call your method and what input is possible. Also I prefer returning early, instead of having all my actual code in an if-block:

public string GetSuburbName(string location)
{
    if (string.IsNullOrWhitespace(location))
    {
        return null; // or String.Empty
    }
    // your stuff
}

Regarding your FindSuburbName-Method: There are a LOT of redundant checks, that could be a lot more simplified. For example you check if Count > 0, then take the FirstOrDefault(), where First() would be enough, because you know there is at leat one, then you check if that result is not null. How could it be null?

One line version (for readability it would probably be best to do this in more lines, but this is just to show what is possible):

private string FindSuburbName(GeoCodeResponse result)
{
    return result?.results?.FirstOrDefault()?.AddressComponents.FirstOrDefault(comp => comp.Types.Contains("locality"))?.LongName;
}

If you want to return string.Empty instead of null, just do ?? string.Empty at the end of that oneliner.

You could omit the constructor of you Results-class, if you do

public List<AddressComponent> AddressComponents { get; set; } = new List<AddressComponent>();

instead.

\$\endgroup\$

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.