London | 26-ITP-Sept | Mars Adesina | Sprint 3 | Javascript Fundamentals - #1620
marscancode wants to merge 7 commits into
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment has been minimized.
This comment has been minimized.
7dae33d to
a70897f
Compare
marscancode
added
📅 Sprint 3
abdishakoor-dev
added
Review in progress
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Your explanations in 1-key-errors are clear, and every file runs with the right output. Prettier passes too.
Two things before I can mark this Complete:
2-mandatory-debug/1.js: the reason on line 11 needs another look. See my comment there.2-cases.js: see my comment on line 19.
Add the Needs Review label again once you've pushed.
|
|
||
| //console.log(`The sum of 10 and 32 is ${sum(10, 32)}`); | ||
| //console output: The sum of 10 and 32 is undefined | ||
| // sum is undefined because we haven't stored the operation we want it to perform correctly in a variable so its value is undefined. |
There was a problem hiding this comment.
Your fixed code works. But is a variable really what was missing? Try return a + b; with no variable. Does it work?
Now look at the original line 5. What does return; do on its own? Does line 6 ever run?
Please update line 11 with the real reason why the original code wasn't working.
There was a problem hiding this comment.
That's the real reason now, and return a + b; works without a variable. Good.
| // This might help https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/toUpperCase | ||
|
|
||
| // define function | ||
| function stringToUpperCase(str) { |
There was a problem hiding this comment.
Your function works: it gives WELCOME_TO_CYF. Only the name needs to change.
The name stringToUpperCase tells the reader that your function only makes the letters capital. But your function makes the letters capital and replaces spaces with _. That is called upper snake case (line 4). So the name of your function should reflect what the function is doing.
Also, JavaScript already has a method called toUpperCase (it capitalises any string passed to it), which you use on line 21. If you use that name as inspiration, and take into account what upper snake case means, what would you call your function now? Please rename it, and the call on line 26.
There was a problem hiding this comment.
stringToUpperCaseSnake says both jobs now, so that's fine.
For next time: a name like toUpperSnakeCase would fit even better. It follows the same pattern as JavaScript's own toUpperCase: "to" plus what the string becomes. No change needed here.
|
|
||
| function toPounds(pence) { | ||
| //Declare penceString variable | ||
| let penceString = pence; |
There was a problem hiding this comment.
Line 10 copies pence into a new variable. Unless penceString is some kind of new value, why not use pence directly? Do we really need to declare a new variable called penceString?
Also, none of the let variables in this file change after they are set. In 1-bmi.js you used const for that. Why not here too? Unless a variable value will change down the line, const is enough.
There was a problem hiding this comment.
Thanks Abdi! I've fixed the code and pushed the changes so hopefully all should be correct now.
There was a problem hiding this comment.
Good, const everywhere and no copy of the parameter. Thanks for doing the optional one too.
abdishakoor-dev
added
Reviewed
marscancode
added
Needs Review
There was a problem hiding this comment.
Nice work on 2-mandatory-debug/1.js. Your new reason on line 11 is exactly right: return ends the function, so a + b never runs.
One small thing before I can mark this Complete. 2-mandatory-debug/1.js now fails Prettier. Line 16, return a + b;, has no indent, and line 11 has a space at the end. Right click in the file, choose Format Document, save and push. To have Prettier do this every time you save, follow the format on save steps here: https://github.com/CodeYourFuture/Module-JavaScript-Fundamentals/blob/main/practical_guide.md
Add the Needs Review label again once you've pushed.
abdishakoor-dev
added
Reviewed
marscancode
added
Needs Review
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Thanks Mars. 2-mandatory-debug/1.js passes Prettier now. Marking this Complete.
abdishakoor-dev
added
Complete
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1053
Changelist
Completed all 5 sections of the sprint-3 coursework.