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

largest-series-product corner cases #89

Merged
merged 3 commits into from
Feb 21, 2016
Merged

largest-series-product corner cases #89

merged 3 commits into from
Feb 21, 2016

Conversation

petertseng
Copy link
Member

As part of the work to have consistency between tracks as in https://github.com/exercism/todo/issues/104 and https://github.com/exercism/todo/issues/13. Comments are in individual commit messages.

@petertseng
Copy link
Member Author

Ah, that's unfortunate. Tests are failing on -Werror because of those Nothings. Going to think about how to deal with that

@petertseng
Copy link
Member Author

Yeah ran locally with -Werror to check, this should be good now. had to add a Nothing that was explicitly Maybe Int

@petertseng
Copy link
Member Author

Going to change that int to be Int -> Maybe Int to avoid all the Just business going on here, I think.

The reasoning behind this is that there are zero ways to make a
4-character string out of "123", so to find the max product is to take
the max of an empty list, which is an error.

(Compare with the case of taking a 0-character string, of which there is
one, so that case is not an error)

These are consistent with the values defined in
https://github.com/exercism/x-common/blob/master/largest-series-product.json
It is prudent to ensure that every combination of edge/non-edge are
tested for the two inputs.
This is distinct from the existing case because there are nonzero digits
in this string as well.
@petertseng
Copy link
Member Author

OK, that's better. Much less churn doing it this way. I saved the old way of doing it at https://github.com/exercism/xhaskell/compare/master...petertseng:lsp-corner-nojust?expand=1 but I think the way it is in this PR is better.

@kytrinyx
Copy link
Member

I'm not sure who's around these days who knows Haskell. It looks like @abo64, @NobbZ and @stevejb71 have all commented recently in the Haskell track... would one of you have a moment to review this?

@NobbZ
Copy link
Member

NobbZ commented Jan 29, 2016

I'll take a closer look into this in the next 36 hours.

Katrina Owen notifications@github.com schrieb am Fr., 29. Jan. 2016 19:58:

I'm not sure who's around these days who knows Haskell. It looks like
@abo64 https://github.com/abo64, @NobbZ https://github.com/NobbZ and
@stevejb71 https://github.com/stevejb71 have all commented recently in
the Haskell track... would one of you have a moment to review this?


Reply to this email directly or view it on GitHub
#89 (comment).

@NobbZ
Copy link
Member

NobbZ commented Feb 2, 2016

As far as I can see, the code in the example is idiomatic, the choose of Maybe is reasonable. Only thing I fear is, that someone new to this exercise might fail miserably because he has to return Just x instead of just x ;) I am not aware of how often Maybe is used in the exercises before this already.

Also all the added tests (and the changes to make old ones pass with new code) seem reasonable.

@petertseng
Copy link
Member Author

Reasonble question. I originally tried to answer this question by seeing what has Maybe in it

$ git grep -l Maybe | cut -d/ -f1 | uniq
_test
atbash-cipher
bank-account
binary-search-tree
binary
connect
go-counting
hexadecimal
largest-series-product
linked-list
pov
queen-attack
saddle-points
say
scrabble-score
sgf-parsing
wordy
zipper

If someone follows the order in config.js, then the ones that come before: scrabble-score, binary, atbash-cipher, bank-account, queen-attack, binary-search-tree, hexadecimal

However, I actually realized that it's not necessarily the case that the tests are testing for Maybe -- it could just be an implementation detail of the examples. Indeed, this is true for a lot of them. So I manually look through the list and see which of them have tests involving Maybe (which means the student would have to write code involving Maybe)

  • The queen-attack has a test where it passes in Just a board position or Nothing.
  • The binary-search-tree problem needs to have a bstLeft and bstRight that return Maybe (BST a).
  • bank-account needs Maybe Int for its bank balance. It also seems to require using the IO monad, an interesting choice.

So someone on this exercise either has seen Maybe in a previous exercise (or is skipping around, but in that case they on their own anyway). So I think the specific question of "will someone be confused at the Maybe in here?" is "I think we are OK"

Separately, we may consider somewhere to discuss general questions that rise from the above comment. Possible questions I can think of:

  • Should we have exercises designed to ease learners of the language into various concepts of the language? In other words, to what extent shall Exercism try to act as an explicit language tutorial of sorts (rather than just providing an impetus to apply what is learned from an external tutorial)?
  • Should there be a limit to the number of new language concepts introduced in each problem?
  • To what extent should our test suites test "the fiddly bits" (dealing with errors, etc.) rather than the algorithmic "meat" of the problem?

@petertseng
Copy link
Member Author

Y'all think anything else needs to be done here? Based on the investigation above, I think the Maybe thing is covered earlier so will not be an obstacle, and y'all seem to think everything else is good.

@NobbZ
Copy link
Member

NobbZ commented Feb 19, 2016

I do also think that this can be merged. @kytrinyx

@kytrinyx
Copy link
Member

Thanks, merging!

kytrinyx added a commit that referenced this pull request Feb 21, 2016
largest-series-product corner cases
@kytrinyx kytrinyx merged commit 5ee7d80 into exercism:master Feb 21, 2016
@petertseng petertseng deleted the lsp-corner branch February 21, 2016 19:38
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

Successfully merging this pull request may close these issues.

None yet

3 participants