-
-
Notifications
You must be signed in to change notification settings - Fork 274
Manchester | 25-ITP-Sep | Khalidbih | Sprint 2 | coursework/sprint-2 #854
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). |
|
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). |
2 similar comments
|
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). |
|
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). |
cjyuan
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.
- This branch contains some unrelated changes in the Sprint-1 folder. Can you revert those changes to keep this branch clean?
| let BMI = weight / (height * height); | ||
| let rounded = Math.round(BMI * 10) / 10; |
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.
Did you notice the variables rounded and BMI are rendered in different colors?
Many IDEs and viewers that support syntax highlighting (including GitHub) display identifiers in different formats and colors.
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.
On separate note, it is a good practice to declare a "variable" using const instead of let when the variable is not going to be reassigned a value.
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.
Thank you! I have changed let to const and checked the variable naming.
| function toPounds (penceString) { | ||
|
|
||
|
|
||
| const penceStringWithoutTrailingP = penceString.substring ( | ||
| 0, | ||
| penceString.length - 1 | ||
| ); | ||
|
|
||
| const paddedPenceNumberString = penceStringWithoutTrailingP.padStart(3, "0"); | ||
| const pounds = paddedPenceNumberString.substring( | ||
| 0, | ||
| paddedPenceNumberString.length - 2 | ||
| ); | ||
|
|
||
| const pence = paddedPenceNumberString | ||
| .substring(paddedPenceNumberString.length - 2) | ||
| .padEnd(2, "0"); | ||
| return `£${pounds}.${pence}`; | ||
| } |
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.
The indentation of the code is a bit off.
Suggestion: You can use this VSCode feature to auto indent JS code.
https://code.visualstudio.com/docs/languages/javascript#_formatting
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.
Thank you for the link! I’ll make sure to keep the indentation consistent next time.
Deleted unrelated Sprint-1 files that were mistakenly included in the Sprint-2 branch to keep the PR clean.
Deleted unrelated Sprint-1 files that were mistakenly included in the Sprint-2 branch to keep the PR clean.
Deleted unrelated Sprint-1 files that were mistakenly included in the Sprint-2 branch to keep the PR clean.
Deleted unrelated Sprint-1 files that were mistakenly included in the Sprint-2 branch to keep the PR clean.
… for non-reassigned variables
… for non-reassigned variables
Sorry about the confusion! I’ve deleted the unrelated changes to keep this branch clean. |
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.
Changes to the code look good. However, the branch is still not quite clean.
Deleted files are still considered as "changed files". "Revert the changes made in the Sprint-1 folder" means to "undo the changes made in that folder. That is, we need to make the files in the Sprint-1 folder identical to their original version (the version in the main branch).
One way to achieve that is to replace the files by the version of the files in main.
.Sprint-1 files have been restored to main and removed from this PR to keep the changes focused and clean. |
|
Branch is clean now. Well done. |
Thank you so much for your guidance. I really appreciate it. |
Self checklist
Changelist
I completed all exercises in Sprint-2, except the last one, 5-stretch-extend/format-time.js.