1
\$\begingroup\$

The scenario here is to get a filename from the console and create it if it does not exist. I have two main questions;

  1. Is the way I used promises the best way?
  2. is there a better way?

Here is my code;

const argv = require("yargs").argv;
const fs = require("fs");
const readline = require("readline");

function ifFileExists(fileName) {
  return new Promise((resolve, reject) => {
    fs.readFile("array.txt", function (err, arrayData) {
      if (err) {
        if (err.code === "ENOENT") {
          handleWhenArrayFileNotFound(reject, resolve);
        } else {
          reject("file read error");
        }
      }

      if (arrayData) {
        handleWhenArrayExists(arrayData, resolve, fileName);
      }
    });
  });

  function handleWhenArrayFileNotFound(reject, resolve) {
    let content = fileName;
    content += "\n";
    fs.writeFile("array.txt", content, (error) => {
      if (error) {
        console.log("Error occured");
        reject("file write error");
      }
      rl.close();
      resolve("created");
    });
  }

  function handleWhenArrayExists(arrayData, resolve, fileName) {
    if (fileNamePresentInArray(arrayData, fileName)) {
      askForNewName("File already exists, Please provide a new filename:=>");
    } else {
      resolve("create file");
    }
  }
}

function fileNamePresentInArray(arrayData, fileName) {
  var array = arrayData.toString().split("\n");
  return array.includes(fileName);
}

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

function askForNewName(message) {
  rl.question(message, (fileName) => {
    fs.readFile("array.txt", function (err, arrayData) {
      if (err) {
        console.log("array.txt not found");
      }
      if (arrayData) {
        if (fileNamePresentInArray(arrayData, fileName)) {
          askForNewName(
            "File already exists, Please provide a new filename:=>"
          );
        } else {
          writeToFile(fileName);
          rl.close();
        }
      }
    });
  });
}

function askForUserInput(message) {
  rl.question(message, (fileName) => {
    ifFileExists(fileName)
      .then((res) => {
        writeToFile(fileName, res);
      })
      .catch((err) => {
        console.log(err);
      });
  });
}

function writeToFile(fileName, data) {
  if (data !== "created") {
    let content = fileName;
    content += "\n";
    fs.appendFile("array.txt", content, (err) => {
      if (err) console.log(err);
    });
  }

  fs.writeFile(fileName, "You are awesome", (err) => {
    if (err) {
      console.log("Error occured");
    }
  });
}

if (argv._[0] == "write") {
  askForUserInput("Please provide the filename:=>");
} else {
  console.log("No write operation");
}
\$\endgroup\$
1
  • \$\begingroup\$ I have updated based on your comments the moment put the above comment. but, don't know how many of those "veto" guys actually read the question. \$\endgroup\$
    – Safnas
    Feb 8, 2022 at 15:09

1 Answer 1

3
\$\begingroup\$

A short review;

The major problem with this code is that the function names lie all the time.

  • askForUserInput does so much more than asking for user input on the one hand and then does not even return the input on the other hand
  • writeToFile both writes to a file and appends to another file instead
  • askForNewName does also so much more than asking for a new name
  • fileNamePresentInArray actually checks whether the fileName is in a string, not an array ;)
  • Here, arrayData does not even contain an array
  • A special call out to rl.close(); which is so hidden that it makes me think of where is Waldo

This makes the logic so hard to follow, that I am still not 100% sure what this code does. I have some more code review suggestions, but in real life, I would push for a complete rewrite.

Also this

function writeToFile(fileName, data) {
  if (data !== "created") {
    let content = fileName;
    content += "\n";
    fs.appendFile("array.txt", content, (err) => {

could be this

function writeToFile(fileName, data) {
  if (data !== "created") {
    let content = fileName + "\n";
    fs.appendFile("array.txt", content, (err) => {

or could be this

function writeToFile(fileName, data) {
  if (data !== "created") {
    fs.appendFile("array.txt", fileName + "\n", (err) => {

The third version tells me immediately that we write the filename to the file

Also, don't write fat arrow functions that call another function with the provided parameters. In other words this

  .catch((err) => {
    console.log(err);
  });

should be this

  .catch(console.log);

To be sure, at some point if this code grows, I would suggest using logging levels and quiet mode.

A minor thing, but array.txt should be a constant, and is not an evocative filename.

\$\endgroup\$
2
  • \$\begingroup\$ thanks for reviewing the code. I got your point on improving the method naming standards etc. but if you could run the program once, you would have understood what it does. I was looking more on if the usage of promises is fine here. thanks anyways \$\endgroup\$
    – Safnas
    Feb 8, 2022 at 15:06
  • 1
    \$\begingroup\$ No worries. To be sure, it's about the naming, its about separating concerns in your code. As for the promise, it is fine within the context of this program. I would love to review this code in a new question where you fixed the code by addressing the separation of concerns (input/output/logic/processing/display) \$\endgroup\$
    – konijn
    Feb 10, 2022 at 7:52

Your Answer

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

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