-
-
Notifications
You must be signed in to change notification settings - Fork 195
London | May-2025 | Fatima Z Belkedari | Module Structuring and Testing Data | Sprint 2 #610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this very good. You've obviously picked up a lot of Javascript fundamentals. However there are a few little slips here and there. Running the code would help you catch these. Let me know if you need me to expand or clarify my comments.
Sprint-2/1-key-errors/0.js
Outdated
|
||
function capitlise(str) { | ||
let capitalise =`${str[0].toUpperCase()}${str.slice(1)}`; | ||
return str; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the result of running this code with a test string e.g. 'hello'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result would be Hello with a capital letter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the right variable being returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I should return capitalise. Thank you
Sprint-2/2-mandatory-debug/0.js
Outdated
|
||
function multiply(a, b){ | ||
return (a*b); | ||
} | ||
console.log(`The result of multiplying 10 and 32 is ${multiply(10*32)}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment out the original faulty code and try running your code. Can you see a problem with the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the problem is that I should add comma instead of *.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, well-spotted.
Sprint-2/2-mandatory-debug/1.js
Outdated
// Finally, correct the code to fix the problem | ||
// =============> write your new code here | ||
|
||
function sum(a, b){ | ||
return(a+b); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the brackets inside the function required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are optional with the return function but I will remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right. Sometimes we do want to use brackets that aren't strictly necessary because it can make complicated expressions easier to read but in this case I think it creates clutter and I wanted to be sure you knew they were optional.
Sprint-2/2-mandatory-debug/2.js
Outdated
console.log(`The last digit of 42 is ${getLastDigit(42)}`); | ||
console.log(`The last digit of 105 is ${getLastDigit(105)}`); | ||
console.log(`The last digit of 806 is ${getLastDigit(806)}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an issue in the console.log
statements. Can you spot it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I can't spot it. Because I already fixed the code by adding a parameter number between brackets . So when you run the code it will give you the last digit of 42 which is 2 , and so on. If I am wrong ,enlighten me please. Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is getLastDigit
the name of your function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I see. My function name is getTheLastDigit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, well-done. Just as a side note - typically getLastDigit
would be preferred to getTheLastDigit
as a function name because 'The' doesn't really add meaning and it's good to be concise. No need to change it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.I will take that into account . Thank you
} | ||
console.log(calculateBMI(55, 1.65)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How might we amend the code so the output is displayed to 1 decimal place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By simply adding toFixed(1).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, toFixed(1)
is the right method to use but how can we use it so that the function returns the BMI to one decimal place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding it to the following code: console.log(calculateBMI(55, 1.65).toFixed(1));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but this means every time we call the function we have to apply toFixed(1)
to the output. But if we use toFixed(1)
inside the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I see. I used it inside the function:
function calculateBMI(weight, height) {
const square =weight/ (height*height);
return square.toFixed(1);
I hope it is fixed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I'll change label to complete. But if you have any questions let me know.
@@ -14,3 +14,12 @@ | |||
// You will need to come up with an appropriate name for the function | |||
// Use the MDN string documentation to help you find a solution | |||
// This might help https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/toUpperCase | |||
|
|||
function ToUpperSnakeCase(input){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this function name follow the usual camelCase convention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it does not. "camelCase" starts with a small lowercase letter and each new word starts with capital letter.
Please remove the Reviewed label when you add the Needs Review label, to make it more clear what state this PR is in. |
If you could just look again at the BMI exercise and amend the code we could move this to complete. |
Thank you for your support. |
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.