two-fer: implementation of two-fer exercise in Kotlin #110
Conversation
|
In Gitter it was recommended to me by @NobbZ to also change the hello-world exercise to be updated to the new style of hello-world, (which simply asks the user to return the string "hello world!"), instead of the current implementation that requires string manipulation. This then allows string manipulation to be introduced to the user via the two-fer exercise that is implemented in this PR. |
|
Hi! Thanks for this contribution! I'll add review comments inline in a second, but I also wanted to post some information relevant to the |
| @@ -0,0 +1,10 @@ | |||
| fun twofer(name: String = ""): String { | |||
There was a problem hiding this comment.
Since we don't need to allow null input here, I'd say the closest interpretation of "no name given" would be invoking the twofer function with no parameter, a case that is currently not exercised by the tests. To implement that, we should probably update this signature to
fun twofer(name: String = "you"): Stringand have this exercise focus on a more typical use of default parameter values. Then the first test should mimic https://github.com/exercism/java/blob/master/exercises/two-fer/src/test/java/TwoferTest.java#L16-L22 but with no parameter passed instead of a null parameter passed.
The Java track debated what to do with an empty string input, and settled on being very literal about handling it: exercism/java#733. I'd say the same boundary case could be exercised here, since invocation with no parameters is the "right way" to call this function without specifying a name. Any explicit input can therefore be interpreted as something the caller wants to display as the name (even if empty). Does that make sense to fresh eyes?
| if(!matched) | ||
| tempName = "you" | ||
|
|
||
| return "One for $tempName, one for me." |
There was a problem hiding this comment.
In light of the above, this sample implementation can be reduced to a single line.
| @@ -0,0 +1,24 @@ | |||
| import org.junit.Ignore | |||
| import org.junit.Test | |||
| import org.junit.Assert.assertEquals | |||
There was a problem hiding this comment.
Just a note that https://kotlinlang.org/api/latest/kotlin.test/kotlin.test/assert-equals.html is probably available - I assume it's a wrapper around the JUnit assertion anyway though!
Changed model answer to better suit tests
|
Hey @stkent, I believe my latest commit has completed your requested changes? If there is something I am missing or more changes required please let me know. Thanks! 👍 |
ghost
mentioned this pull request
|
Looking at this tomorrow! |
| @@ -0,0 +1,3 @@ | |||
| fun twofer(name: String = "you"): String { | |||
| return "One for ${if (name.isBlank()) "you" else name}, one for me." | |||
There was a problem hiding this comment.
I think this can be altered to
return "One for $name, one for me."since supplying a blank string as input implies different intent to providing no input.
| @Ignore | ||
| fun emptyStringGiven() { | ||
| assertEquals("One for you, one for me.", twofer("")) | ||
| } |
There was a problem hiding this comment.
Let's mimic the Java tests and (1) have the expected string here be "One for , one for me.", and (2) move this test to the end of the suite.
| @Ignore | ||
| fun aNameGiven() { | ||
| assertEquals("One for Alice, one for me.", twofer("Alice")) | ||
| } |
There was a problem hiding this comment.
I believe this test should be moved above anotherNameGiven?
Corrected emptyStringTest to reflect changed code and re-ordered tests
ghost
deleted the
No description provided.