shindig-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Dumont" <ddum...@us.ibm.com>
Subject Re: Review Request: Changing equality checks against undefined to typeof checks against 'undefined'
Date Mon, 09 Jan 2012 14:07:22 GMT


> On 2012-01-03 08:45:27, Paul Lindner wrote:
> > I'm okay with the patch as-is, but would prefer something like this which is a modified
version of the closure library goog.isDef
> > 
> > 
> > goog.isDef = function(val) {
> >   var undefined;
> >   return val !== undefined;
> > };
> >
> 
> Dan Dumont wrote:
>     Paul, is google's deployment running with advanced optimizations turned on?
> 
> Stanton Sievers wrote:
>     Paul, where would such a definition live such that it is accessible across all of
the JavaScript files?  Any idea how we could hook that up?  I'd be up for trying that... although,
if we were going to introduce goog.isDef I would like to replace all typeof instances with
that, which is going to be a much bigger change.
> 
> Ryan Baxter wrote:
>     maybe add it to core.util?  Or possibly have its own feature which you add as a dependency
to core...

This is the concern I have:

Even being generous with the name (for a public API I would rather see a more complete name:
isDefined) and location of the new util (shindig.util.isDef(foo)) 
That's 23 characters long to check if a variable is undefined.

The alternative is the regular undefined check typeof(foo)!='undefined'    (24 characters)
The shindig exported namespaces are rather long, and utility is supposed to be easy to remember
and more useful than plain js usage.
I'm not sure the added function call and 1 character savings is worth the addition of the
API.

Paul, is there any benefit to the isDef checking method over the typeof checking?  Or is the
only benefit the rather small length namespace offered by goog.isDef (which clashes with how
utility is organized in shindig)?


- Dan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3332/#review4174
-----------------------------------------------------------


On 2011-12-29 20:05:42, Stanton Sievers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3332/
> -----------------------------------------------------------
> 
> (Updated 2011-12-29 20:05:42)
> 
> 
> Review request for shindig.
> 
> 
> Summary
> -------
> 
> The purpose of this patch is to remove equality checking against the "undefined" keyword,
i.e., "foo != undefined".  I've replaced such instances with "typeof foo != 'undefined'" instead.
 This eliminates possible errors that can arise if the "undefined" keyword is reassigned.
 
> 
> A little background regarding the motivation for this patch.... As of ECMAScript 3 and
before ECMAScript 5, undefined is a keyword but is not restricted, thus, it's value can be
re-assigned.  This is generally caused by a programming error, such as assigning values to
object attributes without first checking that the object attribute exists - making them undefined.
 When these types of errors occur they are difficult to track down and debug.  This patch
is one way to help reduce possible errors caused by undefined being re-assigned.
> 
> To find instances to replace, I searched for this regular expression in the code base
using Eclipse search within *.js files: =[ ]*undefined[ ]*
> 
> I ignored certain files, such as external libs (CodeMirror, etc) and most tests.
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/cconviews.js
1225624 
>   http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js
1225624 
>   http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js
1225624 
>   http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.json/json-flatten.js
1225624 
>   http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/i18n/datetimeparse.js
1225624 
>   http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/open-views/viewenhancements-container.js
1225624 
>   http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/shindig.container/shindig-container.js
1225624 
>   http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/shindig.uri/uri.js
1225624 
>   http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/osapi/batchtest.js
1225624 
> 
> Diff: https://reviews.apache.org/r/3332/diff
> 
> 
> Testing
> -------
> 
> Built everything.  Ensured that the common container sample gadgets still render.
> 
> 
> Thanks,
> 
> Stanton
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message