Is there a more efficient way of coding this?

Question

The code in question is the code contained within the 2nd foreach loop, the purpose of which is to prevent exact duplicate latitude and longitudes.

foreach($aUsers as $k => $v)
        {
            // generate address
            $aAddress = array();
            if(!empty($v['city_location']))
                $aAddress[] = $v['city_location'];
            if(!empty($v['country_child_id']))
            {
                $aRow = $oDb->select('name')
                     ->from(getT('country_child'))
                     ->where('child_id = \''.$v['country_child_id'].'\'')
                     ->execute('getRow');
                $aAddress[] = $aRow['name'];
            }
            if(!empty($v['postal_code']))
                $aAddress[] = $v['postal_code'];
            if(!empty($v['country_iso']))
            {
                $aRow = $oDb->select('name')
                     ->from(getT('country'))
                     ->where('country_iso = \''.$v['country_iso'].'\'')
                     ->execute('getRow');
                $aAddress[] = $aRow['name'];
            }
            $sAddress = implode(', ',$aAddress);
            /// get location
            $aLatLon = $oGeoMap->getLatLon($v['user_id'],1,$sAddress);
            if($aLatLon['success'] === true)
            {
                foreach($aUsers as $k2 => $v2)
                {
                    $iAdd = .01;
                    $iAttempts = 0;
                    while($v2['latitude'] == $aLatLon['latitude'] && $v2['longitude'] == $aLatLon['longitude'])
                    {
                        $iAttempts++;
                        switch($iAttempts){
                            case 1:
                                $aLatLon['latitude'] += $iAdd;
                                break;
                            case 2:
                                $aLatLon['longitude'] += $iAdd;
                                break;
                            case 3:
                                $aLatLon['latitude'] += $iAdd;
                                $aLatLon['longitude'] += $iAdd;
                                break;
                            case 4:
                                $aLatLon['latitude'] -= $iAdd;
                                $aLatLon['longitude'] -= $iAdd;
                                break;
                            case 5:
                                $aLatLon['latitude'] += $iAdd;
                                $aLatLon['longitude'] -= $iAdd;
                                break;
                            case 6:
                                $aLatLon['latitude'] -= $iAdd;
                                $aLatLon['longitude'] += $iAdd;
                                break;
                            case 7:
                                $iAdd += .01;
                                $iAttempts = 0;
                                break;
                        }
                    }
                }
                $aUsers[$k]['latitude'] = $aLatLon['latitude'];
                $aUsers[$k]['longitude'] = $aLatLon['longitude'];
                $aUsers[$k]['address'] = $sAddress;
            }
            else
                unset($aUsers[$k]);
        }

Answer 1

You could use switch() with a semicolon... E.g.

switch($attempts) {
    case 1;
    case 3;
    case 5;
        $aLatLon['latitude'] += $iAdd;
    break;
}

Haven't tested this; but I'm sure if you keep going like this or something similar, it will work.

Hopefully that helped

Answer 2

I strongly agree with Frank Farmer's comment, but if you insist on this way of doing things try something along these lines:

$lls = array();
static $offs = array( array(1,0), array(0,1), array(1,1),
    array(-1,-1), array(1,-1), array(-1,1) );

foreach($aUsers as $k=>&$v) {
    $lla=$la=somelat; $llo=$lo=somelong;
    for($add=.01;isset($lls["{$lla}x$llo"]);$add+=.01) {
        foreach($offs as $o) {
            $lla=$la+$o[0]*$add; $llo=$lo+$o[1]*$add;
            if(!isset($lls["{$lla}x$llo"])) break;
        }
    }
    $lls["{$lla}x$llo"] = true;
    $v['latitiude']=$lla; $v['longtitude']=$llo;
    $v['address'] = $sAddress;
}

That is, if I understand your intention correctly. And change variable names to adhere to your standards yourself ;-)

Code is untested.

And no, I'm not going to re-read the full code ;-)

Answer 3

You could try using a second data structure that holds existing latitude+longitude positions, e.g. a 2-D array of users indexed by latitude then longitude. Rather than looping over the $aUsers array, you check whether a given latitude and longitude exists within this other data structure.

Note that as you can only use integers and strings as array keys and casting floating point numbers to either integers or strings will result in a loss of precision, you should convert floating point numbers to their bit-string equivalents if you're using latitude and longitude as keys. A simple pack('d', $l) should work, or you could use:

function fl2hex($l, $fmt='d') {
    $hex = unpack('H*', pack($fmt, $l));
    return $hex[1];
}

If you know your latitude and longitude are stored as floats and not doubles (though I'm fairly certain PHP only uses doubles), you can use 'f' as the pack format rather than 'd'.

For readability, you can refactor parts of the code (e.g. the switch($iAttempts)) as additional functions.

As for the problem of multiple people at a single location, another solution (assuming the map is displayed on a screen, rather than printed) is to use a different indicator for a group of people. When a user mouses over the group icon, it can "explode" into a star of individual icons (if that makes sense), each with a line connecting the icon to the people's location. I've seen something similar in a couple applications, but can't think which ones right now.

Answer 4

Suggestion:

This is more of a general suggestion for all the code, not just the inner loop. I would change the code to use dedicated structures instead of arrays. Here's a basic example:

foreach($aUsers as $k => $v)
{
    $userInfo = new UserInfo($v);

    // generate address
    $sAddress = $userInfo->getAddress();

    /// get location
    $latLonResult = $oGeoMap->getLatLon($userInfo->user_id, 1, $sAddress);
    ...
}

Actual answer:

As far as optimizing the inner loop, I would just delete that code. Either come up with a new UI element for having multiple users in the same location, or ignore a user if their lat/long has already been added to the map.

If you must keep it, Michael's solution looked good.