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

Glasgow_Class|Shreef_Ibrahim|Structuring _Testing _Datast|Week2_exercise2_update #236

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

Conversation

shreefAhmedM
Copy link

@shreefAhmedM shreefAhmedM commented Dec 22, 2024

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.

Questions

Ask any questions you have for your reviewer.

@shreefAhmedM shreefAhmedM added the Needs Review Participant to add when requesting review label Dec 22, 2024
Copy link

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Code is good in general. I left some comments and suggestions.

There are many typos and misspelled words in the comments. You can try using ChatGPT to help you improve them.

const percentage = `${decimalNumber * 100}%`;

return percentage;
}
// the decimalNumber has already been declared its constant cant except

console.log(decimalNumber);
Copy link

Choose a reason for hiding this comment

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

When you try executing this script, does it output 50%?

Copy link
Author

Choose a reason for hiding this comment

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

Yes , when you execute console.log(convertToPercentage(0.5)) the output is 50%?

@@ -21,4 +28,4 @@ const targetOutput2 = "11:00 pm";
console.assert(
currentOutput2 === targetOutput2,
`current output: ${currentOutput2}, target output: ${targetOutput2}`
);
);
Copy link

Choose a reason for hiding this comment

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

  1. When you executed this script, does you notice any error message (caused by a bug in your program)?
  2. You can add additional tests to check if your function can return the expected value when times are "00:12", "12:59".

Copy link
Author

@shreefAhmedM shreefAhmedM Dec 23, 2024

Choose a reason for hiding this comment

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

I Missed : between hours and minutes, so I have updated it.
All these passed.
{ input: "01:00", expected: "1:00 am" },
{ input: "12:00", expected: "12:00 pm" },
{ input: "23:59", expected: "11:59 pm" },
{ input: "15:45", expected: "3:45 pm" },
{ input: "08:30", expected: "8:30 am" },
{ input: "00:00", expected: "12:00 am" },
{ input: "20:15", expected: "8:15 pm" },

@@ -13,3 +13,7 @@
// Given someone's weight in kg and height in metres
// Then when we call this function with the weight and height
// It should return their Body Mass Index to 1 decimal place
function calc(weigh,height){
const bmi = weigh/(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.

What is the type of value you expect to be returned from this function? A number or a string? Does your function return the type of value you expected?

Copy link
Author

@shreefAhmedM shreefAhmedM Dec 23, 2024

Choose a reason for hiding this comment

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

I added return parseFloat(bmi.toFixed(1)) now will return a number

Comment on lines +11 to +18
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.

It is never too early to develop the habit to indent the code properly; it makes reading code easier.

function add_vat(price){
const vatRate = 1.2;
const vatIncPri = price * vatRate
return `£${vatIncPri.toFixed(2)}`
Copy link

Choose a reason for hiding this comment

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

If we wanted to use the return value in calculation (e.g., total = add_vat(pricd1) + price2) instead of for output purpose, how would you change the function?

@@ -9,23 +9,27 @@ function formatTimeDisplay(seconds) {
const totalHours = (totalMinutes - remainingMinutes) / 60;

return `${pad(totalHours)}:${pad(remainingMinutes)}:${pad(
remainingSeconds
remainingSeconds.
Copy link

Choose a reason for hiding this comment

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

Why add a . here (line 12)?

Copy link
Author

Choose a reason for hiding this comment

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

by mistake, while I'm pushing the code

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Dec 22, 2024
@shreefAhmedM
Copy link
Author

Code is good in general. I left some comments and suggestions.

There are many typos and misspelled words in the comments. You can try using ChatGPT to help you improve them.
ok, thank you

@shreefAhmedM shreefAhmedM added the Complete Participant to add when work is complete and review comments have been addressed label Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Participant to add when work is complete and review comments have been addressed Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants