Converting coordinates need design improvement

[lsolesen]

I am working on refactoring a class to convert coordinates. http://svn.intraface.dk/intrafacepub...aphicPoint.php.

The previous author relied heavily on setting class variables directly and only a few methods actually returns anything. A usage example is here: http://svn.intraface.dk/intrafacepub...es/example.php. Any idea as howto improve the api.

I have tests in place for the current methods, so it should not be to hard to refactor without breaking anything. However, I have at couple of problems as how to structure the whole thing.

- Does all calculations belong in the same class?
- There is not much error checking. Should I introduce some exceptions? (pretty new to this concept, so an example would be nice)
- How would you like to use a class like this?
- Shouldn't I have more methods return something instead of the current setting of variables?

Current minor improvements planned to api

- I want to improve the getter names, so they are more informative

I would be grateful just for some pointers, and I will rush to work

[Jasper Bekkers]

- Does all calculations belong in the same class?

At the least give each coordinate type a different class with a common interface. You might want to look at PEAR:ate to see how they handle different calendars. And you might also want to have a standard coordinate system (lat/long?) and convert every other system to that so calculations can be handled uniformly.

- There is not much error checking. Should I introduce some exceptions? (pretty new to this concept, so an example would be nice)

Depends on how critical the state of the class can be when something messes up. But my guess is that providing a means to validate the data in the class would suffice.

- How would you like to use a class like this?

- With knowing the least math as possible.
- Letting me do common things easily (create some use cases).

- Shouldn't I have more methods return something instead of the current setting of variables?

In this case that would make sense, the calculations are hardly worthy of being object state.

[sweatje]

I once wrote a point class which knew how to calculate the distance to another point:

class Point {
    var $x;
    var $y;
    function Point($x, $y) {
        $this->x = $x;
        $this->y = $y;
    }
    function dist($p) {
        return sqrt(pow($this->x - $p->x,2)+pow($this->y - $p->y,2));
    }
    function show() {
        return '('.$this->x.','.$this->y.')';
    }
}

class PointTestCase extends UnitTestCase {
    function TestPointDistance() {
        $p1 = new Point(1,1);
        $p2 = new Point(2,2);
        $p3 = new Point(1,3);
        
        $this->assertEqual(sqrt(2), $p1->dist($p2));
        $this->assertEqual(2, $p1->dist($p3));
    }
} 

[lsolesen]

Do I understand you correctly, if you mean that I have to do the following:

interface Coordinate {}

class LatitudeLongitude {
    function convertToUTM();
    function convertToLambert();
}
class Lambert() {
    function convertToUTM();
    function convertToLatitudeLongitude();
}
class UTM {
    function convertToLatitudeLongitude();
    function convertToLambert();
} 

[leblanc]

Any idea as howto improve the api.

deja vu

aggregation just like the form validation earlier .. same pattern exist here.

create the render which knows exactly the view it needs to generate.
Pass it into display into your object
$RenderVisitor->dateformat('yyyy/mm/dd');

$myHome->display($RenderVisitor);
//instantiate the object with an address, with exact coordinates

[lsolesen]

> create the render which knows exactly the view it needs to generate.

Where should the actual conversions take place?

By the way, I restructured the class, so it is now more readable, and the methods are more grouped together. It seems objects are trying to get out, if Mr. Martin Fowler's advice are sound

[leblanc]

>
Where should the actual conversions take place?

I kinda knew that was comming next.

the visitor should be getting access to data through $obj->getSomeData().
conversions need to take place inside the data object... since conversions might need other properties inside the object which the visitor shouldn't have access to.

real example of a project i worked on

inside my data object

getMPH() //miles per hour, 
{
  //conversion; use frequency, gear ratio, rear axle ratio
}

getRPM()
{
  //conversion;
}

--------------

another
getDate()
{

} 

inside visitor i simply use $obj->getMPH() whenever i need it displayed.
$obj->getRPM() when required to display..

when printing date; date('Y/m/d', $obj->getDate())

[lsolesen]

Ok. So the renderer will print out what was converted:

$renderer = new Renderer_LatitudeAndLongitude();

$obj = new Math_GeographicPoint();
$obj->setLatLong(11.00, 11.00);
$obj->display($renderer);

// Latitude = 11.00 and Longitude = 11.00

$renderer = new Renderer_UTM();

$obj->display($renderer);

// Northing = 344342 and Easting = 24324 and Zone = 10S 

Having the following display in Math_GeographicPoint:

function display($renderer) {
    $renderer->update($this);

     echo $renderer->display();
} 

Do I undertstand you correctly that you would keep all conversion methods in the class Math_GeographicPoint.

[leblanc]

We both agree that presentation needs to be abstracted.

having $renderer->update($this);
makes your current object very delicate. your logic needs to be adjusted


for every point you should be able to get
getLatitude
getLongitude
getNorthing
getEasting
getZone

no need to run update.
and it should be $visitor->visit($this);

///basic api of a visitor renderer

$visitor = new specificvisitor();
... set visitor properties if you need to

$mainobject = new mainobject();
$mainobject->render($visitor);

echo $visitor->display();


---- actually i revisited your post...
looks like you got it correct. my mistake. -
I always call display on my visitor.. you did it inside... good job.
but i would change update($this); to visit($this)
would have made it clear on my end. for a minute there i thought you were updating your data object.. just because of a new display.

[lsolesen]

Ok. We agree on the presentation now

Do I undertstand you correctly that you would keep all conversion methods in the class Math_GeographicPoint.

Would you just instantiate the class, like it is now?

$gpoint = new Math_GeographicPoint();
$gpoint->setLatLong(11.00, 11.00);

$gpoint->convertLLToUTM();

// I know the names need to be changed to reflect more precisely what
// is returned, but for the example I just use the function names in the
// current implementation
echo $gpoint->getE(); // getting UTM Easting
echo $gpoint->getN(); // getting UTM Northing
echo $gpoint->getZ(); // getting UTM Zone 

[leblanc]

Do I undertstand you correctly that you would keep all conversion methods in the class Math_GeographicPoint.

yes

Would you just instantiate the class, like it is now?

no. how can you create an empty geopoint?
new Math_GeographicPoint();
// I normally have ArgumentExceptions if you try to instantiate an object with bad parameters.
// someone can call $gpoint->getE(); before you even set data.
to delicate for my taste.


should be:
new GeographicPoint(lat, long)
new GeographicPoint('string address')


you shouldn't need $gpoint->convertLLToUTM(); if you go my route.


because once you instantiate the object you can safely call all of these functions:
getLatitude
getLongitude
getNorthing
getEasting
getZone

[lsolesen]

So now we reached the crux I also dislike the empty geographic point. However, suppose I only have the data in UTM and not in longitude or latitude or even an address

Would you do?

$gpoint = Math_GeographicPoint::createFromUTM(3243243, 324243, '10S');
$gpoint = Math_GeographicPoint::createFromAddress('520 20th, 90402, Santa Monica, CA, USA');
$gpoint = Math_GeographicPoint::createFromLambert(11.00, 11.00);

class Math_GeographicPoint {
    public function __construct($lat, $long) {
        // invalid values throws new Exception
        $this->lat = $lat;
        $this->long = $long;
    }
    public function createUTM($easting, $northing, $zone) {
         // do some conversion and return a GeographicPoint
         return new Math_GeographicPoint($lat, $long);
    }
} 

[leblanc]

data in UTM and not in longitude or latitude or even an address

when you construct your object... you need to grab all fields to populate the get... functions.

I haven't studied this area enough .. i had to look up crux cute ... but you should be able to convert back and forworth.. example in the past i used a webservice to convert between address to lat and long and vice versa.


no i don't agree when your createFrom approach..
because I want access to
getLatitude
getLongitude
getNorthing
getEasting
getZone

all those functions once the object is created. so you need a conversion mechanism built into your object.

if its not a simple numeric calculations then create a private variable for lat, long, northing, easting, zone; and in your constructor do all the expensive operation to find all the values... if a webservice cache a serialized result set.

[lsolesen]

Yeah, I know that I need to access all of those variables through getLatitude() etc. when the object is created. But I also need to create the geographic point somehow, and suppose I only have UTM values.

I can only have one constructor How would you construct the object?

Essentially the object has to be able to handle the following coordinate sets:

- Latitude And Longitude
- UTM
- Lambert projection
- Maybe even address

Those are four different ways to express a GeographicPoint. Thanks for your patience, but please enlighten me how you would do it

[leblanc]

since php doesn't implement this nicely for us.. we are stuck to this syntax
http://us2.php.net/func_num_args

      function __construct()
      {
            $numargs = func_num_args();

            if($numargs >= 2)
            {
                   $test = func_get_arg(0);

check types
# is_ array
# is_ binary
# is_ bool
# is_ buffer
# is_ callable
# is_ double
# is_ float
# is_ int
# is_ integer
# is_ long
# is_ null
# is_ numeric
# is_ object
# is_ real
# is_ resource
# is_ scalar
# is_ string
# is_ unicode

if comparing object use 
instanceof

if doesn't match up with argument list throw argumentexception
                  ...
            }
            else
            {
...
            }
      } 

[lsolesen]

I dont quite like that approach, as it is pretty difficult to document. Maybe one could do something like the following:

class GeographicPoint_UTM extends GeographicPoint {
    function __construct($easting, $northing, $zone) {
        // through polymorphism
        $this->convertTMtoLL();
    }
}

$utm = new GeographicPoint_UTM(10123231, 112331, '10S');

// This could be used directly if it just extends the GeographicPoint

$utm->getLatitude(); 

Or as follows

class GeographicPoint_UTM {
    function __construct($easting, $northing, $zone) {
    }
    function iAm() { return 'utm'; }
}


$gpoint = new GeographicPoint($utm);

class GeographicPoint {
    function __construct($object) {
        if ($object->iAm() == 'utm') {
            $this->convertTMToLL(); // fill values
        }
    }

} 

However, there is also a problem with this approach. To get Lambert coordinates you need to do a lot of setup. In the current implementation through Math_GeographicPoint::configLambert. But if I never need Lambert, that would be pretty annoying to have to set GeographicPoint up to handle this. How can one handle a special case but still keep the basic object consistent and usable? (that is the conversion between LatLong og UTM)

[leblanc]

no thats starts going another direction.

overloading is the proper way to go.


i take that approach in my calendar component
function __construct($object)
but its because i implement a builder pattern.

new Calendar(new GregorianCalendar())

//its not about providing a nice constructor;
you move to that step when you truelly have a totaly new object type.. not for a nice constructor. Example gregorian calendar uses a totally different set of calculations then another type of calendar. But all have similar concepts that can be implemented through a builder interface.


also
if ($object->iAm() == 'utm')

should be
if( $object instanceof GeographicPoint_UTM)


sitepoint! when am i going get a "programming god" avatar?

[lsolesen]

Well, I have to disagree. You need a nice constructor. Maybe the following would be a good idea:

class GeographicPoint_UTM {
    function __construct($easting, $northing, $zone) {
        $this->easting = $easting;
        $this->northing = $northing;
        $this->zone = $zone;
    }
    function getEasting();
    function getNorthing();
    function getZone();

    function convertToLatitudeAndLongitude() {
        // do the conversions
        return GeographicPoint_LatitudeLongitude($latitude, $longitude);
    }
    function convertToLambert($config) {
        // do the conversions
        return GeographicPoint_Lambert($easting, $northing, $config);
    }
} 

It will make sense to treat the three types of coordinate sets as different objects, I would think.

[sweatje]

Given your choices, I prefer the static methods acting as GeographicPoint factories from various coordinate systems.

[lsolesen]

Ok. Now I ended up with splitting the whole things into smaller classes. I might want to create static factories from GeographicPoint to make the library easier to use though.

My current api is now:

http://public.intraface.dk/docs/Math_GeographicPoint/

With a usage example:

http://svn.intraface.dk/intrafacepub...es/example.php

Any improvements you could suggest?