Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Introducing GPSPoint Validator #18

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 131 additions & 0 deletions src/GPSPoint.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
<?php
/**
* Zend Framework (http://framework.zend.com/)
*
* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2015 Zend Technologies USA Inc. (http://www.zend.com)
* @license http://framework.zend.com/license/new-bsd New BSD License
*/

namespace Zend\Validator;

final class GPSPoint extends AbstractValidator
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is final class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See Link from @manuakasam in Comments below.

{

const OUT_OF_BOUNDS = 'gpsPointOutOfBounds';
const CONVERT_ERROR = 'gpsPointConvertError';
const INCOMPLETE_COORDINATE = 'gpsPointIncompleteCoordinate';

/**
* @var array
*/
protected $messageTemplates = [
'gpsPointOutOfBounds' => '%value% is out of Bounds.',
'gpsPointConvertError' => '%value% can not converted into a Decimal Degree Value.',
'gpsPointIncompleteCoordinate' => '%value% did not provided a complete Coordinate',
];

/**
* Returns true if and only if $value meets the validation requirements
*
* If $value fails validation, then this method returns false, and
* getMessages() will return an array of messages that explain why the
* validation failed.
*
* @param mixed $value
* @return bool
* @throws Exception\RuntimeException If validation of $value is impossible
*/
public function isValid($value)
{
if (strpos($value, ',') === false) {
$this->error(GPSPoint::INCOMPLETE_COORDINATE, $value);
return false;
}

list($lat, $long) = explode(',', $value);

if ($this->isValidCoordinate($lat, 90.0000) && $this->isValidCoordinate($long, 180.000)) {
return true;
}

return false;
}

/**
* @param string $value
* @param $maxBoundary
* @return bool
*/
private function isValidCoordinate($value, $maxBoundary)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is private method?

Copy link
Member

Choose a reason for hiding this comment

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

@freax We've been gradually moving towards private visibility by default. This encourages good encapsulation, and moves the onus for justifying protected/public visibility to consumers; in other words, we need a very, very good reason to make something visible to extension writers.

Doing this eases our maintenance burden, because we can add, change, or remove private methods with little to no impact on end-users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just look around a whole framework and you hardly notice a private methods. You need a very good reason to mark method as private. I think protected is better.

Copy link
Member

Choose a reason for hiding this comment

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

Again, this is the direction we're moving. While we've defaulted to
protected in the past, we're changing practices based on the maintenance
issues the practice has created. (For your reference, I'm project lead; you
can take this as canonical.)
On Jul 13, 2015 5:12 PM, "dima" [email protected] wrote:

In src/GPSPoint.php
#18 (comment)
:

  •    list($lat, $long) = explode(',', $value);
    
  •    if ($this->isValidCoordinate($lat, 90.0000) && $this->isValidCoordinate($long, 180.000)) {
    
  •        return true;
    
  •    }
    
  •    return false;
    
  • }
  • /**
  • \* @param string $value
    
  • \* @param $maxBoundary
    
  • \* @return bool
    
  • */
    
  • private function isValidCoordinate($value, $maxBoundary)

Just look around a whole framework and you hardly notice a private
methods. You need a very good reason to mark method as private. I think
protected is better.


Reply to this email directly or view it on GitHub
https://github.com/zendframework/zend-validator/pull/18/files#r34518201.

Choose a reason for hiding this comment

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

@freax have a read at http://ocramius.github.io/blog/when-to-declare-classes-final/
This is a pretty easy to understand thing. When you declare classes final, there's no single reason to have protected functions as the class will never be extended. /cc @Ocramius

Copy link
Contributor

Choose a reason for hiding this comment

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

When you declare classes final, there's no single reason to have protected functions as the class will never be extended.

Yes, it's pretty obvious. But my question is why need to forbid extend class from framework, especially from validator package.

Copy link
Member

Choose a reason for hiding this comment

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

But my question is why need to forbid extend class from framework…

See again http://ocramius.github.io/blog/when-to-declare-classes-final/ (2. Encouraging composition)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, if treat the whole article as the ultimate truth...

{
$this->value = $value;

$value = $this->removeWhiteSpace($value);
if ($this->isDMSValue($value)) {
$value = $this->convertValue($value);
} else {
$value = $this->removeDegreeSign($value);
}

if ($value === false || $value === null) {
$this->error(self::CONVERT_ERROR);
return false;
}

$doubleLatitude = (double)$value;

if ($doubleLatitude <= $maxBoundary && $doubleLatitude >= $maxBoundary * -1) {
return true;
}

$this->error(self::OUT_OF_BOUNDS);
return false;
}

/**
* Determines if the give value is a Degrees Minutes Second Definition
*
* @param $value
* @return bool
*/
private function isDMSValue($value)
{
return preg_match('/([°\'"]+[NESW])/', $value) > 0;
}


/**
* @param string $value
* @return bool|string
*/
private function convertValue($value)
{
$matches = [];
$result = preg_match_all('/(\d{1,3})°(\d{1,2})\'(\d{1,2}[\.\d]{0,6})"[NESW]/i', $value, $matches);
Copy link
Contributor

Choose a reason for hiding this comment

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

  • It can be degrees only or degrees minutes formats, like 120°W, and this preg will fails.
  • You can avoid i modifier by replace [NESW] to [NESWnesw]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Ok, i will have a look, how this can be done.
  • Why i should avoid the i modifier?


if ($result === false || $result === 0) {
return false;
}

return $matches[1][0] + $matches[2][0]/60 + ((double)$matches[3][0])/3600;
}

/**
* @param string $value
* @return string
*/
private function removeWhiteSpace($value)
{
return preg_replace('/\s/', '', $value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can \s meant anything but space? If not, I think str_replace better.

Copy link
Member

Choose a reason for hiding this comment

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

\s is any whitespace character, and includes tab, CR, and EOL, among
others.
On Jul 13, 2015 5:06 PM, "dima" [email protected] wrote:

In src/GPSPoint.php
#18 (comment)
:

  •    $result = preg_match_all('/(\d{1,3})°(\d{1,2})\'(\d{1,2}[.\d]{0,6})"[NESW]/i', $value, $matches);
    
  •    if ($result === false || $result === 0) {
    
  •        return false;
    
  •    }
    
  •    return $matches[1][0] + $matches[2][0]/60 + ((double)$matches[3][0])/3600;
    
  • }
  • /**
  • \* @param string $value
    
  • \* @return string
    
  • */
    
  • private function removeWhiteSpace($value)
  • {
  •    return preg_replace('/\s/', '', $value);
    

Can \s meant anything but space? If not, I think str_replace better.


Reply to this email directly or view it on GitHub
https://github.com/zendframework/zend-validator/pull/18/files#r34517767.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. My point is that very unlikely there is need to check coordinate value in format like
120°

34'

23"
N
In most cases strip spaces will be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A line break is very unlikely - that's right. But a \t is valid case and should not cause a validation error.

}

/**
* @param string $value
* @return string
*/
private function removeDegreeSign($value)
{
return str_replace('°', '', $value);
}
}
83 changes: 83 additions & 0 deletions test/GPSPointTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
<?php
/**
* Zend Framework (http://framework.zend.com/)
*
* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2015 Zend Technologies USA Inc. (http://www.zend.com)
* @license http://framework.zend.com/license/new-bsd New BSD License
*/

namespace ZendTest\Validator;

use Zend\Validator\GPSPoint;


/**
* @group Zend_Validator
*/
class GPSPointTest extends \PHPUnit_Framework_TestCase
{

/**
* @var GPSPoint
*/
protected $validator;

public function setUp()
{
$this->validator = new GPSPoint();
}


/**
* @dataProvider basicDataProvider
* @covers \Zend\Validator\GPSPoint::isValid
*/
public function testBasic($gpsPoint)
{
$this->assertTrue($this->validator->isValid($gpsPoint));
}

/**
* @covers \Zend\Validator\GPSPoint::isValid
*/
public function testBoundariesAreRespected()
{
$this->assertFalse($this->validator->isValid('181.8897,-77.0089'));
$this->assertFalse($this->validator->isValid('38.8897,-181.0089'));
$this->assertFalse($this->validator->isValid('-181.8897,-77.0089'));
$this->assertFalse($this->validator->isValid('38.8897,181.0089'));
}

/**
* @covers \Zend\Validator\GPSPoint::isValid
* @dataProvider ErrorMessageTestValues
*/
public function testErrorsSetOnOccur($value, $messageKey, $messageValue)
{
$this->assertFalse($this->validator->isValid($value));
$messages = $this->validator->getMessages();
$this->assertArrayHasKey($messageKey, $messages);
$this->assertContains($messageValue, $messages[$messageKey]);
}

public function basicDataProvider()
{
return [
['38° 53\' 23" N, 77° 00\' 32" W'],
['15° 22\' 20.137" S, 35° 35\' 14.686" E'],
['65° 4\' 36.434" N,-22.728867530822754'],
['38.8897°, -77.0089°'],
['38.8897,-77.0089']
];
}

public function ErrorMessageTestValues()
{
return [
['63 47 24.691 N, 18 2 54.363 W', GPSPoint::OUT_OF_BOUNDS, '63 47 24.691 N'],
['° \' " N,° \' " E', GPSPoint::CONVERT_ERROR, '° \' " N'],
['° \' " N', GPSPoint::INCOMPLETE_COORDINATE, '° \' " N'],
];
}
}