Conrad.js code review

review.md

Reviewing this version

Questions

Which browsers are targeted?

Would you consider making an AMD or UMD module instead of exporting to the global?

Comments

• L131. I feel you may get into random trouble if people use event names like valueOf, etc. Use Object.create(null) or a string-map lib
• L157. typeof arguments[0] === 'object' : null passes this test. You want ````Object(arguments[0]) === arguments[0]```
• L167-177 when you iterate over an array, use .forEach or one of its friends. In this case, I think you want .reduce (problem repeated in different places in the code)
• L205-206. Too much work :-p _handlers = Object.create(null) (recreate an empty object instead of cleaning it. It costs a bit more GC but... one object per _unbind call...)
• L261. This is tricky. The call may throw an error. You don't catch it, so one handler throwing prevents all other handlers from being called. It's unlikemy to be the expected behavior (add a comment if it's on purpose, though). Two possible solutions: try/catch and ignore the error or schedule each handler in its own setTimeout 0.
• L602. I wonder if a job shouldn't be an object with a kill/cancel method instead of the global killJob method.
• L620. It sounds like a lot of work to remove the job. Is the data structure right? Maybe killJob is called rarely enough that it doesn't matter?
• L738. _isRunning is an internal variable. You're in charge of making sure it remains a boolean, you shouldn't need to !!. TypeScript can help ;-)
• L893 - 949. I'm willing to bet Lodash has functions for this and does it better than you ever will. Just import the parts you need from Lodash
• L956 __dateNow: have you considered using performance.now?

Cosmetics

• L30: I believe the semilcolon isn't that important
• No need for the undefined in argument. It can't be overriden. That's an urban legend (or maybe true in super-old IEs?). If someone overides it, it can only be you by using the variable name, shadowing the global one. Just don't use this variable name ;-)
• I'm not sure the coding convention is necessary in the sense that if you export at the end, it's clear what's exported and consequently what isn't.
• .call(this) for an IIFE works, but the global object is then refered as this which is not an obvious way to reference it. I prefer:

(function(global){
   if(global.conrad){...}
})(this)

I feel global.conrad reads better than this.conrad

• L148. _bind: events is of type string, string[] should work too... or I don't know. Sometimes later, it's expected that it can be an object. Anyway, the doc doesn't match the code.
• L154. _bind: the use of this is awkward. what does it reference? (I guess the exported conrad reference, but I feel _bind would be better inlined or something)`
• L163. Object.prototype.toString.call(events) === '[object Array]' this is Array.isArray. Use it and polyfill if necessary. (happens in different places)

Questions

• Which browsers are targeted?
  • Actually, since conrad is just about pure JavaScript (no DOM, no weird methods), I expect it to work "everywhere". Modern browsers of course, but if I need something similar in IE8 for any reason, I hope it will work.
• Would you consider making an AMD or UMD module instead of exporting to the global?
  • Yep. Actually, I was pretty sure it was already done.

About the comments part:

Your points are definitely relevant. Some remarks:

• L167-177 when you iterate over an array, use .forEach or one of its friends. In this case, I think you want .reduce (problem repeated in different places in the code)
  • I usually avoid Array helpers in libraries and use them only in applications, just to never deal with polyfills.
• _L205-206. Too much work :-p _handlers = Object.create(null) (recreate an empty object instead of cleaning it. It costs a bit more GC but... one object per unbind call...)
  • Indeed. That (as well as the weird this reference in the cosmetics part) comes from a custom event dispatcher I use in lot of projects, where _handlers is actually this._handlers and I do not want to get leaked (for instance in domino.js). But you're definitely right here.
• L261. This is tricky. The call may throw an error. You don't catch it, so one handler throwing prevents all other handlers from being called. It's unlikemy to be the expected behavior (add a comment if it's on purpose, though). Two possible solutions: try/catch and ignore the error or schedule each handler in its own setTimeout 0.
  • Wow. I use this code for so long and I just never realized that... That's a huge issue that will have to be fixed in most of my projects, big thanks for noticing it!
• L602. I wonder if a job shouldn't be an object with a kill/cancel method instead of the global killJob method.
  • Looks nice as well, you're right. I have to think about it, but since no one except the new sigma draft uses conrad, it might still be time to update.
• The two last points:
  • Didn't now about these, looks nice.

About the cosmetics part:

I will just trust you on these, since you've clearly seen more code that I have.

• Your global way of writing this is clearly more readable.
• Did not event know about Array.isArray, looks more readable as well.

---

This was all very interesting, and will give a new version soon - and some patches in domino.js, the new sigma.js draft, etc... And thanks a lot. I've never seen a so precise and well organized code review, I'm impressed :)

@jacomyal

I usually avoid Array helpers in libraries and use them only in applications, just to never deal with polyfills.

Hard call to make. These functions are standard and pretty useful, especially as they make the code more readable/idiomatic. But they cost something in transfer. I've never written/maintained a library, but I'd probably take the polyfill path (maybe with a separate file and build step).

Note that if people do try to polyfill these, it may break your code (you're iterating with for-in which visits the prototype). To work around this, either polyfill yourself :-p or add if(o.hasOwnProperty(prop)){...} verifications.