refactored _isvaliddate function

[kshirish]

refactored _isvaliddate function

[ScottHamper]

Hey Kisley,

Thank you for checking out the library and spending time on it!

Using instanceof could definitely be a reasonable replacement for the first part of the conditional (Object.prototype.toString...), but we still need to keep the isNaN check.

An invalid date instance will still be an instance of a Date. For example, var myDate = new Date('2014-02-04T11:39:00Z') is a valid date in most modern browsers, but old IE does not accept this string format. Calling myDate.getTime() in old IE would return NaN.

The only downside to using instanceof for the first part of the conditional, is that apparently it does not work as expected on objects originating from other frames (each frame has their own copy of a root Date type, which will not equate to one another). Realistically, this is only likely to be an issue very rarely, if ever, for Cookies.js, especially since Cookies.js creates Date instances itself, unless one is directly passed in through the 'expires' option.

So it comes down to - is the very slight downside to this change worth a slight improvement in readability/brevity?

I'm pretty torn on this, personally. I don't think the limitation of instanceof is likely to be encountered, but also feel like it's better to err on the side of caution. It's more reliable to keep the function in its current form, even if only marginally so. On the other hand, I really like the succinctness of the instanceof approach. Ultimately though, I think that reliability/functional correctness is more important in the end.

Additionally, there's stuff like issue #36, so perhaps it would be more of a problem than I expect.

What are your thoughts?

[kshirish]

I agree it is hard to chose between readability and reliability at critical points like this. But as you said looking at it realistically, user will hardly encounter/face problem due to usage of instanceof therefore it could be a replacement for first condition to enhance readability.

[ScottHamper]

Alright, thanks for your input, Kisley!

I'm going to leave the code how it is - although it is not as concise as using instanceof, it is still not confusing on what it is doing (perhaps the "why" is not apparent, though). Conciseness is not worth the reliability trade off for me in this situation.