Glasgow | 25-ITP-SEP | Shreef Ibrahim | Sprint 3 | Coursework#717
Glasgow | 25-ITP-SEP | Shreef Ibrahim | Sprint 3 | Coursework#717shreefAhmedM wants to merge 15 commits intoCodeYourFuture:mainfrom
Conversation
shreefAhmedM
added
the
Needs Review
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good start on this sprint's tasks, most of it is really good. I have spotted a few areas where you could improve code further
| // write one test at a time, and make it pass, build your solution up methodically | ||
| // just make one change at a time -- don't rush -- programmers are deep and careful thinkers | ||
| function getCardValue(card) { | ||
| let rank = card.length === 1 ? card : card.slice(0, -1); |
There was a problem hiding this comment.
What circumstances would you have a card of length 1? A card in this case is always a rank + a symbol
There was a problem hiding this comment.
What circumstances would you have a card of length 1? A card in this case is always a rank + a symbol
Thanks for pointing that. I checked that if the card is a single character (length 1) for example, "A" it returns 10 which is not true , because this line ( let rank = card.length === 1 ? card: card.slice(0, -1); ) is handling the case where the card is only one character long, Instead of the card should always include both the rank and the suit.
i have updated the function.
| }); | ||
| // Case 3: Handle Face Cards (J, Q, K): | ||
| test("should return the value 10, a card with a rank of ('10', 'J', 'Q', or 'K')", () => { | ||
| const aceofSpades = getCardValue("K"); |
There was a problem hiding this comment.
Same comment I gave above - This is a king, but which king, hearts? spades?
| if (last2Digit >= 11 && last2Digit <= 13) { | ||
| return num + "th"; | ||
| } | ||
| if (las1Digit === 1) { |
There was a problem hiding this comment.
When you have a long series of if checks like this, is there another conditional structure that might be more appropriate to use?
There was a problem hiding this comment.
I have updated that one using switch case i thnk is more appropiate.
| } | ||
|
|
||
| module.exports = passwordValidator; No newline at end of file | ||
| if (oldPassword.includes(password)) { |
There was a problem hiding this comment.
Could you simplify this final check?
LonMcGregor
added
Reviewed
… unnecessary length check
shreefAhmedM
added
the
Needs Review
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Great. These changes look good to me.I just have one very minor additional suggestion.
| // write one test at a time, and make it pass, build your solution up methodically | ||
| // just make one change at a time -- don't rush -- programmers are deep and careful thinkers | ||
| function getCardValue(card) { | ||
| let rank = card.slice(0, -1); |
There was a problem hiding this comment.
Is there a way you could simplify this slice even more?
(Hint: You can achieve the same thing with only one argument to slice)
There was a problem hiding this comment.
I was not really thinking about the card length - I was more thinking about the .slice function itself. Here you are passing 2 arguments, 0 and -1. Did you know you can call .slice with only a single argument?
There was a problem hiding this comment.
I understand what you meant, but I'm not sure if there is a way to use slice() with one argument to remove the last character of a string. if i used slice(-1) it returns only the last character, which is opposite to what I want. Can you help figure out ?
There was a problem hiding this comment.
Oh,I'm very sorry, I had it backwards when I was looking at the code. You are correct. Apologies for the confusion. I'll mark this as complete now
There was a problem hiding this comment.
that find thank very much for your time
LonMcGregor
added
Complete
3 participants
Self checklist
Her is JavaScript challenges tasks covering running test, error fixing, code interpretation, and a stretch exploration activity.