Also accept a string as an argument

[dpashkevich]

This would be a little extra exercise in API building

[dpashkevich]

Any feedback on this one?

[kytrinyx]

@wilmoore, @theotherzach - would you mind taking a look at this?

[theotherzach]

I would argue against this test. It forces people to create an additional check in the match method for minimal gain.

[theotherzach]

OK, this bugged me for the first time right now:

var detector = new Anagram("ant");

This is how all the tests are setup so it's not specific to this pull or JavaScript. I find it interesting that we have a separate concept for the class and an instance of the class. An Anagram creates a detector. Or maybe they're both an AnagramDetector implicitly. Sorry, just noticed this for the first time.

[kytrinyx]

Sorry, just noticed this for the first time.

Yeah, it's the same in Ruby right now.

I've gone back and forth on this. In the Ruby test suites I'm tempted to make fewer and fewer choices.

Like, perhaps Anagrams.detect('ant', ['tan', 'stand', 'at']) or something like that. Then people can make instances if they want, or not.

[wilmoore]

This would be a little extra exercise in API building

Probably a good idea because I'd like to refactor this, but I'd be happy to see someone else take a crack at refactoring it as well.

The extra API sugar is just a nice incentive to get the refactoring going which I am OK with.

[wilmoore]

As a start; we can probably drop the constructor.

[wilmoore]

Probably something like this (but probably should break out the string transforms into a function):

module.exports = function anagram(word) {
  return {
    matches: matches.bind(this, word)
  };
}

function matches(word, words) {
  words = Array.isArray(words) ? words : [words];

  return words.filter(function (candidate) {
    return candidate.toLowerCase().split("").sort().toString() === word.toLowerCase().split("").sort().toString();
  });
}

[wilmoore]

Test would look something like:

var anagram = require('./anagram');

var ant = anagram('ant');

assert(ant.matches(["tan", "stand", "at"]).length); // translate this to a jasmine matcher of course.

[dpashkevich]

I would argue against this test. It forces people to create an additional check in the match method for minimal gain.

Many real world libraries and frameworks are flexible about the arguments in their functions which makes using them convenient in different scenarios. I would also add a version that accepts a variable number of arguments, e.g. ant.matches('tan', 'stand', 'at'). I think that kind of API would be beautiful.

As a start; we can probably lose the constructor.

Why objections against the constructor? The language has this feature, and while you're not required to use it, JavaScript does allow one to do classic OOP.

[wilmoore]

Why objections against the constructor?

They have their pros and cons (just like everything else); however, I try to only use them when they are absolutely necessary. The object graph (i.e. prototype chain) becomes much more complex when you introduce a constructor; and in a lot of cases, they aren't needed.

If the intention is to always mimic the class-based oo style, then obviously using the constructor is the way to go :)

[wilmoore]

A great article to read in general but also very relevant to this discussion:
http://davidwalsh.name/javascript-objects-deconstruction

[kytrinyx]

If the intention is to always mimic the class-based oo style

Hardly :)

[kytrinyx]

Merged in #13. Thanks!

[dpashkevich]

@wilmoore thanks for your comments and the article link, that makes sense!

[wilmoore]

@dpashkevich: no problem at all 👍

[derekprior]

This is super awkward to do in JavaScript. It also happens to be slow (relatively speaking, of course).

It feels like its likely to make the developer think, "Oh, yeah. I gotta do that slice thing with arguments" or, "What? I guess I'll google this." It's cryptic, often-cargo-culted code.

[wilmoore]

This is super awkward to do in JavaScript.

@derekprior

Please expound. We have no way of knowing what your definition of awkward is :)

It also happens to be slow (relatively speaking, of course).

Perhaps...but, then again, what do you mean by slow?

• How much slower?
• How slow does it need to be before it is unacceptable?
• Is the code in question in a hot-spot? Is it repeated hundreds of times in a loop?

Just trying to get to the bottom of your definition of slow and if it is a necessary optimization or not. Keep in mind, without any parameters around requirements and no discussion of trade-offs, I'm not sure what to do with your claim.

If you can provide some further detail, we can begin to have a discussion about it.

[derekprior]

We have no way of knowing what your definition of awkward is :)

Let's look at the code: Array.prototype.slice.call(arguments, 0) or, if you want to be a little cuter: [].slice.call(arguments, 0).

arguments looks like an array but its not. But we really need it to be an array. So what do we do? We access the slice function from Array's prototype and use call to call that on... the thing we already established wasn't an array. Sounds like a pretty awkward dance to me.

Additionally, the code doesn't pass the smell test. If you are an experienced programmer from another language there's very little chance you'd grok without some serious "WTF?" and googling. It's one of the many sharp edges of JavaScript.

what do you mean by slow?

The MDN Documentation offers this -- laid out in a bright red box so as to get your attention:

You should not slice on arguments because it prevents optimizations in JavaScript engines (V8 for example).

One could use a for loop instead, which performs much better but also increases the amount of code needed to complete this exercise -- which is only related to this entire arguments discussion because of the test case added in this PR.

Due to these issues, I'd argue that this isn't good API design in JavaScript. You should take an array (or somethinng that responds to the array methods you require (likely filter here) or, with ES6, a rest parameter.

[wilmoore]

Sounds like a pretty awkward dance to me...It's one of the many sharp edges of JavaScript.

I agree, it sucks 👍

That being said, it is an idiomatic technique and it's what we've got to work with.

there's very little chance you'd grok without some serious "WTF?" and googling

I agree; however, googling is simply part of the job. Even for an experienced programmer.

laid out in a bright red box so as to get your attention ... You should not slice on arguments because it prevents optimizations in JavaScript engines

Even when things are written in a big red box, we still have to apply our own justifications and check trade-offs. Still, this warning comes with no benchmarks and a short article such as that will never go into trade-offs, etc. It's a warning...yes, you should consider it, but it does not mean you should never use it. Though, I do agree that Array.slice(arguments); would be a fine change.

Due to these issues, I'd argue that this isn't good API design in JavaScript

Depends on the situation. In this case, I'd argue that convenience outweighs the anecdotal WTF argument (pun intended).

Anyway, it's better to have a little fun with this stuff. Let's not be so uptight about this sort of thing unless it is (1) causing major pain without question (2) simply does not work.

Again, more fun, less serious 😄

[wilmoore]

If the above doesn't leave you satisfied for some reason, here is a little more food for thought if you really want to dig into this further:

• Managing arguments A much more reasoned approach to optimizing arguments to array. Includes trade-offs, etc.
• Unorthodox Performance

You've probably just spent more time reading about fast array slicing than the time all your array slicing code will take, cumulatively on all of your code's future users -- Paul Irish

http://www.reddit.com/r/javascript/comments/2eppid/fastest_way_to_slice_arguments_in_javascript/ck24udm

[derekprior]

👍