0

I am writing a simple URL shortening web app to learn JavaScript, Node (focusing on Express), and Mongo. A user submits a URL to my web app and receives a shortened URL (if the submitted URL is a valid URL) or an error message (if the submitted URL is not a valid URL). I have implemented the following algorithm:

  1. Check if the submitted URL is a valid URL (using dns.
  • 1.1 If it is not a valid URL, send a JSON response {"error":"invalid URL"}
  • 1.2 If it is a valid URL, check if there is already a shortened version of that URL in the database
    • 1.2.1 If there is, send a JSON response with the existing shortened URL
    • 1.2.2 If there isn't, make a new shortened URL, save it in the database, and send a JSON response with the newly created shortened URL

The JavaScript code is below. I believe it works (based on some manual testing), but I have the following questions:

  • Question 1: To perform steps 1, 1.2, and 1.2.2 from the above algorithm in that order, I found myself nesting multiple function definitions as callbacks in multiple function calls and I ended up with 3 or more levels of nesting. I did this because I needed one function call to return before I start the next step (e.g., I needed to know whether the URL is a valid URL before I could do further processing with it). Is nesting so many functions within each other an appropriate/normal programming style in these kinds of web apps? I found writing this code confusing/counter-intuitive. Is there a better way to write this code and avoid all this nesting?
  • Question 2: I am not sure if StackOverflow is the best place to get feedback on code style (if you are a new to a programming language) but if it is not, does anyone know of an online community where I could receive this kind of feedback (basically, a quick code review from another human being)?
app.post("/api/shorturl", handleShortenURL);

function handleShortenURL(req, res) {
  console.log("Given URL: " + req.body.url);
  // Check if the provided URL is valid and return otherwise. dns.lookup accepts hostnames without the protocol prefix,
  // so if there is a protocol prefix in the URL, it needs to be removed  
  const REPLACE_REGEX = /^https?:\/\//i;
  let url = req.body.url.replace(REPLACE_REGEX, '');
  dns.lookup(url, function onLookup(err) {  
    if (err)
    {
      console.log("err.code = " + err.code);
      res.json({"error":"invalid URL"});
    }
    else  // The provided URL is a valid URL
    {
      // It the URL is already in the database, don't add it again
      URLModel.find({ original: url }, function (err, docs) {
        if (err)
          return console.log(err);
        
        if (Object.keys(docs).length > 0)
          res.json({original_url: req.body.url, short_url: docs[0].shortened});
        else
        {
          URLModel.find().exec(function (err, results) {
            let count = results.length;
            var urlDocument = new URLModel({original: url, shortened: count});
            urlDocument.save(function(err, data) {
              if (err)
                console.error(err);
            });

            res.json({original_url: req.body.url, short_url: count});
          }); 
        }
      });
    }
  });
  
}

This question is at a high-level similar to mine, but the specific approach for addressing it proposed by @OlivierKrull is somewhat different (it uses async/await along with Promises) and I find it easier to follow and, perhaps, a bit more elegant than the approach proposed at the above link.

Steph
  • 27
  • 6
  • 1
    regarding your question about another place for reviews there is one [here](https://codereview.stackexchange.com/) – Getter Jetter Jul 28 '20 at 22:11
  • The basic idea is to use Promise and async/await –  Jul 28 '20 at 22:15
  • Does this answer your question? [Avoiding callback hell in node](https://stackoverflow.com/questions/49603355/avoiding-callback-hell-in-node) – Sᴀᴍ Onᴇᴌᴀ Jul 28 '20 at 22:19
  • 1
    Thank you @OlivierKrull for the code review suggestion. It seems like what I was looking for! – Steph Jul 31 '20 at 15:43
  • Thank you @ChrisG for the high-level idea. – Steph Jul 31 '20 at 15:49
  • And thank you @SᴀᴍOnᴇᴌᴀ for your suggestion. I think it also shows the right high-level idea. Olivier Krull's answer is more specific to my question and using a different style (async/await), which from what I read is a higher level of abstraction than directly using promises (please, correct me if I am wrong on that). – Steph Jul 31 '20 at 15:50

2 Answers2

1

In order to omit nested callbacks like this you can use async/await.

mongoDB queries do return promises, so you can easily await the queries to resolve.

Not sure if dns.lookup returns a promise but if it does you could also just use await there.

I simplified your code and didn't include the error handling. But it should give you an idea of your possibilities.

app.post("/api/shorturl", handleShortenURL);

function handleShortenURL(req, res) {
  const REPLACE_REGEX = /^https?:\/\//i;
  const url = req.body.url.replace(REPLACE_REGEX, '');
  dns.lookup(url, async function onLookup(err) { 

    if (err) {
      console.log("err.code = " + err.code);
      return res.json({"error":"invalid URL"});
    }

    const docs = await URLModel.find({ original: url });

    if (Object.keys(docs).length > 0) {
      return res.json({original_url: req.body.url, short_url: docs[0].shortened});
    }

    const results = await URLModel.find();
    const count = results.length;
    const urlDocument = new URLModel({original: url, shortened: count});
    await urlDocument.save();
    return res.json({original_url: req.body.url, short_url: count});
  });
}
Getter Jetter
  • 2,033
  • 1
  • 16
  • 37
  • 1
    Thank you, @OlivierKrull ! This was very helpful and clear. A somewhat tangential question, is there an advantage of using 'const' instead of 'let' to declare the variables? Is it simply because there is no intent of modifying these variables or are there some other advantages (e.g., efficiency)? – Steph Jul 31 '20 at 15:37
  • Hi yw, [this](https://stackoverflow.com/questions/41086633/why-most-of-the-time-should-i-use-const-instead-of-let-in-javascript#answer-41086682) answer sums it up pretty well. Personally I like to use `const` wherever possible when I know the variable wont be changed for the sake of readability and make the code cleaner. – Getter Jetter Aug 01 '20 at 08:03
0

You can go async way and use Promise/await. Below is just an example, but you can adopt it for your program

// Use async
(async () => {
  // Function 1
  const fn1 = (val) => {
    return new Promise((resolve, reject) => {
      // Do some stuff here
      val = val + 1;
      // Resolve result.
      // Can be resolved from any level
      // of nested function!
      function nested1() {
        function nested2() {
          resolve(val);
        }
        nested2();
      }
      nested1();
    });
  };
  
  // Function 2
  const fn2 = (val) => {
    return new Promise((resolve, reject) => {
      // Do some stuff here
      val = val * 2;
      // Resolve result
      resolve(val);
    });
  };
  
  // Function 3
  const fn3 = (val) => {
    return new Promise((resolve, reject) => {
      // Do some stuff here
      val = val + 1000;
      // Resolve result
      resolve(val);
    });
  };
  
  // Sync code
  let val = 5;
  
  val = await fn1(val); // Wait until fn1 resolves
  val = await fn2(val); // Wait until fn2 resolves
  val = await fn3(val); // Wait until fn3 resolves
  
  console.log(val);
})();
tarkh
  • 2,424
  • 1
  • 9
  • 12
  • Thank you, @tarkh . This does show the high-level idea. Olivier Krull's answer below is more specific, so I accepted it instead. I appreciate your answer too! – Steph Jul 31 '20 at 15:41