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

Add functionality for calculating Aerial Distances #47

Closed
wants to merge 25 commits into from
Closed

Add functionality for calculating Aerial Distances #47

wants to merge 25 commits into from

Conversation

i-sanyam
Copy link
Contributor

@i-sanyam i-sanyam commented Nov 6, 2022

I frequently find myself finding distance between two points on a random website. Will love if this is handled by dns.toys

TODO:

  1. I have yet to add tests.
  2. Really need the delimiter to be , because that's how points are available to me eg. 12.670,34.870
  3. Would appreciate if you take out time for a code review

PS. My first Golang PR

Closes #48

README.md Outdated Show resolved Hide resolved
@knadh
Copy link
Owner

knadh commented Nov 8, 2022

Thanks for the PR @i-sanyam. Can you please clean up the debug statements and send the final version?

@sanyam-aggarwal-shipsy
Copy link

Thanks for the PR @i-sanyam. Can you please clean up the debug statements and send the final version?

Sure I will but I have mentioned you in the PR as I am having issue with deciding the delimiter. it isn't working either for - or ,.

@i-sanyam
Copy link
Contributor Author

i-sanyam commented Nov 9, 2022

@i-sanyam
Copy link
Contributor Author

i-sanyam commented Nov 11, 2022

I have allowed , in the initial query parsing and have tested on the local server by passing all the example queries and it is not breaking. Removed debug statements as well

TODOs done

  1. Added test cases.
  2. Lat Lng Pair Delimiter is ,

@i-sanyam i-sanyam marked this pull request as ready for review November 11, 2022 18:09
@knadh
Copy link
Owner

knadh commented Dec 6, 2022

Thanks. Will review and merge this week.

}

// calculates aerial distance in KMs
func CalculateAerialDistance(l1, l2 Location) (float64, error) {
Copy link

Choose a reason for hiding this comment

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

can we add reciever here as well?

errString += strconv.FormatFloat(l.Lng, 'f', -1, 64) + " lng out of bounds"
}

if (errString != "") {
Copy link

Choose a reason for hiding this comment

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

nit: to be consistent with the code we can remove parantheses for if statements

@knadh
Copy link
Owner

knadh commented Dec 11, 2022

@i-sanyam. I've made a few minor cosmetic changes to the commit. Since you send the PR from your master branch, I was unable to amend it, so I've pushed it to a new branch here: https://github.com/knadh/dns.toys/blob/aerial/internal/services/aerial/aerial.go

Please take a look at the changes and if they look fine, I'll merge them.

@i-sanyam
Copy link
Contributor Author

Thanks @knadh for the edits! I have gone through them. They look fine to me. And before merging them, due to the simplification in error handling, the test cases' error strings need an update. I have added them in a new PR to aerial branch itself here:
https://github.com/knadh/dns.toys/pull/52/files

TL;DR the simplified error handling does not validate both the locations' coordinates at once but I think that is fine.

eg in the case of lat long pair -
L1 [-91.00, 191.00]
L2 [91.00, -191.00]

the user first gets an error that L1 is invalid, the user then fixes L1 and attempts again
L1 [11.00, 01.00]
L2 [91.00, -191.00]
this time user again gets an error that L2 is invalid, which may have been avoided by the user if the error is sent at once for both locations.

@i-sanyam
Copy link
Contributor Author

Merged in #53

@i-sanyam i-sanyam closed this Dec 24, 2022
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.

Add Aerial Distance Calculation
4 participants