NW6 | Hadika Malik | Module JS1 | Week 4#187
NW6 | Hadika Malik | Module JS1 | Week 4#187HadikaMalik wants to merge 4 commits intoCodeYourFuture:mainfrom
Conversation
pseudopilot
left a comment
pseudopilot
left a comment
There was a problem hiding this comment.
Hi @HadikaMalik !
Great job and I see and appreciate your efforts 👍
Though, there is a room for improvements. I left some comments. Please, check them.
| let timeFormat = (Number(time.slice(0, 2)) - 12) | ||
| if (Number(time.slice(0, 2)) > 12) { | ||
| if (timeFormat < 10) | ||
| return `0${timeFormat}:${time.slice(3,5)} pm`; | ||
| else | ||
| return `${timeFormat}:${time.slice(3,5)} pm`; |
There was a problem hiding this comment.
Please watch code formatting. The alignment of each line is different which is not right.
Try using VS Code Extension Prettier which helps you to format your code on saving document.
Also some semicolons are missing (in the end of the first line). Prettier can help you with both problems.
There was a problem hiding this comment.
This comment is relevant to all the files.
| if (cardString > 1 && cardString < 11) | ||
| return +cardString; | ||
|
|
||
| else if (cardString === 'J' || cardString === 'Q' || cardString === 'K') | ||
| return 10 | ||
|
|
||
| else if (cardString === 'A') | ||
| return 11 | ||
|
|
||
| else | ||
| return 'Invalid card rank' |
There was a problem hiding this comment.
I think this solution is missing something (second part of the card string):
Given a card string in the format "A♠" (representing a card in blackjack),
| function properFraction(num,den){ | ||
|
|
||
| if (den === 0) | ||
| return 'Error'; | ||
|
|
||
| if (num < den) | ||
| return 'True'; | ||
|
|
||
| if (num > den || num === den) | ||
| return 'False'; | ||
|
|
||
| if (num < 0 && num < den) | ||
| return 'True'; | ||
|
|
||
| if (num === den) | ||
| return 'False'; | ||
|
|
||
| } |
There was a problem hiding this comment.
First of all, we shouldn't return 'True' and 'False' as strings. These should be boolean types true and false.
Secondly, you have 2 checks which return true, and 2 checks which return false. Could you please try to merge/combine them to have only one check instead (or at least 2)?
| if (a + b <= c || a + c <= b || b + c <= a) | ||
| return 'False'; | ||
|
|
||
| else if (a <=0 || b <=0 || c <=0) | ||
| return 'False'; | ||
|
|
||
| else if (a + b > c || b + c > a || a + c > b) | ||
| return 'True'; | ||
|
|
There was a problem hiding this comment.
Firstly, same comment about true and false as above.
Secondly, try to combine the if blocks for this task too.
It is really possible to make it in 2 lines just like:
if (...) return false;
return (...);
|
|
||
| function getOrdinalNumber(number) { | ||
| if (number === 11){ | ||
| return `${number}th`; | ||
| } | ||
| if (number%10 === 1) { | ||
| return `${number}st`; | ||
| } |
There was a problem hiding this comment.
I don't think it covers all possible cases 🤔 But the start is good!
| if (count > 0 ){ | ||
| return str.repeat(count); | ||
| } | ||
| if (count === 1){ | ||
| return str; | ||
| } | ||
| if (count === 0){ | ||
| return " "; | ||
| } |
There was a problem hiding this comment.
I don't think we need to check all these 3 scenarios. count > 0 should be enough.
2 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Completed the week 4 exercises
Questions
I was confused a bit about count.test and is-prime.test.