add circular buffer exercism#17
add circular buffer exercism#17wilmoore merged 1 commit intoexercism:masterfrom anthonygreen:circular-buffer
Conversation
|
Thanks for updating. I'll have a look. |
|
Updated pull request with some of the requested changes. |
There was a problem hiding this comment.
The parens are superfluous here and don't really add anything to the readability so we can probably nix them.
|
Ready for review |
There was a problem hiding this comment.
You still seem to have the superfluous anonymous function.
There was a problem hiding this comment.
That's expected. See Jasmine Matchers https://github.com/pivotal/jasmine/wiki/Matchers
|
Mainly typo fixes in these commits. |
|
Apologies @wilmoore Could you review again and suggest the appropriate changes. Thanks |
There was a problem hiding this comment.
The best way I know to explain it is with an example:
function call(fn) {
return fn();
}
function make_money() {
return "$5.00";
}
// superfluous
console.log("superfluous: %s", call(function () {
return make_money();
}));
// treating functions as first-class
console.log("first-class: %s", call(make_money));This will produce output of:
superfluous: $5.00
first-class: $5.00
Same result, but it is obvious which is less code, easier to read and reason about.
The fact of the matter is, whenever a function takes another function, it is doing so because it means to call that function. It doesn't matter which function you pass it, so long as when it is called, it does what you want. In the case of the code we are reviewing, buffer.read is referring to a function. If all you are doing in the anonymous function is calling just that function, you may as well just pass the function reference itself so it will get called directly rather than having to pass a wrapper function where you manually invoke it.
I hope this helps.
There was a problem hiding this comment.
That's expected. See Jasmine Matchers https://github.com/pivotal/jasmine/wiki/Matchers
That's mostly an issue with how they choose to document this case.
|
Ready for review |
The last pull request appears to have been deleted following an attempt to resolve a bad merge.
Here it is again
cc @wilmoore