-
-
Notifications
You must be signed in to change notification settings - Fork 89
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
Two-fer: Implemented two-fer as in #197. #200
Conversation
"slug": "two-fer", | ||
"core": false, | ||
"unlocked_by": null, | ||
"difficulty": 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 3 is reasonable. But can you put it somewhere near to the other ones of difficulty 3?
Because of #199 the placement doesn't need to be perfect, but at least it should be close...
exercises/two-fer/README.md
Outdated
If the given name is "Alice", the result should be "One for Alice, one for me." | ||
If no name is given, the result should be "One for you, one for me." | ||
|
||
## Test-Driven Development |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this section from the README.
That means from here up to but not including the line ## Instructions
exercises/two-fer/src/example.erl
Outdated
|
||
two_fer(Name) -> | ||
case Name of | ||
"" -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to treat the empty string as a "name" given, and to return "One for , one for me."
.
|
||
|
||
say_you_test() -> | ||
?assertEqual("One for you, one for me.", ?TESTED_MODULE:two_fer("")). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated above, I do personally treat the empty string as a name given, and like to have this test as ?TESTED_MODULE:two_fer()
.
We have the ability to create multiple functions with the same name but different arities, so we should use it.
exercises/two-fer/src/example.erl
Outdated
two_fer(Name) -> | ||
case Name of | ||
"" -> | ||
"One for you, one for me."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to recursively call with a proper value.
exercises/two-fer/src/example.erl
Outdated
"" -> | ||
"One for you, one for me."; | ||
_ -> | ||
"One for " ++ Name ++ ", one for me." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use lists:flatten/1
and io_lib:format/2
here.
The current version may cause funny errors when you pass something in that is not a string, while using the above mentioned fuctions will complain in a more human readable way.
exercises/two-fer/src/example.erl
Outdated
-export([two_fer/1, two_fer/0, test_version/0]). | ||
|
||
two_fer() -> | ||
lists:flatten("One for you, one for me."). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to flatten here. A literal string is as flat as it can be.
exercises/two-fer/src/two_fer.erl
Outdated
@@ -0,0 +1,8 @@ | |||
-module(two_fer). | |||
|
|||
-export([two_fer/0, test_version/0]). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add two_fer/1
as well.
Thank you @sjwarner-bp! 🎉 |
Fixes #197.