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

Introducing GPSPoint Validator #18

wants to merge 11 commits into from

Conversation

zf2timo
Copy link
Contributor

@zf2timo zf2timo commented Jul 13, 2015

In this PR i want to introduce a new Validator, which validates GPS Coordinates. The requirements to the Validator are:

  • Validate Decimal Degree Coordinates
  • Validate Degree Minutes Second (DMS) Coordinates
  • Validate minimum and maximum Values for feasible coordinates

Basic Unit Tests are included.
Please Review the code and let me know your opinion.


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.

@zf2timo
Copy link
Contributor Author

zf2timo commented Jul 14, 2015

With my commits, the Code Coverage decreased. Is there a way i can also cover the private Methods to increase the CC?

@vaclavvanik
Copy link

In regard of naming conventions this should be renamed to GpsPointValidator

@zf2timo
Copy link
Contributor Author

zf2timo commented Jul 14, 2015

I think the suffix validator is needless because the word is already in the Namespace. Also all other Validators did not have this suffix.
The CamelCase is acceptable.

@vaclavvanik
Copy link

@zf2timo Sorry, I meant GpsPoint only.

return false;
}

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

Choose a reason for hiding this comment

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

Shouldn't the explode have the third argument 2 to ensure no more than 2 values are returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weierophinney Yes, it's cleaner. But should i make this change, because it was already merged.

Copy link
Member

Choose a reason for hiding this comment

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

I made the change during merge, iirc.
On Jul 17, 2015 6:56 AM, "zf2timo" notifications@github.com wrote:

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

  • \* 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);
    

@weierophinney https://github.com/weierophinney Yes, it's cleaner. But
should i make this change, because it was already merged.


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

@weierophinney weierophinney added this to the 2.6.0 milestone Jul 16, 2015
@weierophinney weierophinney self-assigned this Jul 16, 2015
weierophinney added a commit that referenced this pull request Jul 16, 2015
weierophinney added a commit that referenced this pull request Jul 16, 2015
@weierophinney
Copy link
Member

Merged to develop for release with 2.6.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants