Change Exercise is now ECMAScript ready by RobinCsl · Pull Request #362 · exercism/javascript

RobinCsl · 2017-10-22T18:05:54Z

No description provided.

matthewmorgan

@RobinCsl thanks for taking this on!

There are a few things we could do to make this more idiomatic ES6, and maybe then tighten up some other things .

Can you start with the comments I've left and we'll go from there? Thanks!

matthewmorgan · 2017-10-22T20:31:07Z

+    expect(result).toEqual([25]);
+  });
+
+  xtest('multiple coins coin', () => {


Should this read multiple coin change, as in the canonical file?

matthewmorgan · 2017-10-22T20:33:50Z

+    expect(result).toEqual([]);
+  });
+
+  xtest('test less than the smallest currency represented', () => {


Should this word currency instead be coin?

matthewmorgan · 2017-10-22T20:34:07Z

+    expect(test).toThrowError(Error, message);
+  });
+
+  xtest('test a large value that the currency cannot represent', () => {


Should this word currency instead be coin? (as above)

matthewmorgan · 2017-10-22T20:37:14Z

+  }
+}
+
+Change.prototype.calculate = (coinArray, target) => {


In ES6 we would want to see the Change class written something like this:

class Change { calculate(coinArray, target) { ... } }

The preferred pattern is like you have done for your Candidate class. The pattern of extending the prototype is an ES5 pattern, so we'll want to use the newer paradigm here.

matthewmorgan · 2017-10-22T20:40:01Z

+    const sum = candidate.getSum();
+
+    if (candidate.getSum() <= target &&
+            typeof (candidates[sum]) !== 'number' &&


I'm wondering if this wouldn't be more readable if the conditional were extracted into a named function instead?

Also, the indenting is a little unusual here. You could run the linter against your code in "fix mode" to perhaps take care of a lot of these things automatically. Let me know if I can provide some assistance on how to do that.

matthewmorgan · 2017-10-22T20:43:26Z

+  // is everthing searched?
+  const isDone = () => {
+    let done = true;
+    for (let i = 0; i < candidates.length; i += 1) {


Since you don't need the index i here, you might consider using the now available for-of instead, like so:

for (let candidate of candidates){ ... }

This would allow you to eliminate the indirection of using the index variable i and the temporary variable temp.

matthewmorgan · 2017-10-22T20:44:24Z

+
+  // get the next unsearched member of the candidate array
+  const getNext = () => {
+    for (let i = 0; i < candidates.length; i += 1) {


Same remarks about the for-of as above.

matthewmorgan · 2017-10-22T20:45:22Z

+
+      // copy the curent coins into it and add the new coin type
+      for (let i = 0; i < current.getCoins().length; i += 1) {
+        candidate.addCoin(current.getCoins()[i]);


Again, you have two loops that use index variables that you don't really need. Hit it with some ES6!!!

RobinCsl · 2017-10-24T20:12:30Z

Thank you very much for all your comments, they were really helpful and constructive. I hope that the code is clearer and better now. Let me know if there is anything else I should improve! :)
(Also, just so you know, I had this error when running npm run lint if there were for...of loops:

error  iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations  no-restricted-syntax

so I did without them, which actually produced more readable code in my opinion.)

matthewmorgan · 2017-10-25T23:33:32Z

+$ npm test
+```
+
+In many test suites all but the first test have been skipped.


Probably should change this to read "In the test suite, all tests but the first have been skipped." I'm not sure where this language came from, but I know it's present in many exercises.

matthewmorgan · 2017-10-25T23:34:38Z

Awesome! I'm glad you feel the review was helpful and the code is better for it.

I had one minor language change for the README if you don't mind, otherwise looks ready to merge.

RobinCsl · 2017-10-26T22:19:14Z

Just updated the README file :)

matthewmorgan · 2017-10-26T23:06:53Z

Great! Mind squashing to one commit? If you need assistance with this, I'm happy to help.

RobinCsl · 2017-10-29T14:13:17Z

I managed to squash the commits for this one too! I'll try again on #365!

rchavarria · 2017-10-29T15:52:15Z

@matthewmorgan approved the changes, and commits have been squashed. Merging...

Grouped all tests under a single describe Ordered the tests in a more logical way. All tests except the first one are skipped. Removed mention of enabling tests by changing xdescribe to describe. Implement exercise isbn-verifier (exercism#366) [Housekeeping] Upgrade jest (exercism#368) * Use `package-lock.json` instead of `yarn.lock` * Upgrade jest and babel-jest to latest versions Jest to 21.2.1 babel-jest to 21.2.0 Change Exercise is now ECMAScript ready (exercism#362) Transpose Exercise is ECMAScript ready (exercism#364) Upgrade jest Protein Translation is ECMAScript ready (exercism#365) (exercism#365) Update setup links in all READMEs (exercism#371) Update all setup links in exercise READMEs from http://exercism.io/languages/ecmascript to http://exercism.io/languages/ecmascript/installation Closes exercism#370 Updated setup link

Grouped all tests under a single describe Ordered the tests in a more logical way. All tests except the first one are skipped. Removed mention of enabling tests by changing xdescribe to describe. Implement exercise isbn-verifier (#366) [Housekeeping] Upgrade jest (#368) * Use `package-lock.json` instead of `yarn.lock` * Upgrade jest and babel-jest to latest versions Jest to 21.2.1 babel-jest to 21.2.0 Change Exercise is now ECMAScript ready (#362) Transpose Exercise is ECMAScript ready (#364) Upgrade jest Protein Translation is ECMAScript ready (#365) (#365) Update setup links in all READMEs (#371) Update all setup links in exercise READMEs from http://exercism.io/languages/ecmascript to http://exercism.io/languages/ecmascript/installation Closes #370 Updated setup link

Grouped all tests under a single describe Ordered the tests in a more logical way. All tests except the first one are skipped. Removed mention of enabling tests by changing xdescribe to describe. Implement exercise isbn-verifier (exercism#366) [Housekeeping] Upgrade jest (exercism#368) * Use `package-lock.json` instead of `yarn.lock` * Upgrade jest and babel-jest to latest versions Jest to 21.2.1 babel-jest to 21.2.0 Change Exercise is now ECMAScript ready (exercism#362) Transpose Exercise is ECMAScript ready (exercism#364) Upgrade jest Protein Translation is ECMAScript ready (exercism#365) (exercism#365) Update setup links in all READMEs (exercism#371) Update all setup links in exercise READMEs from http://exercism.io/languages/ecmascript to http://exercism.io/languages/ecmascript/installation Closes exercism#370 Updated setup link

matthewmorgan suggested changes Oct 22, 2017

View reviewed changes

matthewmorgan reviewed Oct 25, 2017

View reviewed changes

matthewmorgan approved these changes Oct 25, 2017

View reviewed changes

Change Exercise is now ECMAScript ready (exercism#362)

d8d6bf2

RobinCsl force-pushed the master branch from d584b5e to d8d6bf2 Compare October 29, 2017 14:11

rchavarria mentioned this pull request Oct 29, 2017

WIP: Implement exercise change #369

Closed

rchavarria merged commit ef5e51d into exercism:master Oct 29, 2017

rchavarria mentioned this pull request Oct 29, 2017

Protein translation #365

Merged

laurmurclar pushed a commit to laurmurclar/ecmascript that referenced this pull request Nov 4, 2017

Change Exercise is now ECMAScript ready (exercism#362)

768531e

Uh oh!

Conversation

RobinCsl commented Oct 22, 2017

Uh oh!

matthewmorgan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobinCsl commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewmorgan commented Oct 25, 2017

Uh oh!

RobinCsl commented Oct 26, 2017

Uh oh!

matthewmorgan commented Oct 26, 2017

Uh oh!

RobinCsl commented Oct 29, 2017

Uh oh!

rchavarria commented Oct 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants