I disagree with Rob.
This is the assignment way of creating a function:
var whatever = function () {};
This is the declaration way of creating a function:
function whatever () {}
The second one is hoisted, which is often convenient and rarely a problem.
However, in those exotic instances when hoisting is a problem it is
catastrophic. There is nothing you can do to prevent the var keyword from
being hoisted, but you can prevent functions from being hoisted. The only
exception about hoisting and declared functions is when the function is
immediately invoked just like the "filterObj" function in your code. This is
because the function executes immediately in its current form and position.
The common argument in favor of the declaration method, which I believe Rob was
implying, is that for some people the declaration method is easier to read
because you can quickly identify what is or is not a function. This is most
certainly valid and I even agree with this fully for smaller applications. For
larger applications containing many functions this argument fails. What is the
value in trying to identify what is or is not a function if you have a
container with 60 functions? At that point it is significantly faster to read
the code by looking for the identifier and then determining whether the
identifier is or is not a function by reading the very next word to its right.
I also find that the declarative way is more challenging to maintain in an
extremely large application because it is not tied to the var keyword. I find
that binding references, whether variables or function names, to a single var
keyword dramatically reduces complexity of actually reading the code. Because
of this I always recommend using no more than one var keyword per function and
ensuring that nothing comes before this one var keyword except immediately
invoked functions and the "use strict" pragma. This one variable rule when
used with the "use strict" pragma results in generally more sturdy and portable
code as well as code that is easier to read.
I see that around line 298 you are using a try/catch block. I absolutely
detest try/catch blocks as a cheap attempt to mitigate known bugs. If you are
aware of the possibility of a bug then correct your code. It the bug is the
result of user input then output a response to this effect so that your user
will know why the executed failed to perform as expected.
I also do not see the value in using a return on an anonymous object literal at
the end of your application. Clearly this goes to the architecture of your
application in that you want a series of sub-global functions to execute in a
particular order that may or may not return anything but none the less result
in a the global "filterObj" returning some object literal. This is an old
convention, particularly the use of something named "init" that strings
together a series of unrelated executions. Part of the reason you are probably
using this convention is that all the functions in your application appear to
exist in an equal scope directly under the global "filterObj" contain, and this
is inefficient. Only create functions in the scope where they are needed, or
if some functions are needed in different locations then at the minimum
possible scope for reuse. Doing this decreases lookups, which dramatically
increases execution speed. Speed in JavaScript really comes down to reducing
lookups and using the most appropriate operator or method for a given job.
You also have some minor syntax violations in your code. For instance the
"attachClickListener" is missing a terminating semicolon. This would prevent a
bug free minification of your code. I would suggestion applying the prior
mentioned guidance first and then after submitting your code through the JSLint
tool.
Thanks,
Austin Cheney, CISSP
From: [email protected] [mailto:[email protected]] On Behalf
Of Rob Griffiths
Sent: Wednesday, October 05, 2011 10:41 AM
To: [email protected]
Subject: Re: [JSMentors] I'm trying to write better JavaScript and I'm looking
for some feedback on the code I wrote for a small app.
I see you do
var runFilters = function(){...};
Whilst this is not wrong, it would be far cleaner to write
function runFilters() {...}
in my opinion, as its quite clearly a function and not a reference to one.
3. Lines 66-81 are painful, and I think you know it. Instead of looping through
every element, try giving all the elements a unique id, or something like that,
so that you can hash the results of one of those arrays and create an O(n) loop
instead of an O(n*n) pattern.
I'd have to agree with Nathan, You could probably manage your data better and
compare a simple ID.
--
To view archived discussions from the original JSMentors Mailman list:
http://www.mail-archive.com/[email protected]/
To search via a non-Google archive, visit here:
http://www.mail-archive.com/[email protected]/
To unsubscribe from this group, send email to
[email protected]
--
To view archived discussions from the original JSMentors Mailman list:
http://www.mail-archive.com/[email protected]/
To search via a non-Google archive, visit here:
http://www.mail-archive.com/[email protected]/
To unsubscribe from this group, send email to
[email protected]
