reverse string exercise

[eliaahadi]

I added new reverse string exercise, I'm happy to edit it.

[matthewmorgan]

@eliaahadi my earlier comments on this PR seem to have mysteriously disappeared.
I had not noticed that there are in fact two PRs (this and #419) for this exercise. That explains the missing comments. @eliaahadi please close either this PR or #417 to prevent confusion-- just keep one open. Thanks!=

I did see a comment from you last night asking how to move forward on this PR, and I think I had not explained myself clearly. At the risk of repeating myself and boring you, here is what I was trying to say:

1. We need to get permission from the author of the article on Medium to reproduce her work here. I know there have been cases in the past where authors did not want their work used. If this is in fact all your original work, simply based on an idea she presented, maybe it's OK but it would still be polite to get her blessing. @kytrinyx would you care to comment further on this? I don't know what other considerations you might have.

2. We don't normally add new exercises to an individual language track unless they're first added to the canonical problem specifications repo, here: https://github.com/exercism/problem-specifications

The process of adding a new exercise normally involves specifying:

• a README
• canonical test data
• metadata
  You've provided the first in this PR, so that work could easily be moved over to a PR on the problem-specifications repository.

If we can get permission on the first point, then it would make sense to open a PR in the problem-specifications repo to add the exercise generally to exercism. Then we can accept a PR to add it specifically to this language.

Does that make sense? Please let me know if I'm being unclear in any way.

[eliaahadi]

@matthewmorgan this is a common interview question, alittle more difficult than hello world and has many solutions on the internet. I just googled a random solution but if you feel like we need to confirm it with that specific author, I can try reaching out to her for her permission or I can just find another article and author that may be easier to reach.

I also have those files you mentioned, I tried to submit it to this PR but was advised that I don't need them at the moment.

[matthewmorgan]

@eliaahadi I understand that it's a common question, I agree with that. I feel like I'm doing an ineffective job of communicating why we can't move forward on this PR yet, so let me try again:

A. You cite the specific article from an author on Medium in the README. Does your PR contain original work from someone other than yourself that you haven't cited? If it does, you need to remove that, or get permission from the author. If, on the other hand, all you are doing is mentioning that she wrote an article that gave you an idea, and that you're not actually using her work, then everything is OK and it's fine to mention the article.

B. Your contribution would be a completely new exercise that has never been implemented on exercism.io. Because of that, it will first need to be added to the problem-specifications repo. When that PR is approved and merged, we can then accept this PR for an implementation of the problem in this track.

Please take the following steps:

1. Submit a PR with the three files I mentioned above in this repo.
2. When the PR in problem-specifications is approved, we can continue with this one.

We are happy to have your contribution, but this is our process.

Is that clear, or is there anything else I can explain?

[eliaahadi]

Ok thanks for the information.

For A, this PR doesn't contain any original work, the article I referred to just shows various ways to solve the problem. I'd think it's just fine to mention the article.

For B, I did now create a new PR referred here at #960. It looks like that PR checks have already failed with error "The Travis CI build failed". I'm not quite sure how to debug.

Thoughts?

[matthewmorgan]

OK @eliaahadi I'm glad we're on the same page now!

I am not a maintainer on the problem-specification repo, but I am happy to take a look and see if I can understand why the build is failing.

[matthewmorgan]

@eliaahadi When I look at the build log at line 483, I see an error that is helpful.

Apparently for the problem-specifications repo you need to call your README.md file by a different name: description.md.

Also, I noticed that your description file contains information about how to run tests etc that is specific to the JavaScript track. You don't want that in the generic description.md as your contribution will be used across all tracks when accepted.

I would suggest checking the contributor guide for instructions, and if you don't find the information clear, you should ask on that PR, so one of the maintainers can help. That's what I would do if I were making a PR there!

[kytrinyx]

It sounds like you've gotten things mostly straightened out.

We have some documentation about adding brand new exercises: https://github.com/exercism/docs/blob/master/you-can-help/make-up-new-exercises.md
I realize it may be a bit late for that, but it may have some context that helps.

In terms of including content from other locations, there's some discussion about this here: exercism/discussions#188

The TL;DR is that if there is an exercise that is commonly used, then we can implement our own description and implementations of it, and refer to the source as "common interview question" (or whatever). We can also cite a source of inspiration (e.g. for the 99 bottles of beer song problem we refer to Chris Pine's book, but could just as easily have referenced http://www.99-bottles-of-beer.net/).

We cannot use other people's copyrighted material. If other people have written code that we include, it has to be released under a compatible license. I prefer to avoid that entirely, because I am not a lawyer, and it's very complicated.

For some problems we make up a similar problem. E.g. instead of "fizzbuzz" we have "raindrops". They're not identical but they have similarities in structure.

[matthewmorgan]

@eliaahadi I'm glad to see your contribution was merged in to the problem-specifications repo. Congratulations!

Do you intend to implement this exercise here? Please let me know, otherwise we'll close this PR for now.

[eliaahadi]

I pushed a commit. Please review @matthewmorgan, thanks!

[matthewmorgan]

Hi @eliaahadi glad you're moving ahead with this PR!

I see that you have some build failures. They are all called out in the Travis build log:

Config issue:
https://travis-ci.org/exercism/javascript/builds/307661724#L1076

Code issue:
https://travis-ci.org/exercism/javascript/builds/307661724#L1097

Linting:
https://travis-ci.org/exercism/javascript/builds/307661724#L1107

Linting:
https://travis-ci.org/exercism/javascript/builds/307661724#L1120

Would you please resolve those and I would be happy to review the code then.

Let me know if you need assistance resolving any of the issues.

[eliaahadi]

@matthewmorgan I'm not quite sure how to check for the lines and edit exactly where they are from the latest commit. Thoughts?

[matthewmorgan]

@eliaahadi I would start by having a look at the Travis log

Near the bottom there is a section that show your tests failing, and explain why they are failing-- IE, match is not a function. Your ReverseString object has no method called match.

Does that help?

[eliaahadi]

@matthewmorgan the latest failure says " Expected 'Ramen' to equal 'nemaR'.". I'm not sure how to test and fix this, thoughts? Thanks.

[matthewmorgan]

What do you think that test result is telling you? Explain it to me in your own words.

[eliaahadi]

    var subject = new ReverseString('Ramen');
    var matches = subject.matches('nemaR');

    expect(matches).toEqual('nemaR');
  });

