London | 26-ITP-Sep | Shirin Panahian | Sprint 2 | Complete Sprint 2 Coursework - #1545
shirinpanahian wants to merge 22 commits into
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Good work on the interpret section. All five answers in 1-percentage-change.js are right, including the declarations, which people often get wrong.
A few things to fix:
-
1-key-exercises/3-paths.jsline 16, see my comment. -
1-key-exercises/4-random.jsline 10, see my comment. -
3-mandatory-interpret/2-time-format.jsanswer f), see my comment. -
2-mandatory-errors/1.jsline 6, see my comment. -
Could you run Prettier on the files you have changed? Most of them are not formatted at the moment.
Add the Needs Review label again when you have pushed.
| const ext = ; | ||
|
|
||
| // https://www.google.com/search?q=slice+mdn No newline at end of file | ||
| const dir = filePath.slice(0,`${lastSlashIndex}`-1); |
There was a problem hiding this comment.
Run this file and read the dir line carefully. It prints .../week-1/interpre, and the folder is called interpret. One character is being lost. What is the - 1 doing at the end of this line, and does slice need it?
There was a problem hiding this comment.
The - 1 makes the slice stop one character too early, which removes the t from interpret. It isn’t needed. thank you for mention .
There was a problem hiding this comment.
That's it, the dir line prints the full folder name now.
| //num represents a random whole number between 1 and 100, | ||
| //first we calculate value inside parentheses(maximum - minimum +1) = 100 -1 + 1 =100 | ||
| // math.random return number between 0 and 1 | ||
| // math.random() * 100 give us a integer number |
There was a problem hiding this comment.
Have a look at this line again. Try running console.log(Math.random() * 100) a few times. Is what you get back an integer? If it were, what would be left for Math.floor to do on the next line?
There was a problem hiding this comment.
Yeah I write wrong, Math.random() * 100 does not give us an integer. It gives us a decimal number between 0 and 100 for example 0.83488 *100 = 83.488. This is why we use Math.floor() . Math.floor(83.488) rounds the number down to 83. Then we add 1, giving us 84.
There was a problem hiding this comment.
Yes, exactly that.
| // another name could be movieTime | ||
|
|
||
| // f) Try experimenting with different values of movieLength. Will this code work for all values of movieLength? Explain your answer | ||
| // This code works for normal positive number representing but for negative number and very big number it is not working No newline at end of file |
There was a problem hiding this comment.
Negative numbers are right. A very big number is fine though, try 999999 and see. Two things worth trying instead: 59, and a number with a decimal like 90.5. What does each one print, and would you show a time that way?
There was a problem hiding this comment.
The code work for big number, I try 999999 and it return correct number. However, if movieLength = 90.5, it prints 0:1:30.5. This is not a good way to show a time because the seconds contain decimals. So the code works properly for whole numbers, but not for negative numbers and decimal numbers because the result can contain decimal values.
There was a problem hiding this comment.
Good. 59 is worth a try too: it prints 0:0:59, which is the other thing that looks wrong.
| let age = 33; | ||
| age = age + 1; | ||
| console.log(`${age}`); | ||
| //We should use let if we want to change the value. No newline at end of file |
There was a problem hiding this comment.
Your fix is right. In 2.js, 3.js and 4.js you also wrote down the message node printed, which is what this section asks for. What did node say here before you changed const to let?
There was a problem hiding this comment.
Before I changed const to let node show this error message: (Assignment to constant variable.)
abdishakoor-dev
left a comment
There was a problem hiding this comment.
All four sorted, and dropping the big-number claim in f) was the right call.
One thing left before I mark this Complete: Prettier still flags three files, 1-key-exercises/2-initials.js and both markdown files in 4-stretch-explore. You clearly ran it on the files you were editing, these are the ones that got missed.
Rather than chasing them file by file, turn on format on save. Then every file you touch is formatted when you hit save and this stops coming up in future PRs: https://github.com/CodeYourFuture/Module-JavaScript-Fundamentals/blob/main/practical_guide.md
There is also a note inline on 3-paths.js, not a blocker.
Add the Needs Review label again once you have pushed.
| const ext = ; | ||
|
|
||
| // https://www.google.com/search?q=slice+mdn No newline at end of file | ||
| const dir = filePath.slice(0, `${lastSlashIndex}`); |
There was a problem hiding this comment.
The - 1 is gone and the folder name is complete now, good. Why the backticks around lastSlashIndex though? It is already a number, so this turns it into a string and slice converts it straight back.
There was a problem hiding this comment.
remove unnecessary backtick
There was a problem hiding this comment.
Backticks gone, and the line reads clearly now.
abdishakoor-dev
left a comment
There was a problem hiding this comment.
All fixed, marking this as complete, well done. Format on save will save you this round in every sprint from here.

Learners, PR Template
Self checklist
Task code
CYF-1039
Changelist
I solve and complete every 4 section and understand how write the java code and find the error.