-
-
Notifications
You must be signed in to change notification settings - Fork 237
Birmingham|25-ITP-SEP|Hadi Vahidi|Sprint 1|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 1|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). |
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.
// 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?
} | ||
return `${time} am`; | ||
} | ||
|
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.
Have you considered adding a test for midday (12:00 / 12pm)?
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>
>
|
Learners, PR Template
Self checklist
Changelist
I did complete the sprint 2
Questions
thank you very much