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:
- 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.