ITP-JAN-25 | London | Ihor Bolzhelarskyi | Module-Structuring-and-Testing-Data |Sprint 2| Week 4#274
Conversation
IhorBolzhelarskyi
added
the
Needs Review
illicitonion
added
Review in progress
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This is generally looking really good, congrats!
Please do updated the code with the suggestions I've made and re-add the "Needs Review" label :)
Sprint-2/1-key-errors/0.js
Outdated
| // =============> write your new code here | ||
|
|
||
| function capitalise(str) { | ||
| let string = `${str[0].toUpperCase()}${str.slice(1)}`; |
There was a problem hiding this comment.
I don't think it's super clear what the difference is between a variable named str and a variable named string.
Can you think of how we could make it more clear what the difference is between these variables?
There was a problem hiding this comment.
The variable str is a parameter of function capitalise(str) (input of this function). It holds a original string passed into the function. The variable string is lacal variable in this function, that stores modified version of parameter str, wich we are going to return (output of this function).
There was a problem hiding this comment.
I can work that out by reading the function, but I don't think it's very clear at a glance.
Imagine if this function was much longer, and you used string at some point inside it - would it be obvious whether it was the original string or the fixed string?
In general if you have two similar variables, I'd recommend changing both of their names to make clear what each one is.
There was a problem hiding this comment.
Yes,i got this, their names are almost the same
Thank you
There was a problem hiding this comment.
Can you rename them to be more clear? :)
illicitonion
added
Reviewed
IhorBolzhelarskyi
added
the
Needs Review
illicitonion
removed
the
Needs Review
|
|
||
| if (hours === 0) { | ||
| hours = 12; | ||
| } else if (hours > 12) hours = hours - 12; |
There was a problem hiding this comment.
I'd recommend always using {}s around the body of if/else statements. As well as consistency (which is good), you can read more about some of the problems not using {}s causes at https://www.blackduck.com/blog/understanding-apple-goto-fail-vulnerability-2.html
Sprint-2/1-key-errors/0.js
Outdated
| // =============> write your new code here | ||
|
|
||
| function capitalise(str) { | ||
| let string = `${str[0].toUpperCase()}${str.slice(1)}`; |
There was a problem hiding this comment.
Can you rename them to be more clear? :)
Sprint-2/1-key-errors/0.js
Outdated
| function capitalise(str) { | ||
| let string = `${str[0].toUpperCase()}${str.slice(1)}`; | ||
| return string; | ||
| let modifiedStr = `${str[0].toUpperCase()}${str.slice(1)}`; |
There was a problem hiding this comment.
I'd generally recommend updating both variables - originalStr and modifiedStr - otherwise if someone just sees str they may still have the question "is this the old one or the new one" :)
There was a problem hiding this comment.
yes, that makes sense, thank you
IhorBolzhelarskyi
added
Needs Review
2 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.