Twofer exercise #520

AvasDream · 2018-04-25T08:02:48Z

Hello, i created my first exercise and would really appreciate your feedback.

coriolinus

Hi @AvasDream, thanks for contributing this! There are a few points which need some improvement, but overall, this looks pretty good.

coriolinus · 2018-04-25T18:37:34Z

exercises/twofer/Cargo.toml

@@ -0,0 +1,5 @@
+[package]
+name = "twofer"
+version = "0.1.0"


This should match the version in canonical-data.json.

coriolinus · 2018-04-25T18:38:43Z

exercises/twofer/example.rs

@@ -0,0 +1,6 @@
+pub fn twofer(name: &str)-> String {
+    return match name {


return is unnecessary here.

coriolinus · 2018-04-25T18:40:34Z

exercises/twofer/src/lib.rs

@@ -0,0 +1,3 @@
+pub fn twofer(_name: &str)-> String {


Instead of using an underscore prefix for the variable, try to fit it into the unimplemented statement. It's fine to get cute with it: unimplemented!("One for {}, one for me.", name).

coriolinus · 2018-04-25T18:42:39Z

config.json

+      "difficulty": 1,
+      "topics": [
+        "Strings",
+		"match"


I'd suggest using the topic "string interpolation" here instead of "match": solutions will almost certainly use string interpolation, but I'd expect a good portion of implementers to use an if statement to solve this instead of a match.

This is a suggestion, not a requirement; feel free to ignore if desired.

petertseng

I'd like to make explicit a decision that has been implicitly made here:

Given that this exercise is to be placed early in the track, is it too early of a point to introduce Option?

If it is too early, no change to be taken in response to this comment. (Or maybe a code comment explaining that "" is not the best way to represent the absence of a string, but better ways will be shown in later exercises?)

If it is not too early, the exercise should use it.

coriolinus · 2018-04-25T21:00:47Z

I thought about that. To my mind, using an Option here actually increases the complexity: given Some(""), what should be returned?

…

On Wed, Apr 25, 2018, 22:36 Peter Tseng ***@***.***> wrote: ***@***.**** commented on this pull request. I'd like to make explicit a decision that has been implicitly made here: Given that this exercise is to be placed early in the track, is it too early of a point to introduce Option? If it is too early, no change to be taken in response to this comment. (Or maybe a code comment explaining that "" is not the best way to represent the absence of a string, but better ways will be shown in later exercises?) If it is not too early, the exercise should use it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#520 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHdeTsjz-a1vgH6oOPFJMc5Ro-jWB8Swks5tsN5jgaJpZM4Ti9EV> .

petertseng · 2018-04-30T08:34:50Z

given Some(""), what should be returned?

I yield on that point, because while I personally have no qualms with saying that the answer should be "One for , one for me", the point of your asking that question is to point out that the API incites such questions, and is not to ask what the exact answer to the question is.

I mentioned that I didn't like Some/None in our hello-world in exercism/problem-specifications#290 (comment). I would say the same thing here.

Okay, in that case my only request for us keeping this exercise (I'd also like to explicitly call out the option of simply not having this exercise in this track) is one of the two following:

have a comment along the lines of saying that there are other ways in Rust to indicate the absence of a value rather than using a sentinel value, but those ways will not be covered in this exercise for simplicity
explain to me that such a comment is actually not necessary for certain reasons (don't confuse the student with unnecessary information, etc., I'll let the person who explains it to me come up with the reason why it's not necessary)

coriolinus · 2018-04-30T08:45:09Z

Right, that makes sense to me. I think that, were I to write the comment, I'd structure it as an analogy: for some functions operating on numbers, 0 is outside the domain of the function, and can safely be used as a null value. For others, 0 is within the domain, so Option _must_ be used if null values are possible. Even in the former case, it's still better practice to use an Option to indicate the null value, but it's common to see this in code translated from languages which do not possess monadic types. Likewise, there are two categories of functions operating on strings: those for which the empty string is within the domain of the function, and those for which it is not. Twofer is an example of the latter. Even though it's better practice to use an Option, we are intentionally structuring the API to avoid that requirement to ease the burden on new learners.

…

On Mon, Apr 30, 2018 at 10:34 AM, Peter Tseng ***@***.***> wrote: given Some(""), what should be returned? I yield on that point, because while *I personally* have no qualms with saying that the answer should be "One for , one for me", the point of your asking that question is to point out that the API incites such questions, and is not to ask what the exact answer to the question is. I mentioned that I didn't like Some/None in our hello-world in exercism/problem-specifications#290 (comment) <exercism/problem-specifications#290 (comment)>. I would say the same thing here. Okay, in that case my only request for us keeping this exercise (I'd also like to explicitly call out the option of simply not having this exercise in this track) is one of the two following: - have a comment along the lines of saying that there are other ways in Rust to indicate the absence of a value rather than using a sentinel value, but those ways will not be covered in this exercise for simplicity - explain to me that such a comment is actually not necessary for certain reasons (don't confuse the student with unnecessary information, etc., I'll let the person who explains it to me come up with the reason why it's not necessary) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#520 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHdeTgNb1ibv6xTsTAlyibwHzMA28RNsks5ttsyrgaJpZM4Ti9EV> .

AvasDream · 2018-05-06T13:35:18Z

Hi, i made the requested changes.
I personally thought about this exercise as some kind of second "hello world" and introduction to pattern matching. In my opinion the Result/Some stuff is a more advanced concept and covered in later exercises.

coriolinus

Thanks @AvasDream for submitting this exercise!

I will now let it sit for about a week to ensure that other maintainers have a chance to take a look. If no issues come up in that period, I intend to merge this on 11 May; if on the 12th I still have not, please feel free to ping me a reminder.

I note that there is still no explicit discussion of the design choice to assign special meaning to the empty input string. Given that @petertseng has already specifically requested that, it would not surprise me if that became a blocking issue. However, I do not presume to speak for him.

ijanos

I'm conflicted. The empty input string as a special case does not feel idiomatic rust for me. Without it there is no reason to use pattern matching though.

I think we can accept this PR in it current form, then later (yeah I know...) re-think the introduction exercise list and make adjustments if we see a bigger picture of how concepts are introduced.

coriolinus · 2018-05-09T09:54:47Z

@AvasDream We're performing some track maintenance in #528, and it is likely that this is going to require some action from you through no fault of your own. Sorry about that.

There are two possible ways to ensure that this PR still passes Travis once #528 is merged:

Manually edit your entry in config.json to ensure that it conforms to the ordering specified in Run configlet fmt on all tracks meta#95 (comment) and enforced by configlet fmt #528 (example).
Via configlet:
1. Update configlet; the necessary feature is in version 3.9.0 and higher
2. Run configlet fmt . from the track root
3. Commit its changes to your branch
Note that if you choose this method, then before configlet fmt #528 is merged, it will produce large unrelated diffs in config.json and config/maintainers.json. Those are expected, and should vanish once configlet fmt #528 is merged.

petertseng · 2018-05-13T01:29:28Z

exercises/twofer/tests/two-fer.rs

+
+#[test]
+fn empty_string() {
+    assert_eq!(twofer(""), "One for you, one for me.");


explain to me that such a comment is actually not necessary for certain reasons (don't confuse the student with unnecessary information, etc., I'll let the person who explains it to me come up with the reason why it's not necessary)

I will consider the following part of #520 (comment) to comprise this explanation:

thought about this exercise as some kind of second "hello world" and introduction to pattern matching. In my opinion the Result/Some stuff is a more advanced concept and covered in later exercises.

I counter that it is still possible to say as a comment in this test "using the empty string as a sentinel value is not preferred in Rust since we have better ways to express the absence of a value, but we will learn those ways later in the track" without having to specify what those alternative ways are.

I cannot comment on what is the best way to teach students these concepts; I would have to spent time on improving my skills and knowledge before I can effectively spend time to think about and comment on how to teach.

Thus, it does not make sense for me to require any changes before this PR should be merged.

coriolinus · 2018-05-18T13:51:59Z

@AvasDream I've jumped in and made the necessary changes to config.json by merging master and then applying configlet fmt .. I was holding off from merging until those changes happened: if I'd merged without those changes, Travis would fail for everyone else until someone went in and made them. I got sick of waiting, so I did it myself. Hope you don't mind.

I intend to merge as soon as Travis reports green status.

Vincent Grimmeisen added 2 commits April 25, 2018 09:58

added twofer exercise

c895576

Added twofer to config.json

4603799

coriolinus suggested changes Apr 25, 2018

View reviewed changes

petertseng reviewed Apr 25, 2018

View reviewed changes

coriolinus mentioned this pull request Apr 30, 2018

Twofer Exercise #522

Closed

Requested changes.Same Version in all files. removed underscore prefix.

32d3708

coriolinus approved these changes May 7, 2018

View reviewed changes

ijanos approved these changes May 7, 2018

View reviewed changes

petertseng reviewed May 13, 2018

View reviewed changes

coriolinus added 2 commits May 18, 2018 15:46

Merge branch 'master' into twofer

878f73a

Apply 'configlet fmt'

6b09683

coriolinus merged commit fb4c277 into exercism:master May 18, 2018

coriolinus mentioned this pull request Jun 18, 2018

twofer: Added escape characters test #548

Closed

ZapAnton mentioned this pull request Jun 19, 2018

Should we remove the twofer exercise entirely? #552

Closed

petertseng mentioned this pull request Mar 15, 2019

If Haskell were to have two-fer and it MUST immediately follow hello-world, what would it look like? exercism/haskell#813

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Twofer exercise #520

Twofer exercise #520

AvasDream commented Apr 25, 2018

coriolinus left a comment

coriolinus Apr 25, 2018

coriolinus Apr 25, 2018

coriolinus Apr 25, 2018

coriolinus Apr 25, 2018

coriolinus Apr 25, 2018

petertseng left a comment

coriolinus commented Apr 25, 2018 via email

petertseng commented Apr 30, 2018

coriolinus commented Apr 30, 2018 via email

AvasDream commented May 6, 2018

coriolinus left a comment

ijanos left a comment

coriolinus commented May 9, 2018

petertseng May 13, 2018

coriolinus commented May 18, 2018

		@@ -0,0 +1,6 @@
		pub fn twofer(name: &str)-> String {
		return match name {

Twofer exercise #520

Twofer exercise #520

Conversation

AvasDream commented Apr 25, 2018

coriolinus left a comment

Choose a reason for hiding this comment

coriolinus Apr 25, 2018

Choose a reason for hiding this comment

coriolinus Apr 25, 2018

Choose a reason for hiding this comment

coriolinus Apr 25, 2018

Choose a reason for hiding this comment

coriolinus Apr 25, 2018

Choose a reason for hiding this comment

coriolinus Apr 25, 2018

Choose a reason for hiding this comment

petertseng left a comment

Choose a reason for hiding this comment

coriolinus commented Apr 25, 2018 via email

petertseng commented Apr 30, 2018

coriolinus commented Apr 30, 2018 via email

AvasDream commented May 6, 2018

coriolinus left a comment

Choose a reason for hiding this comment

ijanos left a comment

Choose a reason for hiding this comment

coriolinus commented May 9, 2018

petertseng May 13, 2018

Choose a reason for hiding this comment

coriolinus commented May 18, 2018