Leeds_ Shimaadnan_JavaScript_Core1_week3 - #129
Conversation
Gevie
left a comment
There was a problem hiding this comment.
Hi Shima
You have done very well here, I am afraid I need to leave my review partially complete at 2-mandatory/4-stock.js but I will resume my review tomorrow.
Hopefully this feedback helps you in the meantime.
-Stephen
| let hello = sayHello(); | ||
| console.log(hello); | ||
|
|
||
| // We have not define any return or outcome for our function. |
|
|
||
| sayHelloToUser(); | ||
|
|
||
| // We have not define anything for our "user " argument. |
| let a; | ||
| console.log(a); | ||
|
|
||
| // Because the value of a is not defined. |
| let evenNum = []; | ||
| let i = 0; | ||
| let even = 0; | ||
| while (i < n) { |
There was a problem hiding this comment.
This works great.
Do you think we could improve this using the modulo operator? (%)?
if (n % 2 === 0) {
evenNumber.push(n);
}There was a problem hiding this comment.
Excuse me.
In the question we have been ask to do with while loop
| } | ||
| i++; | ||
| } | ||
| // TODO |
There was a problem hiding this comment.
It's always a good idea to remove inline comments unless absolutely necessary :)
| */ | ||
| function potentialHeadlines(allArticleTitles) { | ||
| // TODO | ||
| let sutibleAricleTiteles = []; |
There was a problem hiding this comment.
Be cautious of spelling mistakes, I believe this should be suitableArticleTitles :)
There was a problem hiding this comment.
Yes I should change it.
Thanks
| // TODO | ||
| let sutibleAricleTiteles = []; | ||
| for (let i = 0; i < allArticleTitles.length; i++) { | ||
| if (allArticleTitles[i].length <= 65) { |
There was a problem hiding this comment.
This is great, you could do this with some other functions but this works and isn't a performance drain.
| function titleWithFewestWords(allArticleTitles) { | ||
| // TODO | ||
| // TODO | ||
| tilteWithFewest = allArticleTitles.filter((item) => item.length <= 65); |
There was a problem hiding this comment.
You're pretty close here, what you're doing is you're checking against the length of characters instead of the number of words. Words are split up by a space, some words may be 2 characters long and some may be up to 45 characters long. (The longest word in the English dictionary is "Pneumonoultramicroscopicsilicovolcanoconiosis" so if you imagine the title is that word twice, the word count is 2 but the character count is 91 - it'd be the shortest word count but would not pass this check)
There was a problem hiding this comment.
Yes I understand my mistake.
Thanks I will change it
| // TODO | ||
| let popularHeadline = []; | ||
| for (let i in allArticleTitles) { | ||
| if (allArticleTitles[i].match(/[0-9]/g) !== null) { |
There was a problem hiding this comment.
Great use of the [0-9] regular expression, you can also shortcut this to \d which means decimals but what you did is probably more readable! Great job 💯
| aveNumofCharacters.push(allArticleTitles[i].length); | ||
| } | ||
| let total = 0; | ||
| for (let j in aveNumofCharacters) { |
There was a problem hiding this comment.
I'd be careful of using single character variable names just because if you have code which is 50-100 lines long, it becomes hard to read. What you have done here is fine it's just something to consider.
No description provided.