02_repeatString - Line-32, replace "odin" with "hey"

TheOdinProject / javascript-exercises

MIT License

1.28k stars 34.52k forks source link

02_repeatString - Line-32, replace "odin" with "hey" #505

Open bhagyeshsp opened 6 days ago

bhagyeshsp commented 6 days ago

The original code had "hey" as the seed string value and hence the comment on line-29 mentions "hey". But the last commit on the code changed "hey" to "odin".

We can either change instances of comment or the code. I propose changing the code instance from "odin" to "hey" for consistency.

Because

There is an inconsistency between the comment on line-29 and its corresponding code on line-32. It provides clarity to the learner by maintaining the same example of "hey" instead of "odin".

This PR

Replaced two instances of "odin" with "hey" on line-32

Issue

Closes #XXXXX

Additional Information

Pull Request Requirements

[x] I have thoroughly read and understand The Odin Project Contributing Guide
[x] The title of this PR follows the location of change: brief description of change format, e.g. 01_helloWorld: Update test cases
[x] The Because section summarizes the reason for this PR
[x] The This PR section has a bullet point list describing the changes in this PR
[ ] If this PR addresses an open issue, it is linked in the Issue section
[ ] If this PR includes any changes that affect the solution of an exercise, I've also updated the solution in the /solutions folder

MaoShizhong commented 5 days ago

In #460 the instructions were made clear that String.prototype.repeat should not be used and the implementation should be done manually via loops. I wonder if the team would agree that on top of fixing the 'hey' thing, the test be changed to

expect(repeatString('hey', number)).toBe('hey'.repeat(number));

Then the big regex explanation comment can be removed.

bhagyeshsp commented 4 days ago

In #460 the instructions were made clear that String.prototype.repeat should not be used and the implementation should be done manually via loops.

Thanks, I saw #460 I'm unsure how it is related to this PR. The current PR is more content-specific than code-specific.

I wonder if the team would agree that on top of fixing the 'hey' thing, the test be changed to
expect(repeatString('hey', number)).toBe('hey'.repeat(number));
EDIT: this PR does not propose this code change. This PR proposes retaining the same code currently present. odin replaced with hey in this code:
expect(repeatString('odin', number).match(/((odin))/g).length).toEqual(number);
This PR proposes getting the comment, code and example in alignment by maintaining "hey" throughout.

Then the big regex explanation comment can be removed.

I found the regex explanation educational.

MaoShizhong commented 4 days ago

My comment was more an addition to yours and directed at the team when they review as opposed to directed at you, sorry if that wasn't clear. Regex was previously removed from comments and solutions in some of these exercises due to it being rather out of scope for this part of the curriculum. Hence the suggestion to the team that while we're at it, it may be best to apply the same approach here.