Bug - : 03_reverseString - No test case having commas in the input string

[vishpant76]

Complete the following REQUIRED checkboxes:

• I have thoroughly read and understand The Odin Project Contributing Guide
• The title of this issue follows the Bug - location of bug: brief description of bug format, e.g. Bug - Exercises: File type incorrect for all test files

The following checkbox is OPTIONAL:

• I would like to be assigned this issue to work on it

---

1. Description of the Bug:

While doing this exercise, initially it didn't occur to me that there are built-in reverse() and join() methods that can be used to solve it in a single line. I came up with the following approach to reverse the string:

const reverseString = function (str) {
  const strArray = str.split("");
  let start = 0;
  let end = strArray.length - 1;
  while (start < end) {
    let temp = strArray[start];
    strArray[start] = strArray[end];
    strArray[end] = temp;
    start++;
    end--;
  }
  let strArrayToString = strArray.toString();
  strArrayToString = strArrayToString.replaceAll(",", "");
  return strArrayToString;
};

This approach passes all four test cases in the reverseString.spec.js file. However, had there been a test case with the input string having commas, this approach would have produced incorrect results, as it would have removed all commas from the output.

2. How To Reproduce:

1. In the reverseString.js exercise, write the function using the code that I have shared above.
2. Run the tests: npm test reverseString.spec.js. All tests will be passed, which is expected.
3. Now, open the file reverseString.spec.js, and add a new test case:

  test("works with strings having commas", () => {
    expect(reverseString("Hello, World!")).toEqual("!dlroW ,olleH");
  });

This test will fail because the code will remove the comma after 'Hello' from the output string, as shown in the screenshot below:

The reverseString-solution.js passes this test case with comma so all good with that, but I just thought to highlight this as in case someone else attempts to code a solution for this exercise using an approach similar to mine, although they would pass all the current test cases, but may not pass a test where the input string has commas in it. So, I believe an additional test case having comma (or multiple commas) can be added to the spec file.

3. Expected Behavior:

4. Desktop/Device:

• Device:
• OS:
• Browser:
• Version:

5. Additional Information:

[MaoShizhong]

Thanks for opening this issue @vishpant76, that's indeed a very interesting edge case you've discovered!

As opposed to adding a new test case specifically for commas, perhaps we should instead just amend the existing punctuation test to include a comma somewhere. After all, commas aren't any special punctuation, just punctuation nonetheless.

  test.skip('works with numbers and punctuation', () => {
    expect(reverseString('123! abc!')).toEqual('!cba !321')
  })

becomes something like

  test.skip('works with numbers and punctuation', () => {
    expect(reverseString('123! abc! Hello, Odinite.')).toEqual('.etinidO ,olleH !cba !321')
  })

Since you didn't tick the assignment box, I'll open this for assignment if someone wishes to comment below. Let me know if you wish to be assigned to make this change instead.

[vishpant76]

Hey @MaoShizhong - yup, that makes perfect sense! I would love to work on it though still pretty new to PRs, etc 😅. Alright, please assign to me :).

[MaoShizhong]

Assigned! Be sure to read the contributing guide and let me know if you run into any issues as well.