-
-
Notifications
You must be signed in to change notification settings - Fork 241
Birmingham|25-ITP-SEP|Hadi Vahidi|Sprint 2|coursework/sprint-2 #764
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?
Birmingham|25-ITP-SEP|Hadi Vahidi|Sprint 2|coursework/sprint-2 #764
Conversation
|
Your PR's title didn't contain a known region. Please check the expected title format, and make sure your region is in the correct place and spelled correctly. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR's title didn't contain a known region. Please check the expected title format, and make sure your region is in the correct place and spelled correctly. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
1 similar comment
|
Your PR's title didn't contain a known region. Please check the expected title format, and make sure your region is in the correct place and spelled correctly. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
LonMcGregor
left a comment
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.
Good start on this sprint's tasks, I have spotted a few areas where you could improve code further
Did you mean to commit the sprint 1 prep files? We try to keep pull requests specific to just the files in the feature we are changing - for this, everything within the sprint task directory.
Sprint-2/2-mandatory-debug/0.js
Outdated
| // I will add a return statement to the function multiply that returns the result of multiplying a and b. | ||
|
|
||
| function multiply(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.
Do you need to surround this with brackets?
|
Hi Lon,
Thanks for your feedback on my pull request. I appreciate you pointing out
the areas for improvement.
Regarding your first comment, I did not intend to include the sprint 1 prep
files in this pull request. I will make sure to keep future pull requests
specific to the relevant sprint task directory.
Thanks,
Hadi
…On Wed, Oct 15, 2025, 11:13 AM LonMcGregor ***@***.***> wrote:
***@***.**** commented on this pull request.
Good start on this sprint's tasks, I have spotted a few areas where you
could improve code further
Did you mean to commit the sprint 1 prep files? We try to keep pull
requests specific to just the files in the feature we are changing - for
this, everything within the sprint task directory.
------------------------------
In Sprint-2/2-mandatory-debug/0.js
<#764 (comment)>
:
>
// Finally, correct the code to fix the problem
// =============> write your new code here
+// I will add a return statement to the function multiply that returns the result of multiplying a and b.
+
+function multiply(a, b) {
+ return (a * b);
Do you need to surround this with brackets?
------------------------------
In Sprint-2/Prep/12hours.js
<#764 (comment)>
:
> @@ -0,0 +1,27 @@
+function formatAs12HourClock(time) {
+ if (Number(time.slice(0, 2)) > 12) {
+ return `${Number(time.slice(0, 2)) - 12}:00 pm`;
+ }
+ return `${time} am`;
+}
+
Have you considered adding a test for midday (12:00 / 12pm)?
—
Reply to this email directly, view it on GitHub
<#764 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BNQVX5RU2VZAWSNCK6JJZID3XYM27AVCNFSM6AAAAACJCEIEACVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGMZZGU2DIOBZHE>
.
You are receiving this because you authored the thread.Message ID:
<CodeYourFuture/Module-Structuring-and-Testing-Data/pull/764/review/3339544899
@github.com>
|
|
Hi Lon,
Thanks for your feedback on my pull request. I appreciate you pointing out
the areas for improvement.
Regarding your first comment, I did not intend to include the sprint 1 prep
files in this pull request. I will make sure to keep future pull requests
specific to the relevant sprint task directory.
Regarding the `multiply` function in Sprint 2, you are correct; I can
simply write `return a * b;` without the need for brackets around `a * b`.
I will update the code to reflect this more concise syntax.
Thanks,
Hadi
…On Wed, Oct 15, 2025, 1:28 PM Hadi Vahidi ***@***.***> wrote:
Hi Lon,
Thanks for your feedback on my pull request. I appreciate you pointing out
the areas for improvement.
Regarding your first comment, I did not intend to include the sprint 1
prep files in this pull request. I will make sure to keep future pull
requests specific to the relevant sprint task directory.
Thanks,
Hadi
On Wed, Oct 15, 2025, 11:13 AM LonMcGregor ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
>
> Good start on this sprint's tasks, I have spotted a few areas where you
> could improve code further
>
> Did you mean to commit the sprint 1 prep files? We try to keep pull
> requests specific to just the files in the feature we are changing - for
> this, everything within the sprint task directory.
> ------------------------------
>
> In Sprint-2/2-mandatory-debug/0.js
> <#764 (comment)>
> :
>
> >
> // Finally, correct the code to fix the problem
> // =============> write your new code here
> +// I will add a return statement to the function multiply that returns the result of multiplying a and b.
> +
> +function multiply(a, b) {
> + return (a * b);
>
> Do you need to surround this with brackets?
> ------------------------------
>
> In Sprint-2/Prep/12hours.js
> <#764 (comment)>
> :
>
> > @@ -0,0 +1,27 @@
> +function formatAs12HourClock(time) {
> + if (Number(time.slice(0, 2)) > 12) {
> + return `${Number(time.slice(0, 2)) - 12}:00 pm`;
> + }
> + return `${time} am`;
> +}
> +
>
> Have you considered adding a test for midday (12:00 / 12pm)?
>
> —
> Reply to this email directly, view it on GitHub
> <#764 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BNQVX5RU2VZAWSNCK6JJZID3XYM27AVCNFSM6AAAAACJCEIEACVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGMZZGU2DIOBZHE>
> .
> You are receiving this because you authored the thread.Message ID:
> <CodeYourFuture/Module-Structuring-and-Testing-Data/pull/764/review/3339544899
> @github.com>
>
|
and update the function to calculate the minute
|
Good work on the changes so far. Can you figure out how to remove the sprint 1 files from this PR? If you can fix that, I'll mark this as complete |
|
Thanks, Lon! I’ve now removed the Sprint 1 files from this pull request, so it only includes the Sprint 2 work. Please let me know if it looks good now. |
|
Fantastic, this looks complete now! |
Learners, PR Template
Self checklist
Changelist
I did complete the sprint 2
Questions
thank you very much