search

thephpleague / geotools

Geo-related tools PHP 7.3+ library built atop Geocoder and React libraries
MIT License
1.37k stars 122 forks source link

Geohash Integer methods #56

Open mablae opened 9 years ago

mablae commented 9 years ago

It would be quite nice to allow encoding into geohash integers.

For example https://github.com/sunng87/node-geohash offers the encode_int = function (latitude, longitude, bitDepth) function.

Since the current Geohash implemenation manages state inside the class and is not like a static converter class, the way to go would be a new GehashInteger class, right?

Or does it make sense to refactor the the current Geohash class to support additional converting methods for geohash integers?

EDIT: Better description

cheers mablae

mablae commented 9 years ago

@toin0u Any opinion on this?

toin0u commented 9 years ago

Hi @mablae! Sorry to reply so late. I think I'll go to refactor the current Geohash class :) Are you willing to help with this?

mablae commented 9 years ago

Sure, I am. I already looked at the current implementation and noticed that Geohash maintains its state internally as geohash string. How would you refactor the class in a sane way?

EDIT: Another thing is the internal handling of long integer values in PHP when printing them out. When not formatted with number_format or sprintf they get print out as 2.4681012141618E+35 format.

When putting geohashes into Redis, I had to work around this issue by using number format. Should it be handled by the GeoInteger methods, meaning it would return an string?

Or is returning real integer better and let the users decide, depending their usecase?

http://stackoverflow.com/questions/9630793/how-to-display-long-integers-on-browser-in-php

mablae commented 9 years ago

For a personal project I converted the algorithm used by https://github.com/sunng87/node-geohash to PHP.

Since there is also #3 (Geohash36) maybe it would be the better way to have a GeoHashInterface and then multiple self containing implementations on that?

This would be easier to extend and no BC breaks at all.. Let's not couple all different implementations into the GeoHash class.

Converting between different geohashes would also be easily possible then...

What do you think?

toin0u commented 9 years ago

@mablae I agree with you :) The interface is a good idea. I'm :+1: for that.

Thanks for pointing out the issue with the long integer.. I think you're right to return the string make sense in this case.

Thank you very much for commenting and come with inputs, I appreciate it :)

mablae commented 9 years ago

Okay, fine. :+1:

I will prepare a PR on the weekend to be discussed further then.

toin0u commented 9 years ago

That's sound nice! I'm looking forward to it :)

toin0u commented 9 years ago

@mablae any news about this? :)

TeaDove commented 2 years ago

Any news about integer version of Geohash?