Conversation
📝 HackYourFuture auto gradeAssignment Score: 0 / 100 ✅Status: ✅ Passed Test Details |
remarcmij
left a comment
remarcmij
left a comment
There was a problem hiding this comment.
Thanks @Yusuprozimemet for your PR. I have some comments and suggestions for you to consider, see below.
| if (normalizedTitle.includes(normalizedSearch)) { | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The .includes() method already returns a boolean. No need for an if statement.
| if (normalizedTitle.includes(normalizedSearch)) { | |
| return true; | |
| } else { | |
| return false; | |
| } | |
| return normalizedTitle.includes(normalizedSearch); |
| if (dateString.startsWith("MDY")) { | ||
| return { | ||
| day: Number(dateString.substring(7, 9)), | ||
| month: Number(dateString.substring(4, 6)), |
There was a problem hiding this comment.
You are making assumptions about the exact format of the date strings. If, for instance, the function is called without leading zeroes, the output will be wrong:
console.log(parseDateString('MDY 1-21-1983'));
console.log(parseDateString('DMY 21-10-1983'));
console.log(parseDateString('MDY 3-15-2024'));
console.log(parseDateString('DMY 15-03-2024'));{ day: NaN, month: NaN, year: 983 }
{ day: 21, month: 10, year: 1983 }
{ day: NaN, month: NaN, year: 24 }
{ day: 15, month: 3, year: 2024 }
It would be better to use .split('-') to split the date part of the date part of the date string into its individual components.
| year: Number(dateString.substring(10, 14)) | ||
| }; | ||
| } else { | ||
| return { |
There was a problem hiding this comment.
You are making the (untested) assumption here that it must be DMY if it is not MDY. This happens to be the case for the example console.logs, but may not be true in the general case.
|
|
||
| export function convertSecondsToMinutes(seconds) { | ||
| return seconds / 60; | ||
| } No newline at end of file |
| @@ -0,0 +1,80 @@ | |||
| // Temperature conversion and weather report for City 1 | |||
There was a problem hiding this comment.
This code is best left out of a PR. This not code the reviewer needs/wants to see.
| let cityName5 = "Edinburgh"; | ||
| let tempCelsius5 = 12; | ||
| let windSpeed5 = 18; | ||
|
|
There was a problem hiding this comment.
It would be better if you used an array of objects to capture this data, e.g. like this:
const cities = [
{ name: 'Amsterdam', tempCelsius: 22, windSpeed: 15 },
{ name: 'Berlin', tempCelsius: 15, windSpeed: 20 },
{ name: 'Copenhagen', tempCelsius: -5, windSpeed: 25 },
{ name: 'Dublin', tempCelsius: 8, windSpeed: 10 },
{ name: 'Edinburgh', tempCelsius: 12, windSpeed: 18 },
];| console.log("---"); | ||
| } | ||
|
|
||
| console.log(displayWeatherReport(cityName1, tempCelsius1, windSpeed1)); |
There was a problem hiding this comment.
Your displayWeatherReport() function does not return anything. Therefore, this console.log will first call displayWeatherReports() which does all the console logging itself, and then print undefined, because that is what displayWeatherReport() returns.
| console.log(displayWeatherReport(cityName2, tempCelsius2, windSpeed2)); | ||
| console.log(displayWeatherReport(cityName3, tempCelsius3, windSpeed3)); | ||
| console.log(displayWeatherReport(cityName4, tempCelsius4, windSpeed4)); | ||
| console.log(displayWeatherReport(cityName5, tempCelsius5, windSpeed5)); No newline at end of file |
There was a problem hiding this comment.
If you captured the city data in an array as I suggested above, you could now simply print the weather reports like this:
for (const city of cities) {
displayWeatherReport(city.name, city.tempCelsius, city.windSpeed);
}Apart from these comments, the cleanup of the code looks good.
remarcmij
added
the
Reviewed
2 participants
No description provided.