NW-6 | AREEB-SATTAR | JS1| [TECH ED] Complete week 4 exercises | WEEK-4#184
NW-6 | AREEB-SATTAR | JS1| [TECH ED] Complete week 4 exercises | WEEK-4#184areebsattar wants to merge 16 commits intoCodeYourFuture:mainfrom
Conversation
| expect(repeatString("Ali", 0)).toBe(""); | ||
| expect(repeatString("Ali", 1)).toBe("Ali"); | ||
| expect(repeatString("Ali", -2)).toBe("negative count is not valid"); | ||
| }); |
HadikaMalik
left a comment
HadikaMalik
left a comment
There was a problem hiding this comment.
Your code make sense and is well written. In some parts I found that it can be more concise but still it makes sense. Well done!
| function countChar(str, char) { | ||
| let index = 0; | ||
| for (let i = 0; i < str.length; i++) { | ||
| if (str[i] === char) { |
There was a problem hiding this comment.
This is a well written code however can you explain this a bit? It seems a bit confusing to me, maybe if I understand what you have done here, it would make sense to me.
| function getOrdinalNumber(num) { | ||
| if (num % 10 === 1) { | ||
| if (num === 11) { | ||
| return "11th"; |
There was a problem hiding this comment.
I did mine a bit differently. Mine is a bit more concise, but this makes sense as well. Well done!
| // implement a function countChar that counts the number of times a character occurs in a string | ||
|
|
||
| function countChar(str, char) { | ||
| let index = 0; |
There was a problem hiding this comment.
Can you think of a more descriptive name for this variable? We already have i which keeps track of the index
| if (str[i] === char) { | ||
| index++; | ||
| } | ||
| while (str[i + 1] === char) { |
There was a problem hiding this comment.
From my understanding, this loop will keep counting up and moving through the string if the next element in the string is the character we're counting. However if we just let the for loop run, it will already check every character in the string. So this while loop isn't doing much for us, just using the for loop will be easier to understand 🙂
| @@ -1,4 +1,18 @@ | |||
| // In this week's prep, we started implementing getOrdinalNumber | |||
| function getOrdinalNumber(num) { | |||
| if (num % 10 === 1) { | |||
There was a problem hiding this comment.
Great use of the modulo % operator Areeb!
| if (num != Math.round(num)) { | ||
| return false; | ||
| } | ||
| for (var i = 2; i < num; i++){ |
There was a problem hiding this comment.
Great spot that you can begin this loop at 2! I would just replace var with let, as this helps to keep the variable scope as small as possible, in this case, only inside the for loop. Otherwise you would be able to access i outside of the loop which could cause bugs.
| if (num === 0) { | ||
| return ""; | ||
| } | ||
| if (num === 1) { | ||
| return str; | ||
| } else return repeatedString; |
There was a problem hiding this comment.
Check what happens when you give str.repeat(..) the number 0 or 1. You might find that you can remove a few if statements from your function 😉
As a general rule of thumb, even if you don't need to repeat the string (if the number is 1, for example), as long as the built-in function still behaves as expected, it's fine to still use its output.
SpectacularLandscape
left a comment
SpectacularLandscape
left a comment
There was a problem hiding this comment.
Looking good Areeb, you're able to really showcase your JavaScript and algorithm knowledge!
3 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Completed week-4 exercise except stretch
Questions
Ask any questions you have for your reviewer.