Skip to content
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

NW6| FIDAA BASHIR | Module-JS1 | WEEK2 #179

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Fidaabashir89
Copy link

Learners, PR Template

Self checklist

  • [ ✅ I have committed my files one by one, on purpose, and for a reason
  • [ ✅] I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • [ ✅ ] I have tested my changes
  • [ ✅ ] My changes follow the style guide
  • [ ✅] My changes meet the requirements of this task

Changelist

Briefly explain your PR.
I worked on the JS1 module exercises

Questions

Ask any questions you have for your reviewer.

Copy link

@Ara225 Ara225 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, well done!

@@ -5,3 +5,7 @@ function multiply(a, b) {
}

console.log(`The result of multiplying 10 and 32 is ${multiply(10, 32)}`);

//The first line of the function logs the product of the two arguments to the console. The second line of the code calls the multiply() function with the arguments 10 and 32, and then logs the result to the console. The expected output is:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, and well explained.

}

console.log(`The sum of 10 and 32 is ${sum(10, 32)}`);

//the error here that we should place the a + b expression directly after the return keyword.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true, I would probably word it a bit differently - the problem is that the function returns nothing - the line below return is never executed.

function getLastDigit() {
return num.toString().slice(-1);
function getLastDigit(num) {
return num % 10;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a clever solution. No one else I've seen has done this. It does have a small bug, though, if we change the inputs a bit. Also if possible some more explanation would be great

@@ -4,6 +4,7 @@
// interpret the error message and figure out why it's happening, if your prediction was wrong

function capitalise(str) {
let str = `${str[0].toUpperCase()}${str.slice(1)}`;
str = `${str[0].toUpperCase()}${str.slice(1)}`;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant, why did this fix it?

@@ -2,12 +2,14 @@
// Write down the error you predict will be raised
// Why will an error occur when this program runs?
// Play computer with the example to work out what is going on
//In this code, there is an error that the variable decimalNumber, declared twice. once as a parameter of the convertToPercentage function and again outside the function, because in JS variables can't be declared twice within the same scope
// to fix this error, we need to delete the parameter in line 8, so we can call the function without any argument.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well explained 👏

Comment on lines +16 to +21

function calculateBMI(weight, height) {
const BMI = weight / (height * height);
return BMI.toFixed(1);

}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this, simple elegant solution!

// Come up with a clear, simple name for the function
// Use the string documentation to help you plan your solution

function camelCaseToWords(text) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great little function, well done. For next time, I'd probably name it differently, functions rend to get named something g like convertToUpperCamelCase() but this is a mere niggle

Comment on lines +6 to +13

function toPounds(penceString) {
const penceStringWithoutTrailingP = penceString.substring(0, penceString.length - 1);
const paddedPenceNumberString = penceStringWithoutTrailingP.padStart(3, "0");
const pounds = paddedPenceNumberString.substring(0, paddedPenceNumberString.length - 2);
const pence = paddedPenceNumberString.substring(paddedPenceNumberString.length - 2).padEnd(2, "0");
return `£${pounds}.${pence}`;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, appreciate the descriptive var names!

@@ -8,3 +8,11 @@
// Given a number,
// When I call this function with a number
// Then it returns the new price with VAT added on

function calculatePriceWithVAT(price) {
price = price * 1.2;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works, Could just do return price * 1.2; here - this line isn't strictly necessary

//we can use padStart(2, '0') to pad the numbers to a length of two characters with the padding character '0'. like this:
//function pad(num) {
//return num.toString().padStart(2, '0');
//}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All amazing here no faults that I can find, well done 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants