mjhjmtu

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)
 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

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#.

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.