Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature Us Census Bureau #118

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

DR9885
Copy link
Contributor

@DR9885 DR9885 commented Dec 22, 2018

No description provided.

@DR9885 DR9885 requested a review from chadly January 28, 2020 00:32
Copy link
Owner

@chadly chadly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had a chance to dig deep into this yet. But could you also update the README documentation to include this new geocoder, how it works, how to set it up. and any caveats?

@DR9885
Copy link
Contributor Author

DR9885 commented Feb 29, 2020

I haven't had a chance to dig deep into this yet. But could you also update the README documentation to include this new geocoder, how it works, how to set it up. and any caveats?

sure i'll get back to you when I do.

@DR9885
Copy link
Contributor Author

DR9885 commented Feb 29, 2020

@chadly done, added to readme

@DR9885 DR9885 requested a review from chadly March 5, 2020 20:37
@DR9885
Copy link
Contributor Author

DR9885 commented Mar 5, 2020

Also note, it doesnt support reverse geocoding & is US only... I added that to readme

@potatman
Copy link

potatman commented Apr 4, 2020

@chadly Any chance this could get merged? My company is using @DR9885's fork for our projects, but it would be nice to get back onto the main repo so we could pull from nuget rather than building the lib ourselves.

Copy link

@seanat280 seanat280 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My team has been using this for a substantial period of time at this point with no issues. Can this get merged?

@DR9885
Copy link
Contributor Author

DR9885 commented Mar 29, 2021

@RodneyRichardson can you take a look?

Copy link
Contributor

@RodneyRichardson RodneyRichardson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not be able to test, but generally it looks like it might work, but there are a few improvements that could be made (such as not swallowing errors).

I don't have authority to approve merge requests.

private readonly string _format;
private readonly HttpClient _client;

public UsCensusBureauGeocoder(int benchmark = 4, string format = "json")
Copy link
Contributor

@RodneyRichardson RodneyRichardson Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the magic number "4" (benchmark)? Can this be made into a (string) constant?

private readonly string _format;
private readonly HttpClient _client;

public UsCensusBureauGeocoder(int benchmark = 4, string format = "json")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the format parameter - the code only supports JSON, so hardcode it.

@@ -0,0 +1,98 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting in this file could be improved - Visual Studio autoformat should be sufficient.

/// </summary>
public class UsCensusBureauGeocoder : IGeocoder
{
private readonly int _benchmark;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API defines benchmark as a string, and the example use, for example, "Public_AR_Census2020". Should this be a string? It might be worth referencing https://geocoding.geo.census.gov/geocoder/benchmarks for the list of available benchmarks.


var errors = json[UsCensusBureauConstants.ErrorsKey];
if (errors != null)
return new UsCensusBureauAddress[] {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't swallow errors. Create and throw a UsCensusBureauGeocodingException (wow, that's a long name - how about Ucb?) to wrap and expose the error.

/// - https://geocoding.geo.census.gov/
/// - https://geocoding.geo.census.gov/geocoder/Geocoding_Services_API.pdf
/// </summary>
public class UsCensusBureauGeocoder : IGeocoder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding an IBatchGeocoder, as this looks to be supported by the API.

namespace Geocoding.Tests
{
[Collection("Settings")]
public class UsCensusBureauTest : GeocoderTest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests for other "benchmark" values.

using Geocoding.UsCensusBureau;
using Xunit;

namespace Geocoding.Tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't skip tests. Assert that they behave as expected (such as throwing NotSupportedException, or returning/throwing an appropriate error).

sb.Append("street=").Append(WebUtility.UrlEncode(street))
.Append("&city=").Append(WebUtility.UrlEncode(city))
.Append("&state=").Append(WebUtility.UrlEncode(state))
.Append("&zip=").Append(WebUtility.UrlEncode(postalCode))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work if any parameters are omitted? Perhaps add a test for partial address.

@chadly
Copy link
Owner

chadly commented May 20, 2021

I don't have authority to approve merge requests.

I'm obviously busy and neglecting things here. Do you want write access to the repo @RodneyRichardson. I would appreciate the help for this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants