Conversation
|
I will review your PR after you have added task 2. |
📝 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.
Hi @pavel-tisner, everything works fine, but the implementation of both tasks could be improved. Please see my comments below.
| this.#secondsFromMidnight = hours * 3600 + minutes * 60 + seconds; | ||
| this.hours = hours; | ||
| this.minutes = minutes; | ||
| this.seconds = seconds; |
There was a problem hiding this comment.
It is not very clear which variable(s) represent the "single source of truth" of the time value. The class encapsulates the time as a number of seconds as well as an hours, minutes and seconds value. Moreover, the #secondsFromMidnight property is made private, while the hours, minutes and seconds properties are public despite the fact that there are "getter" methods for these values.
It would be clearer to have a single private property, #secondsFromMidnight, to represent the time value and compute the hours, minutes, and seconds on the fly in the "getter" methods.
| } | ||
|
|
||
| getHours() { | ||
| return this.hours; |
There was a problem hiding this comment.
Rather than maintaining a separate class property for the hours value, you can simply compute the value using integer arithmetic from the #secondsFromMidnight property.
| } | ||
|
|
||
| getMinutes() { | ||
| return this.minutes; |
| } | ||
|
|
||
| getSeconds() { | ||
| return this.seconds; |
| return this.seconds; | ||
| } | ||
|
|
||
| #normilized() { |
There was a problem hiding this comment.
Nitpick: spelling error -> normalized
| ${chalk.blue(result.question)} | ||
| ${result.answers.map((answer, i) => `${i + 1}. ${answer}`).join("\n")} | ||
| `); |
There was a problem hiding this comment.
To maintain correct indentation you could refactor this as:
| ${chalk.blue(result.question)} | |
| ${result.answers.map((answer, i) => `${i + 1}. ${answer}`).join("\n")} | |
| `); | |
| console.log( | |
| `\n${chalk.blue(result.question)}\n${result.answers | |
| .map((answer, i) => `${i + 1}. ${answer}`) | |
| .join("\n")}`, | |
| ); |
| import { BASE_PROMPT } from "./basePrompt.js"; | ||
| import {userAnswer, isValidAnswer} from "./utils.js" | ||
|
|
||
| const prompt = promptSync(); |
There was a problem hiding this comment.
A user who wants to break off the quiz should either be able to press Ctrl-C or you should provide a menu option (say, x) to exit the quiz prematurely. The sigint option below enables Ctrl-C.
| const prompt = promptSync(); | |
| const prompt = promptSync({ sigint: true }); |
| prompt("Press enter to start the game"); | ||
|
|
||
| const score = { | ||
| totalQuestions: 10, |
There was a problem hiding this comment.
This property is never mutated. It might as well have been a const, e.g.
const TOTAL_QUESTIONS = 10;| totalQuestions: 10, | ||
| currentQuestion: 1, | ||
| correctAnswers: 0, | ||
| wrongAnswers: 0, |
There was a problem hiding this comment.
This property is updated but never reported. It serves no purpose.
| }); | ||
|
|
||
| async function app() { | ||
| try { |
remarcmij
added
the
Reviewed
2 participants
No description provided.