LONDON | May-2025 | Anuar Duisenbek | Module-Structuring-and-Testing-Data | Sprint-2 #559
Conversation
rohan-corbyn
added
Review in progress
| @@ -1,13 +1,29 @@ | |||
| // Predict and explain first... | |||
| // =============> write your prediction here | |||
| // | |||
| // I predict we will make mistake because str has been already declared in body of function | |||
|
|
|||
There was a problem hiding this comment.
Yes: The variable "str" is declared in the function signature as a parameter, it is then illegally redeclared in the body of the same function.
| // Finally, correct the code to fix the problem | ||
| // =============> write your new code here | ||
| /* |
There was a problem hiding this comment.
This is great but I think you left your code commented out.
|
|
||
| function makeAllCaps(text) { | ||
| return text.toUpperCase().split(" ").join("_"); | ||
| } |
There was a problem hiding this comment.
This works functionally but it could be improved regarding it's time and space complexity.
You are creating a new array when you call split() on a string. Imagine the input String was the size of a chapter in "lord of the rings" and you called split(" ") on the string? The array would be very large and use up memory.
You could instead, use a different single method that replaces characters without using split() and join().
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/split
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace
| @@ -4,3 +4,22 @@ | |||
| // You will need to declare a function called toPounds with an appropriately named parameter. | |||
|
|
|||
| // You should call this function a number of times to check it works for different inputs | |||
|
|
|||
There was a problem hiding this comment.
The function only logs the result. It should ideally return the formatted string to make it "reusable" in other areas of your program. For example: when using the function in the rest of the program you might want to pass the result into another function instead of just logging it.
| if (hours > 12) { | ||
| return `${hours - 12}:00 pm`; | ||
| const newHours = String(hours - 12).padStart(2, "0"); | ||
| return `${newHours}:${minutes} pm`; | ||
| } | ||
| return `${time} am`; | ||
| } |
There was a problem hiding this comment.
You didn't write any new tests that provide coverage for the issues you found in the code.
Please also include them. In TDD we write tests first and write the code later. It's a common strategy for ensuring requirements and edge-cases are considered before time is spent solving the problem. Your tests should initially all fail before you start working on the code to make them pass.
AnuarBey
changed the title
AnuarBey
changed the title
Self checklist
Changelist
-Fixed bugs in
1-key-errorsand2-mandatory-debug-Implemented functions in
3-mandatory-implement(BMI, unit conversions)-Interpreted code and added explanation in
4-mandatory-interpret/time-format.jsCompleted stretch task
5-stretch-extend/format-time.jsQuestions
Ask any questions you have for your reviewer.