Subject using ReverseString function ('Ramen') as argument. The variable matches using the function subject which is from ReverseString. I'd expect subject function which reversing the string Ramen to match 'nemaR' from the "expect" line. Thoughts?

I referred to this to write my tests.
anagram

[matthewmorgan]

1. What does the matches function do to its input?
2. What is the input?
3. Therefore, what is the output of matches?

[eliaahadi]

1. What does the matches function do to its input? I expect subject.matches to reverse 'Ramen' and match 'nemaR'.
2. What is the input? Input is 'Ramen'
3. Therefore, what is the output of matches? Output after going through ReverseString function, gives, 'nemaR'.

I'm not so familiar with writing tests which is why I referred to the anagram example.

[matthewmorgan]

2. The input to matches is not Ramen. It is nemaR. If matches reverses its input, what is its output?

[eliaahadi]

Ok I think those errors were fixed in latest commit. Now I have spacing issues. Is there a way to see exactly where they are? Maybe an online ESLINT to check?

[matthewmorgan]

I'm glad to hear it passes the tests, but we're still not ready to call this PR finished. The basic problem is simple: you are doing the reversing of the string in your matches method.

A function called ReverseString should do one thing: reverse a string

You really do not need any other functions to make this work. Just fix that one.

[eliaahadi]

If I just fix the function ReverseString, it passes all tests. However, you previously mentioned to update the matches function to test if input string and revString are equal, return true, otherwise return false. If I change to boolean output, the tests fail since the reverse-string.spec.js matches test against actual values, not booleans. Thoughts?

'use strict';

function ReverseString(string) {
    var revString = '';
  for (var i = string.length - 1; i >= 0; i--) {
    revString += string[i];
  }
}

ReverseString.prototype.matches = function (string) {
  var revString = '';
  for (var i = string.length - 1; i >= 0; i--) {
    revString += string[i];
  }
  console.log(revString);
  return revString;
};

module.exports = ReverseString;

[matthewmorgan]

@eliaahadi I think I should be more explicit about what needs to change about the PR for it to be ready to merge. My plan had been to help you refactor your solution, first by fixing your two functions, then by simplifying the solution, but I can see that I confused you, so sorry about that.

What I was suggesting earlier is that you have two functions, each of which:

1. is clearly named
2. does one thing
3. the name of the function and the action of the function are similar.

Let's try a different approach. This exercise can, and ultimately should, be simplified to one function: ReverseString. This function should take a string, and return the reverse of that string. You don't need a matches function at all.

Right now, you have a reverseString that doesn't return anything, and a ReverseString.matches function that doesn't do any matching, but rather returns a reversed string.

Please write a ReverseString (capital R) class that

1. Takes a string
2. Returns the reversed string

You will still need this line also:

module.exports = ReverseString;

Remove all of the other code in your example solution please.

After that we will fix your tests.

[eliaahadi]

Ok I apologize again for not understanding this better. So just this function for the reverse-string.js?

'use strict';

function ReverseString(string) {
  var revString = '';
  for (var i = string.length - 1; i >= 0; i--) {
    revString += string[i];
  }
  return revString;
}

module.exports = ReverseString;

[matthewmorgan]

@eliaahadi please, no need to apologize! We're all here to learn-- that's the point!

That function does what it says, which is good. Now the tests will need to be updated to reflect this change in the API of the business class.

Two things:

1. Since your new function is not really a class constructor, but a simple function, you no longer need to call new. Just pass the input to the function and capture the result in a variable for testing. I suggest a variable called actual.
2. You don't need the matches function. You can just compare the expected reversed string with whatever actual value the ReverseString gave you using toEqual().

Let me know if you need further assistance.

[eliaahadi]

