-
-
Notifications
You must be signed in to change notification settings - Fork 556
reverse-string: Add new exercise #960
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
Conversation
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.
Also, please remove the .DS_Store
files, they do not belong into this repository.
exercises/reverse-string/README.md
Outdated
|
||
## Tutorial | ||
|
||
This exercise has two files: |
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.
This is to specific for a generic exercise description. My Erlang exercises won't have those 2 files, but a lot of other files.
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 agree. The tutorial shouldn't really be here, as it is JS specific.
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.
Ok I followed instructions from the specific JS PR 417 here to go over to this general exercise.
Once I'm "approved" of this exercise here, I thought to go back to the JS PR 417 to complete that.
How should I proceed?
"subject": "car", | ||
"candidates": ["girl", "Japan", "bear", "candy"], | ||
"expected": [] | ||
}, |
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 don't get this… What is "candidates"
for? And how can reversing a three letter long string result in an empty array?
"subject": "ant", | ||
"candidates": ["tna"], | ||
"expected": ["tan"] | ||
}, |
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 well, what is candidates for, and why do you expect an array of strings to be returned?
"property": "reverse-string", | ||
"subject": "NinJa", | ||
"candidates": ["ajnin"], | ||
"expected": ["ajnin"] |
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.
Why is the expectation lowercased? I haven't seen this requirement in the DESCRIPTION.md
Oh, and you need to rename README.md to description.md |
exercises/reverse-string/README.md
Outdated
- Run the test suite and make sure that it succeeds. | ||
- Submit your solution and check it at the website. | ||
|
||
If everything goes well, you will be ready to fetch your first real exercise. |
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 this line is copied from the hello-world exercise, and it should be removed I think.
exercises/reverse-string/README.md
Outdated
@@ -0,0 +1,106 @@ | |||
# Reverse String | |||
|
|||
The introductory exercise to arrays and strings. Just reverse input string. |
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 would rephrase this to lose the "arrays" bit, as that is a language-specific implementation detail. Maybe just focus on the actual exercise:
Given an input string, return its reversed string.
exercises/reverse-string/README.md
Outdated
|
||
## Tutorial | ||
|
||
This exercise has two files: |
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 agree. The tutorial shouldn't really be here, as it is JS specific.
@@ -0,0 +1,54 @@ | |||
{ | |||
"exercise": "revere-string", |
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.
typo here
@@ -0,0 +1,54 @@ | |||
{ | |||
"exercise": "revere-string", | |||
"version": "1.0.1", |
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.
first version should be 1.0.0
Made more edits. I'm not quite sure how to delete the _DS store files, I don't have it on my folder that I pushed changes. Where do I go about deleting those? |
They are there… Have you done Those files are probably not needed in any repository, as they are specific to your computer. Please create a global gitignore file, as described in https://gist.github.com/subfuzion/db7f57fff2fb6998a16c and add those store files there. A good start for a global gitignore on a mac is https://www.gitignore.io/api/osx |
Ok I had a typo when doing git rm, last commit shouldn't have it now. |
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've left some comments again.
Some of my former ones remain valid.
{ | ||
"description": "detects reverse-string", | ||
"property": "reverse-string", | ||
"input": "robot", "superman", "ramen", |
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.
This is not valid JSON.
Also I can't find a word in the desription.md that tells me that I have to handle more than a single string.
And it feels weird that this is the sole testcase…
I'd expect to have at least 3 testcases:
- reverse the empty string
- reverse a string consisting of a single lowercase letter
- reverse a string having more than a single lowercase letter
- reverse a string with at least a capital letter
- reverse a string that contains a proper sentence with punctuation.
- reverse a string twice and check if it is the original again (add a comment that this is perfect for poperty based testing)
- Run the test suite and make sure that it succeeds. | ||
- Submit your solution and check it at the website. | ||
|
||
## Tutorial |
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.
You can remove everything starting with ## Tutorial
up to the end of the file, since it is specific to JavaScript.
The tutorial stuff can be added by each tracks maintainer if they feel the need.
The section about how to run the tests and how to set up the environment (as a student) are generated automatically by each track.
My bad… This has to be And there is still a DS file left in |
@@ -0,0 +1,5 @@ | |||
--- | |||
blurb: "Given a string, reverse it’s letters and output a new string." |
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.
it's
must be its
.
Added 3 test cases, I'm not sure what the test cases with capitalization and punctuation are suppose to test that's why I didn't add them. I modeled my test cases off the hello world and leap programs since they are some of the beginner programs and they don't have much varied edge cases. Thoughts? Happy to make more edits. |
"version": "1.0.0", | ||
"cases": [ | ||
{ | ||
"description": "detects reverse-string", |
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 disagree with detects
. I don't think the function under test is detecting anything. It is reversing a string. The description should reflect that.
"description": "detects reverse-string twice and check if it's original string", | ||
"property": "reverse-string", | ||
"input": "robot", | ||
"expected": "robot" |
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.
nothing other than the description tells us that reverse is supposed to be applied to this string twice. If it is to be, the property
must be different.
"description": "detects empty string", | ||
"property": "reverse-string", | ||
"input": "", | ||
"expected": "String is empty, input valid string" |
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.
the reverse of ""
should be ""
. This string is only expected if the input is "gnirts dilav tupni ,ytpme si gnirtS"
"expected": "tobor" | ||
}, | ||
{ | ||
"description": "detects empty string", |
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 disagree with
detects
. I don't think the function under test is detecting anything. It is reversing a string. The description should reflect that.
same
"expected": "String is empty, input valid string" | ||
}, | ||
{ | ||
"description": "detects reverse-string twice and check if it's original string", |
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 disagree with
detects
. I don't think the function under test is detecting anything. It is reversing a string. The description should reflect that.
same
Tests should be in order, starting with the simplest case. Therefore I think, we should start with the empty string. Also in the spirit of TDD we should start slowly with a single letter string. Keeping it simple as So we increase the length to something where we actually see something happen. this is were the student usually starts to implement real logic. Now some random words to simply check if it was done right. Last but not least we do some tests with mixed case and punctuation simply to remind the student, that all strings are valid input. I've seen unnecessary verification logic in real world code which rejected valid input just because the examples the programmer had originally hadn't uppercase input (while uppercase was explicitely specified as valid)… |
Added more edits to test cases. As for reverse-string twice, not sure what a new property should be, so I just put new property called "reverse-string twice". Thoughts or edits? Thanks. |
}, | ||
{ | ||
"description": "reverse a string twice and check if it's original string", | ||
"property": "reverse-string twice", |
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.
What is this trying to test?
What code needs to be changed in a solution that would pass all preceding tests in order for it to pass this one?
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.
This was added on my suggestion, I suggested to add a comment/description as well, that this should be done through property testing like QuickCheck if available.
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.
Can such a test be described using our current json format?
I'm not sure what this is testing that hasn't already been tested.
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.
Maybe you are right. Seems as if I had shoot to fast after heaving heard a podcast about property based tasting lately ;)
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'm not entirely clear what I should be updating now if any, I'm still new to this exercism and open to making changes.
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.
Also, I'm not even sure how reversing a string twice is tested with this json format, is a person suppose to take an input string, call the function, take the output and put it into the input of the next function call? Below is code example.
reversestring("robot"); // -> tobor, reverse string test passed
reversestring("tobor"); // -> robot, reverse string twice test passed
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 we can simply put a comment in the “prelude” describing that this is a property that can be tested if such testing tools are available and remove this test completely.
"expected": "tobor" | ||
}, | ||
{ | ||
"description": "reverse a string consisting of a single lowercase letter", |
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.
Description does not match test.
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'm not sure what you mean, the "reverse a string" case with robot string or "reverse a string with a single lowercase letter" with string WOLf?
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.
a “string consisting of a single lowercase letter” has a length of 1, your string has a length of 4.
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.
Oh I see, I read the test as any string length as long as string has 1 single lowercase letter, not a string that ONLY has a single lowercase letter. If the only single letter case is more clear, then I can just update that.
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 don't think character case is something that needs to be tested in this exercise. Seems a superfluous detail.
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 I've explained in another comment, I've seen a lot of code that has arbitrary “validation” built in, because there were no examples with uppercase values…
Therefore I think, we should explicitely show, that any kind of input is to be expected.
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 it's OK to have some mixed case examples. I also think that this test suite spends too much time concentrating on case based tests.
A capitalised palindrome might be an interesting test.
"expected": "fLOW" | ||
}, | ||
{ | ||
"description": "reverse a string having more than a single lowercase letter", |
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.
This also describes the third test?
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'm not quite sure what you are asking. Descriptions should align to cases.
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.
When you are implementing a solution to this problem, you should be able to read the description and know from that what new aspect of the problem the test covers.
"a string having more than a single lowercase letter" has already been tested in the third test: "robot"
So either this test is redundant and should be removed because it is testing something that has already been tested.
OR
This test needs a description that describes why it needs to test this particular input.
Why is "DEssERTS" useful to test?
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.
Perhaps you have a good example in one of the exercises that clearly shows good JSON and description files. I want to make sure we are on same page and not talking in circles for this simple problem. I know I'm new to this, so this will take alittle longer the first time around.
"expected": "nemaR" | ||
}, | ||
{ | ||
"description": "reverse a string that contains a proper sentence with punctuation.", |
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.
This is also the first test that contains a space.
"expected": "" | ||
}, | ||
{ | ||
"description": "reverse a single character string", |
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.
"reverse a" is redundant and can be omitted from the descriptions.
Made more JSON and description file edits, happy to do more. If anybody has JSON and description files as examples so I can see what is good to commit, please let me know. |
"expected": "tobor" | ||
}, | ||
{ | ||
"description": "reverse a string with at least a capital letter", |
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.
Maybe shorten this to: "reverse a string with a capital letter"
@@ -0,0 +1,5 @@ | |||
Reverse a string | |||
|
|||
For Example: |
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.
Could you change "For Example" to "For example"?
"expected": "nemaR" | ||
}, | ||
{ | ||
"description": "reverse a string that contains a proper sentence with punctuation", |
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.
Maybe shorten this to "reverse a string with punctuation"
@ErikSchierboom ok in last 2 commits, I think those changes were implemented correctly. |
@eliaahadi I wasn't approving earlier because I agreed with the issues that @petertseng had raised. |
.gitignore
Outdated
@@ -1 +0,0 @@ | |||
node_modules |
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.
this line already existed. it should not be removed.
.gitignore
Outdated
@@ -1 +0,0 @@ | |||
node_modules |
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.
There is still a .gitignore
change here.
Are you checking the files that have changed?
Try running: git diff --name-only master
on your local branch.
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.
@Insti I don't think I added .gitignore to
case description updates commit, please let me know if I did.
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.
"expected": "" | ||
}, | ||
{ | ||
"description": "reverse a string", |
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.
remove "reverse"
what about "a word"
Why are you testing this string?
Is the fact it has symmetric o
s relevant?
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 like this suggestion.
"expected": "tobor" | ||
}, | ||
{ | ||
"description": "reverse a string with a capital letter", |
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.
remove "reverse a"
what about "a capitalized word"
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 like this suggestion.
"expected": "nemaR" | ||
}, | ||
{ | ||
"description": "reverse a string with punctuation", |
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.
remove "reverse a"
what about "a sentence with punctuation"
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 like this suggestion.
"expected": "!yrgnuh m'I" | ||
}, | ||
{ | ||
"description": "reverse a string and check if it is the same as input string (Palindrome)", |
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.
What about just "a palindrome"
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 like this suggestion.
@@ -0,0 +1,5 @@ | |||
Reverse a string |
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.
Short description is good 👍
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.
Agree, short and to the point!
exactly. This was already pointed out by me a month ago:
In a repositories Files specific to a certain IDE, an editor or an operating system belong to a global git ignore as described in the linked and quoted comment. |
Ok in last git push, I just edited and pushed the canonical-data.json file. I don't think I added .gitignore or the DS_store files. I even did a rm in command line to those files and looked to see if there were hidden. If they are still added, I'm missing something then to do to remove them. |
@eliaahadi The issue you're facing is that you've already committed an incorrect You need something like this: # add this first line only if you do not already have the exercism master
# configured as a remote named exercism
git remote add exercism [email protected]:exercism/problem-specifications.git
# once that's configured, you can
git checkout exercism/master -- .gitignore
git add .gitignore && git commit -m "Restore .gitignore"
git push [edit] |
@coriolinus thanks, I did just try that now from your instructions. It says
Thoughts? |
@eliaahadi See my edit to my previous response. |
@coriolinus ok I did that and also thought I was on the exercism/master by typing
I still get the error. I guess I'm still not on the exercism/master. Is git pull not the right way to get to exercism/master? |
"input": "I'm hungry!", | ||
"expected": "!yrgnuh m'I" | ||
}, | ||
{ | ||
"description": "reverse a string and check if it is the same as input string (Palindrome)", | ||
"description": "a string that reads the same backward as forward (i.e. palindrome)", |
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.
Why are you defining palindrome here?
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 don't think everybody knows what a palindrome is, and personally, I'd rather have it defined here than have to google it. If you feel strongly we don't need to define it, please state why you think everybody knows this. Perhaps wording can change some.
@eliaahadi However, I just realized we were going through all this over a file with one line in it. Using git is killing a mosquito with a sledgehammer. Do this: echo "node_modules" > .gitignore
git add .gitignore && git commit -m "Restore .gitignore"
git push It may be possible for things to go wrong if you do those commands in that order, but off the top of my head I can't think of how. |
To reset the
That should reset the |
@coriolinus I pushed the gitignore using those commands. Hopefully, it was done right. If there are any more edits in general, if not please let me know. |
@eliaahadi Very good, it worked! I believe that this solves the most pressing issue with this PR. Now it's a matter of the rest of the reviewers deciding whether or not all of their issues are now resolved. |
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.
Looking good to me!
it is no longer the case that gitignore got changed, and that was my objection, therefore my objection is dismissed.
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.
@eliaahadi thanks for your hard work and perseverance.
"expected": "!yrgnuh m'I" | ||
}, | ||
{ | ||
"description": "a string that reads the same backward as forward (i.e. palindrome)", |
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.
Continuing discussion from: #960 (comment)
It is helpful for descriptions to be short and to the point, so either
"a string that reads the same backwards and forwards"
or
"a palindrome"
but not both.
My preference is for "a palindrome", because that is the briefest description of what it is.
It's still possible to solve the problem without knowing the definition of the word and the definition can easily be found by a search if necessary.
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.
To give a concrete example of why concision is useful, consider the output of programs which generate tests based on the canonical data. Which is best:
fn test_a_palindrome() { ... }
fn test_a_string_that_reads_the_same_backwards_and_forwards() { ... }
fn test_a_string_that_reads_the_same_backward_as_forward_ie_palindrome() { ... }
Depending on which option gets merged, the Rust test generator will create one of the above. IMO, the shortest is the best.
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.
Ok I'm convinced. I added just "a palindrome" for description to latest commit.
Please review @Insti. Thanks.
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 we've finally got there.
Remember to squash and merge.
Merged! Thanks for the hard work @eliaahadi! |
Great thanks to all! I learned and glad I could help out. |
new exercise