-
Notifications
You must be signed in to change notification settings - Fork 18
Diana C #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Diana C #3
Conversation
📝 HackYourFuture auto gradeAssignment Score: 0 / 100 ✅Status: ✅ Passed Test Details |
| const cleanBook = bookName.toLowerCase(); | ||
|
|
||
| const containWord = cleanBook.includes(cleanSearch); | ||
| return containWord; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You created clear variables with easy to read logics, well done!
But what if I provide an empty string? Or what if I call the isBookApplicable function without a parameter?
console.log(isBookApplicable(""));
console.log(isBookApplicable());
That's why, in software engineering, it's very important to first consider all the requirements and error cases that need to be handled ("Error Handling").
| const datePart = dateString.slice(4); // "10-21-1983" | ||
|
|
||
| const first = Number(datePart.slice(0, 2)); | ||
| const second = Number(datePart.slice(3, 5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable names first and second are not clear enough. It would be hard for future developers to tell what is first and what is second at first glance.
| // convertDaysToHours, | ||
| // convertHoursToDays, | ||
| // convertMinutesToSeconds, | ||
| // convertSecondsToMinutes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to avoid using excessive comments.
|
|
||
| export function convertSecondsToMinutes(seconds) { | ||
| return seconds / 60; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You implemented all the requirements, well done!
| if (celsius < 0) return "Status: Freezing"; | ||
| if (celsius < 10) return "Status: Cold"; | ||
| if (celsius < 20) return "Status: Mild"; | ||
| if (celsius < 30) return "Status: Warm"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactoring of status logic 👏
| console.log(getStatusCelsius(celsius)); | ||
| printWindChill(cityNameValue, celsius, windSpeed); | ||
| console.log("---"); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your refactoring is incomplete. You also need to call the printWeatherReport with parameters. Did you manage to run this code? Because when you run, you should have seen that there are some errors that you need to resolve. But you're almost there. And keep in mind that when you run your logic, the output should be the same as the original file.
No description provided.