London | 26-ITP-May | Alex Jamshidi | Sprint 3 | Practice tdd#1319
London | 26-ITP-May | Alex Jamshidi | Sprint 3 | Practice tdd#1319Alex-Jamshidi wants to merge 4 commits into
Conversation
Alex-Jamshidi
added
📅 Sprint 3
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
LGTM - a couple of naming things to look at
| return "1st"; | ||
| if (!Number.isInteger(num) || num < 1) {throw new Error("Invalid number");} | ||
|
|
||
| let number = num.toString().slice(-2); |
There was a problem hiding this comment.
"num" and "number" are very hard to distinguish variables - it's not clear from their names what the differences between them are. Can you think how to improve this?
There was a problem hiding this comment.
renamed number to lastTwoDigits to make clearer what is happening
|
|
||
| let number = num.toString().slice(-2); | ||
| if (number == 11 || number == 12 || number == 13) {return `${num}th`;} | ||
| if (number.slice(-1) == 1) {return `${num}st`;} |
There was a problem hiding this comment.
I'd extract a variable for that number.slice(-1) operation that you're repeating three times.
There was a problem hiding this comment.
extracted and named lastDigit, code much more readable now
illicitonion
added
Reviewed
Alex-Jamshidi
added
Needs Review
|
|
||
| const lastTwoDigits = num.toString().slice(-2); | ||
| const lastDigit = lastTwoDigits.slice(-1); | ||
| if (lastTwoDigits == 11 || lastTwoDigits == 12 || lastTwoDigits == 13) { |
There was a problem hiding this comment.
We almost never want to use == in JavaScript, and almost always want to prefer === - can you do some research as to why, and update your code to use === instead?
There was a problem hiding this comment.
So I did know the difference here, == doesn't care about type, just value.
I've not really thought about being strict with this when I know what the code is going to do, but I realise it's much better to restrict as much as possible to prevent any incorrect data leaking through. I will aim to always use === going forward, expect in cases where type make not matter.
updated code. thanks
There was a problem hiding this comment.
Does your code now pass its tests? :)
There was a problem hiding this comment.
I thought I check them!
No they didn't pass, because I was now trying to compare a string to number
converted strings to numbers to pass === check
tests all pass
illicitonion
added
Reviewed
Alex-Jamshidi
added
Needs Review
illicitonion
added
Reviewed
Alex-Jamshidi
added
Needs Review
illicitonion
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
Added solutions to Sprint 3 Practice TDD