adlnet / xAPIWrapper

Wrapper to simplify communication to an LRS
https://adlnet.gov/projects/xapi/
Apache License 2.0
219 stars 114 forks source link

funny conditional #106

Closed rscharfer closed 6 years ago

rscharfer commented 6 years ago

This conditional bugs me. If the first part of the conditional is true, then the second part can't possible be false. Isn't the second part redundant? It is used several times in the documentation. if (res.more && res.more !== "") ...

rscharfer commented 6 years ago

Isn't an empty string always a falsely value? Meaning if the more value is defined as an empty string, the first condition doesn't pass?

On Fri, Apr 27, 2018, 14:28 vbhayden notifications@github.com wrote:

res.more checks for whether that is defined at all. res.more !== "" checks that it's at least not a non-empty string.

To your point,

res.more = null first part is false res.more = "" first part is true, second is false. res.more = both true

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/adlnet/xAPIWrapper/issues/106#issuecomment-384955528, or mute the thread https://github.com/notifications/unsubscribe-auth/AEGNPejbWyA2qebv5nsI91XEhnIef-Mzks5tsw8HgaJpZM4TqQ0P .

vbhayden commented 6 years ago

Yes, but there's nothing inherently wrong with that format aside from some redundancy. I'm surprised that "" (and apparently 0 length iterables in general) will falsify an if statement and I imagine the developers who used these conventions also balked at that.

Personally, I consider anything of form if (something) to check whether that value is either null, undefined, or precisely false. JavaScript / Python may consider zero length iterables to follow suite given their value as web languages, but I don't think the if statements need to change right now.