NW6|Zeliha-Pala | JS1 Module | [TECH ED] Complete week 3 exercises | Week 3 #172
Conversation
week-3/implement/get-angle-type.js
Outdated
| if (angle === 360) return "Full angle"; | ||
| if (angle < 360) return "Reflex angle"; | ||
| return "Unknown angle"; | ||
| } |
There was a problem hiding this comment.
I really like how you have wriiten your if statements in one line. It is easy to follow and easy to read.
I like that you also have a statement for unknown angle. That is really good. 👍 💯
|
|
||
| function getCardValue(card) { | ||
| const cardRank = card.slice(0, -1); | ||
|
|
There was a problem hiding this comment.
I am a bit confused why you are using slice here. Could you explain please. Maybe I am missing your point of what you are trying to do.
There was a problem hiding this comment.
i used it to extract the rank of the card from the card string
For instance, if card is "5♠", card.slice(0, -1) will return "5" because it's taking all characters from index 0 to the last character (-1 index is the last character in the string) excluding the last character ("♠" in this case).
| } else { | ||
| throw new Error("Invalid card rank"); | ||
| } | ||
| } |
There was a problem hiding this comment.
I learnt something new here. I did not know what !isNan and parseInt and throw new Error mean. But I looked up on MDN website and found the answer.
That is a pretty good code. Well done!
| console.assert( | ||
| isProperFraction(3, 3) === false, | ||
| "Equal Numerator and Denominator failed" | ||
| ); |
There was a problem hiding this comment.
This code is written nicely and clean. It is easy to follow and understand. Well done! 💯
| console.assert( | ||
| isValidTriangle(0, 3, 3) === false, | ||
| "Zero side length is invalid" | ||
| ); |
There was a problem hiding this comment.
Nice, clean, easy to follow code. 👍
There was a problem hiding this comment.
Hi @zelihapala , this is a very good work!
I have only few comments for you. Mostly they are suggestions/recommendations about possible refactoring/improvements.
Only isProperFraction implementation seems to be a little buggy. I explained one corner case below.
Good luck!
| if (Number(time.slice(0, 2)) > 12) { | ||
| return `${Number(time.slice(0, 2)) - 12}:00 pm`; | ||
| return `${(Number(time.slice(0, 2)) - 12) | ||
| .toString() | ||
| .padStart(2, "0")}:${time.slice(time.indexOf(":") + 1)} pm`; // I got this part |
There was a problem hiding this comment.
Would you be able to refactor it a bit? You have same piece of code 2 times which may be stored in a variable instead: Number(time.slice(0, 2)).
| if (denominator === 0) { | ||
| return "Error: Denom cannot be 0"; | ||
| } else if (numerator < denominator) { | ||
| return true; | ||
| } else if (numerator >= denominator) { | ||
| return false; | ||
| } else if (numerator < 0 && Math.abs(numerator) < denominator) { | ||
| return true; | ||
| // condition checks if the fraction has a negative numerator. The Math.abs() function finds the absolute value, which is the positive size or amount of a number, no matter if it's negative or positive. So, for a negative number, its absolute value gives the positive size of that number. | ||
| } else if (numerator === denominator) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
There are few points here:
-
you have few checks which return
trueand few which returnfalse. Can you combine them to have only one check which returnstrueotherwise we just returnfalse. -
in your code you have:
} else if (numerator < denominator) {
...
} else if (numerator < 0 && Math.abs(numerator) < denominator) {
I'm almost sure that if you have have negative numerator (like -10) and positive denominator (like 2) then your first check returns true and you won't even reach the second check.
But I think you can avoid this problem if you re-organise the function as I suggested in 1)
| if (a <= 0 || b <= 0 || c <= 0) { | ||
| return false; | ||
| } | ||
|
|
||
| if (a + b <= c || a + c <= b || b + c <= a) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
You have 2 checks and both return false. Is it possible to merge them in one check?
| if (lowercaseAlphabet.includes(char)) { | ||
| // if lowercase letter | ||
| const rotatedIndex = (lowercaseAlphabet.indexOf(char) + shift) % 26; // read for me comment | ||
| return lowercaseAlphabet[rotatedIndex]; | ||
| } else if (uppercaseAlphabet.includes(char)) { | ||
| const rotatedIndex = (uppercaseAlphabet.indexOf(char) + shift) % 26; // same for upperAlphabet | ||
| return uppercaseAlphabet[rotatedIndex]; | ||
| } else { | ||
| return char; // (non-letter remains unchanged) | ||
| } |
There was a problem hiding this comment.
This is good and definitely should work. Though it may be optimised in a few ways:
- For now each time we call the function
rotateCharacterwe check/iterate the same string of letters 2 times:
- 1st time when we call
includes - 2nd time when we call
indexOf
Can we avoid usingincludesat all in this task and just use onlyindexOfand only once?
What this method returns us when there is no value we search in string?
- In the worst case (e.g. the argument is a number) we will check 2 strings of letters before returning original value back. But I think we can use one string of letters (e.g.
lowercaseAlphabet) instead of two if we utilise the string methodstoLowerCase()andtoUpperCase()
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.