May-25_Glasgow|Shreef_Ibrahim|Module-Structuring-and-Testing-Data|CourseWork_sprint2_Week2 #455
Conversation
shreefAhmedM
requested review from
Ameneh-Keshavarz,
Ara225,
CalumNaughton,
alexandru-pocovnicu,
awadhufidh,
bashirzadehdanial and
cjyuan
shreefAhmedM
added
the
Needs Review
|
Please do not request for reviewers. We still have plenty of PRs in Module Onboarding to review. I am afraid the PRs in the second module will have to wait. Good job in preparing the PR description. You can probably further improve this PR by asking ChatGPT for second opinion or to check for grammar/spelling mistakes. |
|
Hi, I think I was tagged by mistake so I'll just go ahead and untag myself, all the best |
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Code is good! I left you some suggestions.
| // } | ||
|
|
||
| // =============> write your explanation here | ||
| // =============> SyntaxError: Identifier 'str' has already been declared |
There was a problem hiding this comment.
The instruction was "write your explanation here".
Can you write your explanation instead of just showing the error message produce by the runtime?
| const percentage = `${decimalNumber * 100}%`; | ||
| return percentage; | ||
| } | ||
| console.log(convertToPercentage("0.5")); // ====> 50% |
There was a problem hiding this comment.
Should the parameter be a number (0.5) or a string ("0.5")?
| return `The result of multiplying ${a} and ${b} is ${a * b}` | ||
| } | ||
|
|
||
| console.log(multiply(9, 6)); // ===> The result of multiplying 9 and 6 is 54 No newline at end of file |
There was a problem hiding this comment.
A function should only do one specific task.
- Calculate the product of two numbers (Yes)
- Return the result in a specially formatted string (Well, what if I want to format the result in different ways?)
Can you reimplement the function to improve its reusability?
| // return the BMI of someone based off their weight and height | ||
| } No newline at end of file | ||
| let bmi = weight /(height * height); | ||
| return bmi.toFixed(1) |
There was a problem hiding this comment.
What type of value do you expect the function to return? A number or a string?
Does your function return the type of value you expect it to return?
| // Use the MDN string documentation to help you find a solution | ||
| // This might help https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/toUpperCase | ||
| function toUpperSnakeCase(str){ | ||
| let SnakeCaseStr = str.replace(/ /g, "_") |
There was a problem hiding this comment.
It is more common practice to begin variable names with a lowercase letter. Names that start with an uppercase letter are typically reserved for user-defined data types or class names.
| const pounds = paddedPenceNumberString.substring(0,paddedPenceNumberString.length - 2); | ||
| const pence = paddedPenceNumberString.substring(paddedPenceNumberString.length - 2).padEnd(2, "0"); |
There was a problem hiding this comment.
You should take a look at .slice(). With this function, lines 11-12 can be rewritten as
const pounds = paddedPenceNumberString.slice(0, -2);
const pence = paddedPenceNumberString.slice(-2); // .padEnd() is redundant regardless of which function we use
| return `${hours - 12}:00 pm`; | ||
| const minutes = time.slice(3); | ||
| let currTime = "am"; | ||
| let HoursModefied = hours; |
There was a problem hiding this comment.
Should begin variable name with a lowercase letter.
GitHub even assign a different color to identifiers that begin with an uppercase letter:
| } | ||
| return `${time} am`; | ||
|
|
||
| const formattedHour = HoursModefied < 10 ? "0" + HoursModefied : HoursModefied.toString(); |
There was a problem hiding this comment.
There is a String function you can use to make this statement shorter (and clearer).
| const currentOutput3 = formatAs12HourClock("01:00"); | ||
| const targetOutput3 = "01:00 am"; | ||
| console.assert( | ||
| currentOutput3 === targetOutput3, | ||
| `current output: ${currentOutput3}, target output: ${targetOutput3}` | ||
| ); |
There was a problem hiding this comment.
"12:00" would make a good edge case to test.
Also, we should also test cases where minute part is not equal to "00".
cjyuan
added
Reviewed
4 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Her is JavaScript challenges tasks covering exercises, error fixing, code interpretation, and a stretch exploration activity.
Questions
Ask any questions you have for your reviewer.