Skip to content

Commit

Permalink
resistor-color-duo: Change to value :: (Color, Color) -> Int
Browse files Browse the repository at this point in the history
TL;DR:
 - I don't think we should be teaching people to write functions like this.
 - The less controversial action would be to reject this PR in favor of one
   where either

        value :: [Color] -> Maybe Int
        value :: [Color] -> Either String Int
        value :: [Color] -> Either ColorError Int

Quick chronological recap of PRs that provide necessary context:

 - 2019-03-08: exercise created in exercism/problem-specifications#1466
 - 2019-03-12: exercise ported to Haskell track in #808
 - 2019-08-17: third-band test case and formulation "brown-green-violet should
   return 15 too, ignoring the third color." + one other similar formulation
   added to README.md in exercism/problem-specifications#1569.
 - 2019-10-01: third-band test case added to Haskell track in #849.

When this exercise was ported to Haskell track, it was decided to not address
error handling in this exercise via `Maybe Int` or similar, which otherwise
would be necessary since `value` is not defined for `[Color]` of length 0 or 1.

The alternative proposed here, to limit the input to exactly two bands, does
not appear to be mentioned.

In the meantime, the third-band test case (and formulations) have made `value`
explicitly more ambiguous: Unlike 0 or 1 bands, which is undefined, the third
band should be ignored, and >3 bands are undefined. Presumably they should be
ignored as well, but following this logic, should 0 or 1 bands also be ignored?
Why, exactly, are we creating a function so vulnerable to wrong input and not
let it handle validation gracefully?

README.md explicitly asks to disregard a third color in two places. Given this
proposal, those messages become meaningless; you can only specify two bands.

I can think of these ways to address the deficiency that this change causes:

 - Remove the lines from README.md, breaking a track invariant and CI check.
 - Apply a track-specific hint saying how this exercise is adapted to Haskell
   track in this way.
 - Don't change the description; confuse students. ("What third band?!")
 - Reject this PR in favor of another solution, as highlighted above.
 - Something else entirely.

The Read instance is removed from the stub as it isn't necessary.

This changes the version from 2.1.0.2 to 0.1.0.3, because the three-band test
case is removed, which makes the test suite no longer reflect canonical tests.
  • Loading branch information
sshine committed Oct 12, 2019
1 parent 799da03 commit c05e826
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,19 @@ data Color =
| Violet
| Grey
| White
deriving (Eq, Show, Read)
deriving (Eq, Show)

convert :: Color -> Int
convert Black = 0
convert Brown = 1
convert Red = 2
convert Orange = 3
convert Yellow = 4
convert Green = 5
convert Blue = 6
convert Violet = 7
convert Grey = 8
convert White = 9
value1 :: Color -> Int
value1 Black = 0
value1 Brown = 1
value1 Red = 2
value1 Orange = 3
value1 Yellow = 4
value1 Green = 5
value1 Blue = 6
value1 Violet = 7
value1 Grey = 8
value1 White = 9

value :: [Color] -> Int
value = read . concatMap (show . convert) . take 2
value :: (Color, Color) -> Int
value (a, b) = 10 * value1 a + value1 b
2 changes: 1 addition & 1 deletion exercises/resistor-color-duo/package.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: resistor-color-duo
version: 2.1.0.2
version: 0.1.0.3

dependencies:
- base
Expand Down
6 changes: 3 additions & 3 deletions exercises/resistor-color-duo/src/ResistorColors.hs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ data Color =
| Violet
| Grey
| White
deriving (Eq, Show, Read)
deriving (Eq, Show)

value :: [Color] -> Int
value cs = error "You need to implement this function."
value :: (Color, Color) -> Int
value (a, b) = error "You need to implement this function."
14 changes: 5 additions & 9 deletions exercises/resistor-color-duo/test/Tests.hs
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,25 @@ specs = describe "value" $ for_ cases test
assertion = value input `shouldBe` expected

data Case = Case { description :: String
, input :: [Color]
, input :: (Color, Color)
, expected :: Int
}

cases :: [Case]
cases = [ Case { description = "Brown and black"
, input = [Brown, Black]
, input = (Brown, Black)
, expected = 10
}
, Case { description = "Blue and grey"
, input = [Blue, Grey]
, input = (Blue, Grey)
, expected = 68
}
, Case { description = "Yellow and violet"
, input = [Yellow, Violet]
, input = (Yellow, Violet)
, expected = 47
}
, Case { description = "Orange and orange"
, input = [Orange, Orange]
, input = (Orange, Orange)
, expected = 33
}
, Case { description = "Ignore additional colors"
, input = [Green, Brown, Orange]
, expected = 51
}
]

0 comments on commit c05e826

Please sign in to comment.