Decode() accepts latitude digits beyond North Pole

[viettaml]

"C2000000+" is a slice at the North Pole. But Decode() will accept "F2000000+" and return a CodeArea with latitude values of 110 degrees.

Now that I think about it, Decode() has no way of signaling an error other than returning an invalid CodeArea, so maybe the solution is to add an IsValid() method to CodeArea.

[viettaml]

(Forgot to mention I observed this in the cpp implementation; can't speak to the others.)

[viettaml]

Looking at the Decode() implementation some more, it's clear that it doesn't perform any sort of validation. And maybe that's intentional, in which case the correct "fix" would be for openlocationcode::IsValid() to reject codes such as "F2...".

[bocops]

(Forgot to mention I observed this in the cpp implementation; can't speak to the others.)

Seems to be true for the Java implementation as well. There's isValidCode(String) which does check the first two characters of a plus code - but that method doesn't seem to be called from decode() either directly or indirectly.

[bocops]

Actually, disregard that. In the Java implementation, an OpenLocationCode object can only be created with a code string that passed the isValidCode() check in the first place.

[zongweil]

@drinckes what do you think about this? Should we modify isValid in the C++ implementation?

[viettaml]

I think maybe this is working as intended, and that what I think of as "IsValid()" is actually fulfilled by "IsFull()" which does appear to validate (albeit inefficiently) the first two digits.

[drinckes]

I'm not a c++ expert so I'm happy to defer to others on what the best solution is. Since we don't throw exceptions, we're going to accept an invalid string and return something. The question is, what should the something be?

I'm tending to lean away from adding a flag attribute to the codeArea but I'm reasonably happy with saying that a code area that is all zeros (lat lo, lat hi, lng lo, lng hi, codelen) indicates invalid input, and following @viettaml original suggestion to (at least for C++) add an isValid() to the codearea class.

That means:

• there will be some languages where bad input results in throwing an exception (Java, Javascript) or returning an error (Go)
• in other languages decode() returns a CodeArea object with an isValid() method, and encode() with invalid input returns an empty string (say)

What do you think?

[viettaml]

SGTM. I'll have a go at implementing this when I have a few minutes. Thanks!

[viettaml]

#185

[drinckes]

An alternative (and I know you did just send a pr), would be to refactor decode() to take as arguments the code string, and a pointer to a CodeArea, and return a bool depending on whether the code string was valid:

  ca = openlocationcode::CodeArea();
  if (openlocationcode::Decode(query, &ca)) {
    // the query was a valid code, ca now holds the decoded values.
  } else {
    // query is not a valid code
  }

That would change the method signature though. At first thought that's a disadvantage, but it would mean that because the code would have to be refactored, anyone using this method would be more likely to add this kind of check to it.

@viettaml what do you think? (It's ok to say "yeah nah".)

[viettaml]

I'm reluctant to change the syntax of an existing function, though I'm more open to adding or overloading the function. That being said, even after my PR Decode() doesn't do a full job of validating codes, so I wouldn't want users relying on it to do so (they should use "IsFull()" instead or first, which does seem to reject the out-of-range codes that initially prompted this report, in addition to various other syntactic errors).

With that in mind, following the philosophy of avoiding duplicated or overlapping functionality in an API, I'm inclined to leave Decode() as-is. By the same argument my PR isn't strictly necessary, and I'd be okay with that too.

[drinckes]

Let's close this since we seem to have settled on doing nothing. ;-)

Feel free to reopen if you change your mind!

[fulldecent]

A full code MUST match one these regular expressions:

1. /^[2-9C][2-9CFGHJMPQRV]0{6}\+$/
   

2. /^[2-9C][2-9CFGHJMPQRV][2-9CFGHJMPQRVWX]{2}0{4}\+$/
   

3. /^[2-9C][2-9CFGHJMPQRV][2-9CFGHJMPQRVWX]{4}0{2}\+$/
   

4. /^[2-9C][2-9CFGHJMPQRV][2-9CFGHJMPQRVWX]{6}\+([2-9CFGHJMPQRVWX]{2,7})?$/
   

I am working to add this to the specification.

And so the above is not valid.