Glasgow | 25-ITP-SEP | Alaa Tagi | Sprint 1 | coursework/sprint-1 #723
Conversation
Alaa-Tagi
added
📅 Sprint 1
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good start on this sprint's tasks, I have spotted a few areas where you could improve answers further
| declaring a new variable priceAfterOneYear: | ||
| let priceAfterOneYear = "8,543"; | ||
| */ | ||
|
|
There was a problem hiding this comment.
These are correct - are they any other variables being declared?
There was a problem hiding this comment.
@LonMcGregor , priceDifference and precentageChange are also variables.
| /* | ||
| The variable result display the total length of the movie in these form (Hours:Minutes:seconds) in string format. | ||
| I can think of "MovieDuration" as a better name because it gives more description to the output. |
There was a problem hiding this comment.
Is it clear what the difference is between movieDuration and movieLength? Could I understand the difference by quickly reading these two variable names?
There was a problem hiding this comment.
@LonMcGregor
I think movieDuration it emphasizes the meaning of time span than movieLength.(I guess)
There was a problem hiding this comment.
@LonMcGregor what about (totalscreentime) ? is this more obvious ?
There was a problem hiding this comment.
@Alaa-Tagi They both show the total screen time. Think of what the difference is between the movie length variable and this new variable at the end of the code (what type of data is used, what content the data holds). What different type of data is stored there? How could you make it obvious what the difference is?
There was a problem hiding this comment.
Oh, I got what you meant now. Are you saying that the difference in datatype and representation for movie length is numeric data, while screen time is considered structured data? Is that correct? What about total_runtime_minutes?
There was a problem hiding this comment.
Yes, that's what I was thinking. Because your final argument is formatted to show hours, minutes, seconds, rather than total seconds as an integer, picking a name that makes that clear is better. It shows the runtime with more details than just minutes, so is there an even better name?
There was a problem hiding this comment.
I guess it can be ( runtime_hms) Is it still not better name?
There was a problem hiding this comment.
runtime_hms sounds good to me - this makes it clear that it contains all of hours, minutes and seconds.
LonMcGregor
added
Reviewed
Alaa-Tagi
added
Needs Review
|
If I was looking at that variable name, would it be obvious which one is the original input and which one had the formatted output? Is there a name you could use that would make that more obvious? |
LonMcGregor
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
I have made all the changes required and run the tests needed
Questions
No questions.