Glasgow | ITP May -25 | Pandi Simatupang | Module-Structuring-and-Testing-Data | coursework/sprint-3 #651
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
YoanHlebarov
added
the
Review in progress
There was a problem hiding this comment.
The code is correct but it can be improved in terms of performance (doing many checks before returning instead of returning short whenever a check fails) and readability. Think how you can improve the code, is there a way you can write things that are easier to read?
Comment on lines
10
to
+16
| function getAngleType(angle) { | ||
| if (angle === 90) return "Right angle"; | ||
| // read to the end, complete line 36, then pass your test here | ||
| if (angle === 90) return "Right angle"; | ||
| else if (angle === 180) return "Straight angle"; | ||
| // read to the end, complete line 36, then pass your test here | ||
| else if (angle < 90) return "Acute angle"; | ||
| else if (angle > 90 && angle < 180) return "Obtuse angle"; | ||
| else return "Reflex angle"; |
There was a problem hiding this comment.
Here you don't need to write an if elseif statement. When you return short like this. you can simply write if statements each line. Another approach is to use switch.
Comment on lines
10
to
+14
| function isProperFraction(numerator, denominator) { | ||
| if (numerator < denominator) return true; | ||
| if (numerator === 0 || denominator === 0) return false; | ||
| else if (numerator === denominator) return false; | ||
| else if (Math.abs(numerator) < Math.abs(denominator)) return true; | ||
| else if (Math.abs(numerator) > Math.abs(denominator)) return false; |
Comment on lines
10
to
20
| function getCardValue(card) { | ||
| if (rank === "A") return 11; | ||
| let rank = card.slice(0, -1); //start from 0 until just before one last character | ||
| if (rank === "A") return 11; | ||
| else if ((rank === "10") | (rank === "J") | (rank === "Q") | (rank === "K")) | ||
| return 10; | ||
| else if (rank > 1 && rank < 10) | ||
| return Number( | ||
| rank | ||
| ); //need to convert to number, because === compare type and exact number | ||
| else return null; | ||
| } |
Comment on lines
+4
to
9
| else if (angle === 180) return "Straight angle"; | ||
| // read to the end, complete line 36, then pass your test here | ||
| else if (angle < 90) return "Acute angle"; | ||
| else if (angle > 90 && angle < 180) return "Obtuse angle"; | ||
| else return "Reflex angle"; | ||
| } |
Comment on lines
1
to
9
| function isProperFraction(numerator, denominator) { | ||
| if (numerator < denominator) return true; | ||
| // add your completed function from key-implement here | ||
| //if (numerator < denominator) return true; | ||
| // add your completed function from key-implement here | ||
|
|
||
| if (numerator === 0 || denominator === 0) return false; | ||
| else if (numerator === denominator) return false; | ||
| else if (Math.abs(numerator) < Math.abs(denominator)) return true; | ||
| else if (Math.abs(numerator) > Math.abs(denominator)) return false; | ||
| } |
Comment on lines
+5
to
+15
| if (myNum < 10) { | ||
| if (myNum === "1") return `${num}st`; | ||
| else if (myNum === "2") return `${num}nd`; | ||
| else if (myNum === "3") return `${num}rd`; | ||
| else return `${num}th`; | ||
| } else if (myNum >= 10) { | ||
| if (myNumLastChar === "1") return `${num}st`; | ||
| else if (myNumLastChar === "2") return `${num}nd`; | ||
| else if (myNumLastChar === "3") return `${num}rd`; | ||
| else return `${num}th`; | ||
| } |
Comment on lines
+1
to
+7
| function repeat(str, rep) { | ||
| myStr = ""; | ||
| for (let i = 0; i < rep; i++) { | ||
| myStr = myStr + str; | ||
| } | ||
| if (rep < 0) return "negative counts are not valid"; | ||
| return myStr; |
There was a problem hiding this comment.
Suggested change
| function repeat(str, rep) { | |
| myStr = ""; | |
| for (let i = 0; i < rep; i++) { | |
| myStr = myStr + str; | |
| } | |
| if (rep < 0) return "negative counts are not valid"; | |
| return myStr; | |
| function repeat(str, rep) { | |
| if (rep < 0) return "negative counts are not valid"; | |
| myStr = ""; | |
| for (let i = 0; i < rep; i++) { | |
| myStr = myStr + str; | |
| } | |
| return myStr; |
You should always be validating your inputs first, that way there are no dubious values being passed down your program which can create a problem. In this case you should check first that the count is a positive integer, before running any other part of your function.
Comment on lines
17
to
+31
| function passwordValidator(password) { | ||
| return password.length < 5 ? false : true | ||
| } | ||
| let arr = password.split(""); | ||
| // - Have at least 5 characters. | ||
| if (arr.length < 5) return false; | ||
| // - Have at least one of the following non-alphanumeric symbols: ("!", "#", "$", "%", ".", "*", "&") | ||
| const symbols = new Array("!", "#", "$", "%", ".", "*", "&"); | ||
|
|
||
| const hasSymbol = arr.some((char) => symbols.includes(char)); | ||
| const hasDigit = arr.some((char) => /[0-9]/.test(char)); | ||
| const hasLower = arr.some((char) => /[a-z]/.test(char)); | ||
| const hasUpper = arr.some((char) => /[A-Z]/.test(char)); | ||
|
|
||
| const usedPass = !usedPasswords.includes(password); | ||
| return hasSymbol && hasDigit && hasLower && hasUpper && usedPass; | ||
| } |
There was a problem hiding this comment.
I would consider moving the check if password is used near the top of the function. Similar to the comment above, to check if it is in the usedPasswords list is simple and should be done first, as if it is then the password cannot be used. In your implementation we are doing all checks and then deciding whether the password can be used or not. Consider if using similar approach to other exercises form the module can be used here (eg return short if a check fails).
YoanHlebarov
added
Reviewed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Learners, PR Template
Self checklist
Changelist
Answers and comments to sprint-3
Questions
Ask any questions you have for your reviewer.