Ok, I'm not sure this is what you mean. So in reverse-string.spec.js for testing, it should be this below? I'm not sure if the actual variable or the input was passed correctly.

var ReverseString = require('./reverse-string');

///Just pass the input to the function and capture the result in a variable for testing. I suggest a variable called actual.
describe('Reverse a string', function (input) {
  var actual = input;

  it('reversing a string', function () {
    //compare the expected reversed string with actual value the ReverseString gave you using toEqual().
    expect(ReverseString(string)).toEqual(actual);
  });
});

This is from reverse-string.js

'use strict';

function ReverseString(string) {
  var revString = '';
  for (var i = string.length - 1; i >= 0; i--) {
    revString += string[i];
  }
  return revString;
}

module.exports = ReverseString;

[matthewmorgan]

Your example above compares the output of ReverseString to the input.

Do you expect the output of ReverseString to equal its input?

[eliaahadi]

So input doesn't seem to do anything, it just is passed as argument. I'd just think to do the same reverse string test inside and then compare. Does this make sense below?

var ReverseString = require('./reverse-string');

///Just pass the input to the function and capture the result in a variable for testing. I suggest a variable called actual.
describe('Reverse a string', function (input) {
  var actual = '';
 for (var i = input.length - 1; i >= 0; i--) {
    actual += input[i];
  }
  it('reversing a string', function () {
    //compare the expected reversed string with actual value the ReverseString gave you using toEqual().
    expect(ReverseString(string)).toEqual(actual);
  });
});

Thoughts?

[matthewmorgan]

@eliaahadi I did not mean to imply that you should pass an input to the test method (it()). I can't think of a situation in which you would want to do that.

What I meant was: you are passing an input to ReverseString. So, what do you expect the output of ReverseString to be? Compare the expected output to the actual output.

Each test should follow a simple pattern that is easy to understand. For example:

it('can reverse a cat', function() {
  var expected = 'tac';                // the correct result is visible right in the test
  var actual = ReverseString('cat');   // call your business function

  expect(actual).toEqual(expected);    // compare the two values
});

[eliaahadi]

Ok that makes more sense. The output of reverse string should be the reverse of a string. For example, in reverse-string

'use strict';

function ReverseString(string) {
  var revString = '';
  for (var i = string.length - 1; i >= 0; i--) {
    revString += string[i];
  }
  return revString;
}

module.exports = ReverseString;

console.log(ReverseString("cat")); // -> tac

Here's the screenshot of output from the console of Chrome.

So in the reverse-string.spec.js file, the code you written is as follows:

describe('ReverseString', function () {
it('can reverse a cat', function() {
  var expected = 'tac';                // the correct result is visible right in the test
  var actual = ReverseString('cat');   // call your business function

  expect(actual).toEqual(expected);    // compare the two values
});

Is that right?

[matthewmorgan]

Yes:

1. ReverseString should take a string and reverse it.
2. The tests should compare the expected value to the actual value.

Do you feel you understand how to finish the example code and the tests? If not, please ask whatever questions you have.

[eliaahadi]

I updated the reverse-string.spec.js with the new example.js that passes all the tests. Can you please review and let me know your thoughts @matthewmorgan? Thank you and happy holidays!

[matthewmorgan]

@eliaahadi excellent progress here! Happy Holidays to you as well.

I have two small comments that I would like you to address.

Also, it appears that you have some linting errors that are preventing your build from passing CI on Travis: some lines apparently have trailing spaces, which the linter does not allow.

Please make the changes I requested, and fix the linting errors, and this will be ready to merge!

See how much simpler and easier to understand your implementation is now? Well done.

[matthewmorgan]

Please add a blank line between this it() block and the next.

[matthewmorgan]

For this test, the canonical data tests reversing robot instead of cat.

Please change this test to match the canonical one.

[matthewmorgan]

One last change!

All tests but the first should be skipped by using xit instead of it.

As soon as this is done we'll merge the PR.

[matthewmorgan]

All it() blocks except the first one should be changed to xit(). This allows the student to pass one test at a time.

[eliaahadi]

Ok made the change you requested I think, please review again and thanks for all your help!

[matthewmorgan]

Nice! Congratulations on a job well done.

[eliaahadi]

Great thanks! Will this show up on the exesrcism website or are there more steps I need to do?

[matthewmorgan]

@eliaahadi I believe each track is deployed once per day by an automated process.

Normally changes show up within 24 hours, but let me know if you don't see the change by this time tomorrow and I can check in to it.

[matthewmorgan]

@eliaahadi I have not seen this deployed yet. I put in an issue to find out why.

[matthewmorgan]

OK! Katrina deployed the site, and your contribution is live.

Congratulations again. 🎉

[eliaahadi]

Great thanks again Matt! It took awhile, but we got it done. Happy new year!

…

On Sat, Dec 30, 2017 at 6:50 AM, Matthew Morgan ***@***.***> wrote: OK! Katrina deployed the site, and your contribution is live. Congratulations again. 🎉 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <exercism/javascript#417 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJTL0QV-VU_hOdiY6O7Biq1Klusw4bRiks5tFSZ8gaJpZM4P2gqO> .