Skip to content

Conversation

@glennj
Copy link
Contributor

@glennj glennj commented Jan 29, 2022

The moves() function now returns an object with keys: moves, goalBucket, otherBucket.
This removes the need for goalBucket and otherBucket properties.
I also fixed the "bug" where moves() can return after the 2nd move if the second bucket is the same size as the goal (fixes #1551).

The moves() function now returns an object with keys: moves, goalBucket, otherBucket.
This removes the meed for goalBucket and otherBucket properties.
I also fixed the "bug" where moves() can return after the 2nd move if the second bucket is the same size as the goal.
@github-actions
Copy link
Contributor

Dear glennj

Thank you for contributing to the JavaScript track on Exercism! 💙
You will see some automated feedback below 🤖. It would be great if you can make sure your PR covers those points. This will save your reviewer some time and your change can be merged quicker.

  • ⬆️ The instructions and test cases for many practice exercises originate in the Exercism-wide problem-specifications repo. If the exercise you changed is listed there in the exercises folder, please consider the following.

    • Improvements to the instructions.md file should be made in the problem-spec repo so that all tracks can benefit.
      You can open a PR there instead.
    • If you want to add some language specific information, use the instructions.append.md file (see Practice Exercise Docs).
    • Test cases selected in the .meta/tests.toml file need to be implemented in the <exercise>.spec.js file according to the specification in canonical-data.json.
  • ✍️ If your PR is not related to an existing issue (and is not self-explaining like a typo fix), please make sure the description explains why the change you made is necessary.

  • 🔤 If your PR fixes an easy to identify typo, if would be great if you could check for that typo in the whole repo. For example, if you found Unicdoe, use "replace all" in your editor (or command line magic) to fix it consistently.

Dear Reviewer/Maintainer

  • 📏 Make sure you set the appropriate x:size label for the PR. (This also works after merging, in case you forgot about it.)

  • 🔍 Don't be too nit-picky. If the PR is a clear improvement compared to the status quo, it should be approved as clear signal this is good to be merged even if the minor comments you might have are not addressed by the contributor. Further improvement ideas can be captured in issues (if important enough) and implemented via additional PRs.

  • 🤔 After reviewing the diff in the "Files changed" section, take a moment to think about whether there are changes missing from the diff. Does something need to be adjusted in other places so the code or content stays consistent?

Automated comment created by PR Commenter 🤖.

Copy link
Member

@junedev junedev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up the issue!

I didn't look at the code in detail yet but noticed two things:

  • You mentioned before that the tests in problem spec for this exercise were updated so I was expecting this PR would include a change to the tests.toml file due to the new sync via configlet. 🤔
  • I saw you changed the output format for the exercise. I think it is definitely better then before.
    • I was wondering whether the stub really needs to contain a class definition with the new changes you made. Would it make sense that the stub is just a function (e.g. solve) that returns the result object you defined? Then the student would be free too choose what classes they think they need in the implementation. (You don't have to change anything if you don't want to but I would like to hear your thoughts.)
    • Whatever we decide for, the format of the input and expected output should be described in the instructions.append.md file so the student does not have to guess the format from the tests. You can also create an issue for this and it can be addressed later.

@glennj
Copy link
Contributor Author

glennj commented Jan 30, 2022

  • You mentioned before that the tests in problem spec for this exercise were updated so I was expecting this PR would include a change to the tests.toml file due to the new sync via configlet. 🤔

There was no changes necessary to tests.toml. This PR is actually reflecting the actual problem-spec: https://github.com/exercism/problem-specifications/blob/87cd20ff0b2f078d5eb2da716dc7d74fb71ea258/exercises/two-bucket/canonical-data.json#L85

  • I saw you changed the output format for the exercise. I think it is definitely better then before.

Based on @joshgoebel's suggestion. It certainly simplifies things.

  • I was wondering whether the stub really needs to contain a class definition with the new changes you made. Would it make sense that the stub is just a function (e.g. solve) that returns the result object you defined? Then the student would be free too choose what classes they think they need in the implementation. (You don't have to change anything if you don't want to but I would like to hear your thoughts.)

I don't have strong opinions about it. I think this problem naturally points to an OO solution, so I'm fine with what's there. I didn't want to make overly large changes to what was there already.

  • Whatever we decide for, the format of the input and expected output should be described in the instructions.append.md file so the student does not have to guess the format from the tests. You can also create an issue for this and it can be addressed later.

I see something similar for the satellite exercise. I'll add it to this PR.

@junedev
Copy link
Member

junedev commented Feb 1, 2022

I think this problem naturally points to an OO solution, so I'm fine with what's there.

@glennj Thanks for your feedback. Then let's keep it as is. One small thing though, what do you think about renaming the moves method to solve? Currently moves returns a result object which then has moves again as property. That seems a bit strange.

Copy link
Member

@junedev junedev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great improvement for the exercise, not only the bug regarding the tests but also the proper, document output format. Thanks a lot!

@junedev junedev added the x:size/medium Medium amount of work label Feb 1, 2022
@junedev junedev merged commit 8347091 into exercism:main Feb 1, 2022
@glennj glennj deleted the two-bucket-rework branch February 1, 2022 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

x:size/medium Medium amount of work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Two-Bucket has an incorrect test

2 participants