On Sat, Jan 8, 2011 at 11:36 PM, Nicholas C. Zakas
<[email protected]>wrote:
> Hi Nick,
>
> I took a quick look at your code. Your comments already point out a few
> things you want to fix, so I’ve not mentioned any of those. My comments are:
>
Thank you for taking your time to do this!
>
> 1. Lines 34 and 36 can be combined (could be var slice =
> Array.prototype.slice, noodles;).
>
>
Yeah, there was no need to separate them, I've combined the lines.
> 2. On line 47 – I typically like to put the public interface for a library
> at the bottom of the file. Nothing technically wrong with putting it at the
> top, I just find it easier to follow when I define the individual pieces
> first and then export the public interface.
>
Yeah, it was basically an artifact of the development process that it ended
up on the top. I like the bottom better as well, so I will go through and
change this a little later.
>
> 3. I see you’ve used the timed array processing technique from High
> Performance JavaScript. Glad you’re having fun with it. [image: Smile]
>
>
:D I like counting ms so much more than just doing another setTimeout every
n iterations!
> 4. There’s a handful of places where you use underscore as a named function
> argument. I prefer to always use explicit names so there’s no confusion. I’d
> also suggest that some of the other function arguments could be a bit longer
> to better indicate their usage.
>
My past experiences with Prolog and Erlang have just ingrained in me that if
you aren't going to use a variable, you make it an underscore to
"officially" ignore it. I think I will start a new thread to discuss this in
a javascript setting...
Regarding the other arguments that might be too terse, which ones were you
referring to? Obviously I am familiar with the code and so I'm looking at it
from a different perspective and it can be hard for me to switch my
perspective.
>
> 5. Both in every() and some() you have a try-catch around a call to
> reduce(). However, since the internals for reduce() use timers to delay
> processing, you won’t catch any errors that occur after 50ms (the timer code
> runs in the global scope, outside of the try-catch).
>
This is not a bug, and I alluded to it in the bigger comment above, but I
suppose I should have made an inline comment. These are for the cases when
the array is empty and a TypeError will be thrown before the setTimeouts
start happening.
However, I am glad you made me look at this code again, because I realize
that since I supply an initial param, the type error will never be thrown! I
can safely remove the try/catch block :)
>
> 6. In every() and some(), it would be helpful to have some comments about
> arguments, as this code is a bit confusing:
>
>
> if ( arguments.length < 3) {
> callback = testFn || function () {};
> testFn = function (o, next) {
> return next(!!o);
> };
> }
>
> I had to read it a few times before I realized (I think) that when there
> are two arguments, the second is considered the callback instead of the test
> function. I’m not sure if that’s intentional or not, but I can’t think of a
> reason why you’d want do this (wouldn’t you just be iterating over the items
> in the array without doing anything to those items?).
>
Yes, that is madly in need of a comment. The intention is that when you pass
two arguments (which I should make more explicit by checking if the length
is 2 instead of less than 3) than the testFn is assumed to be coercion to
boolean. For example,
noodles.every([1, true, {}, null], console.log.bind(console));
should log "false".
>
> Overall, looks like a nifty bit of coding. I hope these comments help.
>
>
Yes, they did! Again, thank you so much!
_Nick_
--
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]
