Skip to content

dibs: I will implement exercise run-length-encoding #461

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

Merged
merged 4 commits into from
Dec 30, 2016

Conversation

ed359
Copy link
Contributor

@ed359 ed359 commented Dec 26, 2016

No description provided.

Copy link
Contributor

@rbasso rbasso 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 porting this exercise and sorry for making you wait that long.

The implementation is great! The only thing that I would like to see changed is removing the import in the stub solution.

I wrote a few additional comments, but fell free to change just what you want.

@@ -0,0 +1,9 @@
module RunLength (decode, encode) where

import Data.Char (isDigit)
Copy link
Contributor

@rbasso rbasso Dec 28, 2016

Choose a reason for hiding this comment

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

There are many ways to solve this problem without using isDigit, and it is not need for the type signatures.

This import would direct people to look for a specific solution, reducing diversity. I think that it should be removed.


-- Test cases adapted from file
-- `exercism/x-common/exercises/run-length-encoding/canonical-data.json`
-- on 2016-12-26.
Copy link
Contributor

Choose a reason for hiding this comment

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

This test suite could be better "translated" as a big do block, reproducing the exact test ordering from x-common.

That said, the cases ordering doesn't seem to be so important in this exercise, so I think it is great the way it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a conscious decision to change the order of the tests. I like having tests for different methods separated so that I can fully implement decode and test it before I work on encode. I prefer this to testing that both encode and decode work for the simplest case, and then another simple case, etc. I feel this workflow is nicer for beginners, especially since we stop the test on first failure by default. On the other hand, it's a pain to manually reorder things. I assume that in the long run the exercism project will implement an automated system for generating the tests from the common json files though that's another story.

decode = undefined

encode :: String -> String
encode = undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

This is perfect the way it is and also matches exactly what we did in all the other exercises.

Recently, we discussed in #459 that it would be nice to use error to give better messages than undefined, so an alternative would be:

encode xs  = error "You need to implement this function."

No change is needed here, and we are not yet in the processes of rewriting the stub solutions. Your choice!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If at a later date the decision is made to replace undefined with error (a decision that I'd support) the change can easily be implemented with a single find-replace.

@@ -23,6 +23,12 @@
]
},
{
"slug": "run-length-encoding",
"difficulty": 1,
"topics": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what is the best position for this exercise, so I think it is ok here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This depends. As it stands this is a very simple exercise, but if some of the more complicated error-handling and number-handling ideas (here exercism/problem-specifications#238) are implemented it would no longer be simple. I put it before rna-transcription because at the moment it only requires simple types and not Maybe for error handling.

@rbasso
Copy link
Contributor

rbasso commented Dec 28, 2016

Questions

  • How should the decode function behave when the input ends with a number?
  • How should the encode function behave when the input contains numbers?!

It seems that, using String to represent the encoded data, there is no simple way to tell the numbers from the characters...

Well...I'm digressing. This is a problem in the design of the exercise, not in the implementation, that seems correct. So this discussion should not prevent merging.

Any ideas about how to deal with that? @exercism/haskell

@abo64
Copy link
Contributor

abo64 commented Dec 28, 2016

I found something here.
So one could use negative numbers to indicate a non-encoded character sequence. Additionally one would need some separator like a blank space.

For example,
123AAA45 -> -3 123 3 A -2 45
(one could also encode number sequences: 111 -> 3 1)

But all this would of course complicate the exercise quite a bit - IMO in an interesting way! :)

@rbasso
Copy link
Contributor

rbasso commented Dec 28, 2016

Found this open issue in exercism/problem-specifications#238.

@ed359
Copy link
Contributor Author

ed359 commented Dec 29, 2016

I think a simple problem where we explicitly state that handling errors isn't necessary and that the alphabet we encode doesn't contain digits (though any other unicode is fine) is best here. But I'm happy to wait for the discussion to settle before this exercise gets added to haskell.

@rbasso rbasso merged commit f247ee4 into exercism:master Dec 30, 2016
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.

3 participants