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

nucleotide-count: Change types to return an Either. #157

Closed
rbasso opened this issue Jun 20, 2016 · 8 comments
Closed

nucleotide-count: Change types to return an Either. #157

rbasso opened this issue Jun 20, 2016 · 8 comments

Comments

@rbasso
Copy link
Contributor

rbasso commented Jun 20, 2016

We are considering changing the interface of the problem nucleotide-count. The idea (#115) is to allow a more idiomatic solution to this problem, returning a Maybe, an Either or something else, instead of forcing the user to throw an error with the specific message: invalid nucleotide 'X'.

It could be something like this:

module DNA (count, nucleotideCounts) where

count :: Char -> String -> Maybe Int
nucleotideCounts :: String -> Maybe (Map Char Int)

This is just and sketch, and it can/should change. It depends only on the feedback here!

Any suggestion or comment is welcome! 😄

@abo64
Copy link
Contributor

abo64 commented Jun 21, 2016

+1 for not not throwing any unforced errors in Haskell.
But if you change the signature I would rather go for an Either String (Map Char Int).
This way you also stay closer to the original exercise.

@rbasso
Copy link
Contributor Author

rbasso commented Jun 22, 2016

In that case, what would Left String contain? An error message, the input String or just the invalid characters?

@abo64
Copy link
Contributor

abo64 commented Jun 22, 2016

Definitely the error message. I think this is also how most people use Either for error handling.

@rbasso
Copy link
Contributor Author

rbasso commented Jun 22, 2016

We don't need specify that. For the tests it probably would be enough to check if it is a Left.

@abo64
Copy link
Contributor

abo64 commented Jun 22, 2016

fine with me.

@petertseng
Copy link
Member

I'm assuming that if we'll have nucleotideCounts :: String -> Either String (Map Char Int) then we would also have count :: Char -> String -> Either String Int.

That seems reasonable to me (the initial suggestions of just the Maybe seem fine as well). Agreed that there's no need to test for a specific error message, just that it's Left.

@rbasso rbasso assigned rbasso and unassigned rbasso Jun 23, 2016
@rbasso rbasso changed the title What do you think about rewriting nucleotide-count to return a Maybe? nucleotide-count: Change types to return an Either. Jun 23, 2016
@rbasso rbasso removed the question label Jun 23, 2016
@samjonester
Copy link
Contributor

samjonester commented Jun 24, 2016

I agree with @abo64 that the original error string should be the Left. It feels like it would be an extremely useful message if I was to use these methods IRL.

@rbasso
Copy link
Contributor Author

rbasso commented Jun 28, 2016

If anyone is willing to write a PR, I will gladly review/merge it. 😄

Anyone?

samjonester added a commit to samjonester/xhaskell that referenced this issue Jun 29, 2016
By using an `Either` instead of throwing an error, the resulting exercise
is more idiomatic to Haskell. The `Either` will provide a useful error
message to end users of the functions.

Closes exercism#157
samjonester added a commit to samjonester/xhaskell that referenced this issue Jul 2, 2016
By using an `Either` instead of throwing an error, the resulting exercise
is more idiomatic to Haskell. The `Either` will provide a useful error
message to end users of the functions.

Closes exercism#157
samjonester added a commit to samjonester/xhaskell that referenced this issue Jul 2, 2016
By using an `Either` instead of throwing an error, the resulting exercise
is more idiomatic to Haskell. The `Either` will provide a useful error
message to end users of the functions.

Closes exercism#157
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

4 participants