2
\$\begingroup\$

I am writing a program that asks questions, takes the user's input, and responds according to whether the answer they gave is correct or not.

I am looking to improve my code with respect to cleanliness and readability.

const readline = require('readline');

const rl = readline.createInterface({ input: process.stdin, output: process.stdout });

rl.question('What is the sum of 2+5: ', (ans) => {
    if (ans == 7) {
        console.log('You are correct');
        rl.close();
    }
    else {
        rl.setPrompt('You are wrong. Try again\n');
        rl.prompt();
        rl.on('line', (ans) => {
            if (ans == 7) {
                console.log('You are correct');
                rl.close();
            }
            else { console.log('You are wrong. Try again');}
        });
    }
});
\$\endgroup\$
1
  • 3
    \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ Commented Oct 6, 2021 at 14:52

1 Answer 1

3
\$\begingroup\$

This code is WET (wrote everything twice) and hard to extend/reuse. What if you want to ask, say, 10 questions? How about 100? Following the original design would likely create a pyramid of doom.

We can DRY it out and flatten the pyramid with promises and a loop:

const readline = require("readline");

const rl = readline.createInterface({
  input: process.stdin,
  output: process.stdout
});
const askQuestion = q => new Promise(res => rl.question(q, res));

(async () => {
  for (;;) {
    const answer = await askQuestion("What is the sum of 2+5: ");

    if (answer === "7") {
      console.log("You are correct");
      rl.close();
      break;
    }

    rl.write("You are wrong. Try again\n");
  }
})();

Sample run:

What is the sum of 2+5: 6
You are wrong. Try again
What is the sum of 2+5: asdf
You are wrong. Try again
What is the sum of 2+5: 7
You are correct

Now it's easy to extend the code to support additional questions by moving the loop to a function askUntilCorrect and adding a data structure to store questions and answers:

const readline = require("readline");

const rl = readline.createInterface({
  input: process.stdin,
  output: process.stdout
});
const askQuestion = q => new Promise(res => rl.question(q, res));

const askUntilCorrect = async (question, answer) => {
  for (;;) {
    const response = await askQuestion(question);
    
    if (response === answer) {
      console.log("You are correct");
      return;
    }
    
    rl.write("You are wrong. Try again\n");
  }
};

(async () => {
  const questions = [
    {
      question: "What is the sum of 2+5: ",
      answer: "7",
    },
    {
      question: "What is the result of 22-43: ",
      answer: "-21",
    },
    {
      question: "What is the largest city in the USA? ",
      answer: "New York",
    },
    // ...
  ];

  for (const {question, answer} of questions) {
    await askUntilCorrect(question, answer);
  }

  rl.close();
})();

Sample run:

What is the sum of 2+5: 7
You are correct
What is the result of 22-43: -21
You are correct
What is the largest city in the USA? new york
You are wrong. Try again
What is the largest city in the USA? New York City
You are wrong. Try again
What is the largest city in the USA? NYC
You are wrong. Try again
What is the largest city in the USA? New York
You are correct

You can see there's still room for improvement -- it's poor user experience to basically get the correct answer but still be rejected by the program. The askUntilCorrect function can accept a validator callback so the caller can specify something other than === as the test for correctness:

const readline = require("readline");

const rl = readline.createInterface({
  input: process.stdin,
  output: process.stdout
});
const askQuestion = q => new Promise(res => rl.question(q, res));

const askUntilCorrect = async (
  question,
  answer,
  validator = (r, a) => r === a
) => {
  for (;;) {
    const response = await askQuestion(question);
    
    if (validator(response, answer)) {
      console.log("You are correct");
      return;
    }
    
    rl.write("You are wrong. Try again\n");
  }
};

(async () => {
  const questions = [
    {
      question: "What is the sum of 2+5: ",
      answer: "7",
    },
    {
      question: "What is the result of 22-43: ",
      answer: "-21",
    },
    {
      question: "What is the largest city in the USA? ",
      validator: (r, a) =>
        r.toLowerCase().includes("new york") || 
        r.toLowerCase() === "nyc"
    },
    // ...
  ];

  for (const {question, answer, validator} of questions) {
    await askUntilCorrect(question, answer, validator);
  }

  rl.close();
})();

Sample run:

What is the sum of 2+5: 7
You are correct
What is the result of 22-43: -21
You are correct
What is the largest city in the USA? new brunswick
You are wrong. Try again
What is the largest city in the USA? new york
You are correct

There are still hardcoded strings in the askQuestion function, so you can keep generalizing its behavior, but I'll call it good enough for now.

As an exercise, you might try extending this code to restrict the number of attempts for each question instead of prompting infinitely.


A few additional remarks on your code:

  • Good use of const!
  • (Minor) prefer 2 to 4 spaces indentation.
  • Make sure lines never exceed ~70 characters.
  • When you open a brace, generally don't put any more code on that line, as in else { console.log('You are wrong. Try again');}. Spread it over 3 lines.
  • Your use of == as an intentional coercion is pretty clear, but on principle I never use ==. It's just too hard to reason about, so you can't go wrong pretending it doesn't even exist. I prefer to explicitly convert the result with + and use ===, or use the string "7" to represent the answer. This way, no potentially surprising coercion is involved and it's easier to follow intent. It might seem silly here, but if you always avoid == it'll make life easier in the long run.
  • rl.write() seems more appropriate than rl.prompt() for displaying the error message.
  • Sometimes adding a return in an if can save your else from having to exist, and is typically cleaner for preconditions in a callback.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.