Glasgow | 25-ITP-SEP | Mohammed Abdoon | Sprint 2 | Coursework#725

Closed

M-Abdoon wants to merge 14 commits intoCodeYourFuture:mainfrom

M-Abdoon:coursework/sprint-2

M-Abdoon commented Oct 1, 2025 •

edited

Loading

Learners, PR Template

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Changelist

I have completed all the exercises and worked through errors, debugging, implementations, interpretations, and practicing handling time formats.

M-Abdoon added 6 commits

September 24, 2025 14:22


          �[200~Variable declared twice fixed

6959f9a


          exercise 1.2 done

8472bf4


          1-key-errors Done

80d9aff


          2-mandatory-debug Done

4ccd588


          3-mandatory-implement Done

a6c691a


          4-mandatory-interpret Done

3edc12c

github-actions bot commented Oct 1, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

2 similar comments

github-actions bot commented Oct 1, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

github-actions bot commented Oct 1, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

M-Abdoon added the Needs Review label

github-actions bot commented Oct 1, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

1 similar comment

github-actions bot commented Oct 1, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

M-Abdoon changed the title ~~Glasgow | 25-ITP-SEP | Mohammed Abdoon | Sprint 2 | coursework/sprint-2~~ Glasgow | 25-ITP-SEP | Mohammed Abdoon | Sprint 2 | Coursework

github-actions bot commented Oct 3, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

M-Abdoon added 3 commits

October 5, 2025 18:25


          Format-time exercise Done

c8fe831


          minutes recognition added, exercice 5 Done

be15364


          Exercise 5 done

5b3cd1e

cjyuan reviewed

View reviewed changes

Contributor

cjyuan left a comment •

edited

Loading

Code is clean and clear. I just have a few suggestions.

Regarding the PR description, the Self-checklist should be a list of items with checkboxes; you removed the boxes. (Look up "how to represent checked boxes in Markdown")

Sprint-2/3-mandatory-implement/1-bmi.js Outdated

Comment on lines 20 to 22

+                  let BMI = weight/squaredHeight;
+                  BMI = BMI.toFixed(1);
+                  return Number(BMI);

Contributor

cjyuan Oct 12, 2025

Did you notice the variables squaredHeight and BMI are rendered in different colors?

Many IDEs and viewers that support syntax highlighting (including GitHub) display identifiers in different formats and colors. Can you find out why?

let bmi, camelCase;
let Bmi, PascalCase;
let BMI, UPPER_SNAKE_CASE;

Can you look up the naming conventions in JavaScript? In particular,

Variable and function names
Class and Types names
Named constants

Then, update the variable names according to those conventions.

Sprint-2/4-mandatory-interpret/time-format.js Outdated

Comment on lines 32 to 33

		// # The Python visualizer stops working when it runs pad because (padStart is not a function), maybe because it uses ES6 version of javascript!
		// The return value of pad when it's called for the first time is 00

Contributor

cjyuan Oct 12, 2025

A common convention for indicating that a value is a string is to enclose it in double quotes. For example, "00".

Sprint-2/5-stretch-extend/format-time.js Outdated

Comment on lines 39 to 40

		currentOutput = formatAs12HourClock("00:37");
		targetOutput = "00:37 am";

Contributor

cjyuan Oct 12, 2025

Can you look up the 12-hour clock equivalents of the 24-hour times "00:37" and "12:37"?

cjyuan added Reviewed and removed Needs Review labels

M-Abdoon added 3 commits

October 13, 2025 00:13


          Changed name of function calculateBmi and var bmi to camelCase

f3c3be7


          the 00 output will be a string not a number so it is

fa69127


          fixed a bug in the function, should return 12:37. and should return

eb8e26b

M-Abdoon added the Needs Review label

cjyuan reviewed

View reviewed changes

Contributor

cjyuan left a comment •

edited

Loading

Changes look good.

Please note that in CYF courses, the recommended way to inform the reviewer of your changes is to do both of the following:

Reply to their feedback.
- In the responses, clarify how each piece of feedback was addressed to demonstrate that you've carefully reviewed the suggestions.
  - You may find the suggestions in this PR Guide useful.
- Your response may trigger a notification (depending on the reviewer's settings), helping ensure they’re aware of the updates you’ve made.
Replace the "Reviewed" label by a "Needs review" label (which you have done -- great!)
- Without this label, the reviewer would not know if your changes is ready to be reviewed.

Sprint-2/5-stretch-extend/format-time.js Outdated

Comment on lines 6 to 20

+                let hours = time.slice(0, 2);
+                let minutes= time.slice(3, 5);
+                let timeIndicator;
+                if (Number(hours) >= 12) {
+                  hours = String(hours - 12);
+                  hours = hours.padStart(2, "0");
+                  timeIndicator = "pm";
+                } else {
+                  timeIndicator = "am";
                 }
-                return `${time} am`;
+                if ( hours === "00")
+                  hours = "12";
+                return `${hours}:${minutes} ${timeIndicator}`;

Contributor

cjyuan Oct 13, 2025

On lines 10-11: Mixing numbers and strings can be dangerous.

A better approach could be:

Convert hours to a number once at the beginning
Carry out calculation using only numbers in the function body (no type conversion needed)
At line 20, call .padStart() once to format hours as a 2-digit string.

cjyuan added Complete and removed Needs Review Reviewed labels

M-Abdoon added 2 commits

October 15, 2025 19:16


          Set the variable hours as a number then converted it into a string wh…

f4b2947

…en printing


          updated wrong if confition

3d1b3c8

Author

M-Abdoon commented Oct 15, 2025

Changes look good.

Please note that in CYF courses, the recommended way to inform the reviewer of your changes is to do both of the following:

Reply to their feedback.

In the responses, clarify how each piece of feedback was addressed to demonstrate that you've carefully reviewed the suggestions.

You may find the suggestions in this PR Guide useful.

Your response may trigger a notification (depending on the reviewer's settings), helping ensure they’re aware of the updates you’ve made.

Replace the "Reviewed" label by a "Needs review" label (which you have done -- great!)

Without this label, the reviewer would not know if your changes is ready to be reviewed.

Ok, I do that for the next PL reviews, thank you so much.

illicitonion closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels