Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change: Which, if any, input/coin combinations are errors? #1313

Closed
petertseng opened this issue Aug 31, 2018 · 4 comments
Closed

change: Which, if any, input/coin combinations are errors? #1313

petertseng opened this issue Aug 31, 2018 · 4 comments

Comments

@petertseng
Copy link
Member

petertseng commented Aug 31, 2018

Notice that https://github.com/exercism/problem-specifications/blob/master/exercises/change/canonical-data.json uses -1 to indicate "cannot make the target amount using the coins given". The question: Should this be considered an error? If so, change is covered by #1311.

I consider searching for a negative target an occurrence that an implementation should not be expected to handle. So I think using an error object is reasonable here. We can discuss implementations that should handle negative targets with negative coins, but I think it can be considered out of scope for the exercise. But you can see my previous comments #336 (comment) on it.

I consider searching for a non-negative but unattainable target an occurrence that the implementation should expect to routinely happen -- in a language with exceptions, for example, I would not recommend the use of an exception for this. A language with optionals would use its None for this case, and use Some(coins) when there actually is a coin combination.

I would normally refrain from commenting on whether this should be represented in JSON as a -1 sentinel or an error object or null.

But, as I say in #905 (comment) , I also wouldn't encourage languages with both optionals and errors (Rust's Option and Result, or Haskell's Maybe and Either) to use both of those. So in the target language, I'd suggest just using one.

So this leads me to think they should all be expressed as an error object.

In the absence of any other preference, I would suggest to say that all of these should be represented by an error object, so this should be handled as part of #1311. In that case, we can close this issue, for #1311 will remind us to change change's JSON appropriately.

@Insti
Copy link
Contributor

Insti commented Aug 31, 2018

    {
      "description": "no coins make 0 change",
      "property": "findFewestCoins",
      "input": {
        "coins": [1, 5, 10, 21, 25],
        "target": 0
      },
      "expected": []
    },
    {
      "description": "error testing for change smaller than the smallest of coins",
      "property": "findFewestCoins",
      "input": {
        "coins": [5, 10],
        "target": 3
      },
      "expected": -1
},

I don't understand why an error even needs to be considered here.
These cases should be equivalent.
The expected value should be an empty array.

I propose all the -1s be changed to empty arrays.

@NobbZ
Copy link
Member

NobbZ commented Aug 31, 2018

@Insti there is a clear distinction between those cases. In the first, we need none of the coins available to create a stack of value 0, while in the latter case, we can not reach the target value with the coins provided.

In my opinion, there should be an error in the latter case.

@petertseng
Copy link
Member Author

I propose all the -1s be changed to empty arrays.

I oppose. It is not possible to make target 3 with coins 5 and 10. It is possible to make target 0 (use no coins).

@Insti
Copy link
Contributor

Insti commented Sep 1, 2018

❤️ Thanks for clearing that up for me.
Also with my proposal it would not be possible to tell the difference between 'total is zero' and 'total is not possible'

proposal withdrawn.

petertseng added a commit that referenced this issue Sep 24, 2018
change 1.3.0

As proposed in
#1313
#905 (comment)

In contrast to the proposal in
#336 (comment)

Although -1 is a sentinel value, the sentinel value had been overloaded
in this JSON file to mean two separate conditions:

* "Can't make target" (ostensibly, we might be able to make the target
  with a different set of coins)
* "Target is negative" (no matter what coins we provide, this target is
  always invalid)

To make clear that these two conditions are different, we use an error
object describing each.

This error object was defined in
#401

Note that this commit is not a decree that all languages must represent
these conditions as errors; languages should continue to represent the
conditions in the usual way for that language. It is simply a
declaration that these two conditions bear enough consideration that
we'll represent them with a different type and also clearly
differentiate between the two.

Closes #1313
Checks the related box in #1311
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants