Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validates polygon coords passed to boolean. #67

Closed
wants to merge 2 commits into from
Closed

Validates polygon coords passed to boolean. #67

wants to merge 2 commits into from

Conversation

erikh2000
Copy link

@erikh2000 erikh2000 commented Feb 13, 2018

See issue #57 and #65 for context.

The philosophy of the validator is to fail earlier with a clear message to the caller that the passed param is wrong. At the least, it should save time on answering support issues related to troubleshooting bad params.

I understand this project's goals to have performance as a high priority, and the extra code execution to check the params is a tradeoff of sorts. However, it's a small, one-time check outside of any repeating loop and in running the test suite with console.time/timeEnd checks, I saw no delays greater than a tenth of a millisecond for validity checking.

I modified one test under edge_cases.test.js to have closing coordinates. I think this change is consistent with the intent of the test.

@erikh2000
Copy link
Author

erikh2000 commented Feb 21, 2018

A few more thoughts on this. The PR here doesn't increase the processing time by any substantial amount, IMO. But I understand that performance is important in this project.

And also, there are more validations of input that we could be doing which could increase processing time more. For example, checking params to make sure caller is not passing inner ring coordinates that are outside of outer ring. These would create a real tradeoff to consider - Speed of library vs cutting down your support load and making users more self-reliant and less likely to blame the library for their own mistakes.

With that in mind, you may want to have a set of wrapper functions that perform the validation, e.g. diffChecked() that are like...

function diffChecked(subject, clipping) {
  var validationFailures = validateCoords(subject, clipping));
  if (validationFailures) { throw('You passed in garbage - ' + validationFailures); }
  diff(subject, clipping);
}

The idea being that if someone wants pure speed and has high confidence in what they pass in, they call diff() directly. But if they want to know why diff() is blowing up on them, they can call diffChecked(). In support issues, this can be a requested first step to the person filing an issue.

Or maybe some users prioritize robustness and ease of fixing errors over speed (this would be me). These people might just call checked versions by default and leave them that way until the calling code seemed to need optimization.

So I think you can have the best of both worlds with a set of wrapper functions that do more validation.

@rowanwins
Copy link
Collaborator

The other option, rather than error-ing, would be to mutate the input and add the end coord pair if they are missing.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants