London | 26-ITP-Jan | Johnny Vargas | Sprint 1 | Coursework#968
London | 26-ITP-Jan | Johnny Vargas | Sprint 1 | Coursework#968JohnnyBoyV wants to merge 19 commits intoCodeYourFuture:mainfrom
Conversation
…also adding a console.log to check it is all working
This comment has been minimized.
This comment has been minimized.
JohnnyBoyV
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
JohnnyBoyV
changed the title
JohnnyBoyV
added
the
Needs Review
hkavalikas
added
Review in progress
hkavalikas
added
Reviewed
There was a problem hiding this comment.
Awesome one, this is mostly correct, although less common, there is a case that can break this, what would happen if we have a file/directory name with a dot? Can we update the implementation slightly so we can handle that?
| const age = 33; | ||
| age = age + 1; | ||
|
|
||
|
|
There was a problem hiding this comment.
Spot on, let's take it a step further and fix the code itself too
| const cityOfBirth = "Bolton"; | ||
|
|
||
|
|
||
| // The 'ReferenceError' occurs because the code tries to use the variable cityOfBirth before it has been defined. |
There was a problem hiding this comment.
Same as the other file, let's also fix the code itself
|
|
||
|
|
||
| // I predict an error will happen because cardNumber is a number and not a string and slice can not be used for pure numbers. | ||
| // We can fix it by turning the number into a string by using .toString() before we slice it. |
There was a problem hiding this comment.
The solution is mostly correct, but there is an issue with typing. By doing toString(), we have now converted last4Digits from number to string, but it's not obvious that this is a String now, and we could mistakenly try to do numeric operations on it. Once we are done slicing the value, how can we convert it back to the original type?
| // 4, 5, 10. | ||
|
|
||
| // b) Run the code and identify the line where the error is coming from - why is this error occurring? How can you fix this problem? | ||
| // It is coming from line 5. Because ("," "") is missing a comma separating the strings. |
There was a problem hiding this comment.
Awesome, let's implement the comment and fix the code itself too
| // A better name for it could be movieLengthFormatted. | ||
|
|
||
| // f) Try experimenting with different values of movieLength. Will this code work for all values of movieLength? Explain your answer | ||
| // If the seconds are a single digit like "5", the result will look something like 2:26:5. It makes it look unprofessional compared to say 02:26:05. No newline at end of file |
There was a problem hiding this comment.
Good answer, can you try to spot a few more values/cases to test against?
JohnnyBoyV
added
Needs Review
|
Thanks, made the changes requested. |
2 participants
Learners, PR Template
Self checklist
Changelist
I followed the questions in each js & md file and commented answers or directly changed code as requested by the questions.