-
-
Notifications
You must be signed in to change notification settings - Fork 218
London | 25-ITP-Sep | Imran Mohamed | Sprint 1 | Coursework/sprint 1 #726
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?
Conversation
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. 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 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. 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 answers further
// This should produce the string "CKJ", but you must not write the characters C, K, or J in the code of your solution. | ||
|
||
let initials = ``; | ||
let initials = `${ |
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 the string formatting ${}
for this if all you are doing is concatenation?
Sprint-1/1-key-exercises/3-paths.js
Outdated
|
||
const dir = ; | ||
const ext = ; | ||
const dir = filePath.slice(1, lastSlashIndex); |
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.
In filepaths, the first character /
is important to include - can you change this to ensure it is captured?
Sprint-1/2-mandatory-errors/3.js
Outdated
|
||
// Then try updating the expression last4Digits is assigned to, in order to get the correct value | ||
// by concatenating an empty string the code works as expected | ||
const cardNumber = "" + 4533787178994213; |
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.
This works, but it's maybe not the best practice as changing types in this way can get a bit confusing. Is there any other way to convert an integer to a string that makes it more obvious what is happening?
|
||
// f) Try experimenting with different values of movieLength. Will this code work for all values of movieLength? Explain your answer | ||
|
||
// Yes the code will work for all values of movieLength, because the use of a variable makes it so that the code can be reused for any value of movieLength |
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.
Can you find any edge cases where the formatting looks a bit odd?
I have made some changes following the comments on the PR review. |
Fantastic! You are finished with this sprint task now |
Self checklist
Changelist
I have completed the tasks set out in the files within the sprint-1 folder of this module. I have made a slight error in the path names I have used as commit messages for the tasks in the mandatory-interpret section