Echo JS 0.11.0

<~>
jylauril 3674 days ago. link 3 points
"I find this the clearest way to deal with optional args and I find the logic very easy to follow." I would have to argue this very strongly. It is neither clearest way, nor easy to follow.

The logic on your code is so volatile and error-prone that I figured I'd open up some parts here:

1. Your logic of testing the type of the argument doesn't consider the user giving wrong type of parameter into the function. Say, I give the first parameter as a number instead of a string, and your test would assume that I didn't give the name parameter at all, and now you'd add one more argument into the args array, which would shift the number to second place(index 1) of that args array. Now the next test would fail, even though I would have given an appropriate parameter for that position originally. Also, what happens if I give null as the name parameter, which seems to be what you're replacing the name with if it's not a string?

2. You're "re-constructing" the arguments array in reverse, so the values are not in the same order you expect in the array destructor. Unshift pushes an element in front of the array, so consider I only pass in a function as a parameter to that method: What happens now is that your first check would push a null value in front of the array. Now the function is on second place and null on first. Next you check if the second parameter is an array (it's not, it's a function), so you push the empty array in front of the array again.. so the order of the array is now: [deps, name, fn]. If I didn't pass in ANY parameters to that array, the order would be: [fn, deps, name]


Here's some test cases for you:
require(null, [], function(){});
-> results to args array of: [function(){}, [], null, null, [], function(){}]

require('foo', 'myDependency');
-> [function(){}, [], 'foo', 'myDependency']

require('foo', function(){});
-> [function(){}, [], 'foo', function(){}]



Over all, writing the "perfect" handling for arguments always depends on the function in question and it's parameters. At times it's important to verify that given parameters are correct (ie, when you're providing a method that a 3rd party developer might call), and sometimes it might be a bit redundant in private functions/methods, where the application logic ensures that the passed in parameters are always correct somewhere higher up in the call chain.

Best way to deal with arguments is to always use one single object as an argument. The benefits of this are huge compared to a pre-defined order of possibly optional parameters. This method forces the caller to name the parameters that are passed in, which eliminates the problem of accidentally giving the parameters in wrong order, etc.

This way of doing it also prepares you for future(and even present if you're smart!) by making it easy to transition into using Promises instead of callback functions. Of course there are exceptions to this where clearly separating the arguments is more beneficial than clumping them into one. The general rule I've used is that if I have more than two parameters, I'll rather use an object.

Replies

kolodny 3673 days ago. link 1 point
1. The tests could be a little stricter, what I have it just an example.

2. You're right, it should have been:

  // if name is omitted
  args.splice(0, 0, null);

  // if deps is omitted
  args.splice(1, 0, [])

  // if fn is ommited
  args.splice(2, 0, noop);

I'll edit the post when I get a chance