Seeking feedback on my URL Shortener code

Hey everyone,

Just finished my URL Shortener project. Everything seems to be working. I was hoping to get some feedback on my code since it seems like the first backend project where I’m writing a lot of code from mostly scratch. In my urlShortener.js file, for instance, I use a lot of functions within functions. I’m not sure if this is best practice or if it matters. Any and all feedback welcome.

Here’s a link on Glitch: https://glitch.com/edit/#!/fcc-urlshorten

And here’s some code from my files:

server.js:

'use strict';

let express = require('express');
let mongo = require('mongodb');
let mongoose = require('mongoose');
let cors = require('cors');
let app = express();

// Basic Configuration 
let port = process.env.PORT || 3000;

/** this project needs a db !! **/ 
mongoose.connect(process.env.MONGOLAB_URI);

app.use(cors());

/** this project needs to parse POST bodies **/
// you should mount the body-parser here
let bodyParser = require('body-parser');
app.use(bodyParser.urlencoded({ extended: true })); // middleware to capture the input field of a form

app.use('/public', express.static(process.cwd() + '/public'));

let urlShortener = require("./urlShortener.js");
let redirectAction = require("./redirectAction.js");

app.post("/api/shorturl/new", urlShortener.createShort);

app.get("/api/shorturl/:short", redirectAction.redirect);

app.get('/', function(req, res){
  res.sendFile(process.cwd() + '/views/index.html');
});

// your first API endpoint... 
app.get("/api/hello", function (req, res) {
  // res.redirect("http://freecodecamp.org");
  res.json({greeting: 'hello API'});
});

app.listen(port, function () {
  console.log('Node.js listening ...');
});

urlShortener.js:

'use strict';

let mongoose = require("mongoose");
let Schema = mongoose.Schema;
let dns = require('dns');

let UrlSchema = new Schema({
  long: {
    type: String,
    required: true
  },
  short: Number
});

let Url = mongoose.model("Url", UrlSchema);
let regex = /^https?:\/\//m;

let createShort = function(req, res) {
  let newlink = req.body.url; // captures input field of form; "url" here matches <input name="url"> in index.html file
  
  if (regex.test(newlink)) {
    newlink = newlink.split(regex)[1]; // gets rid of http(s):// in entry if it exists so dns.lookup will work
  }
  
  dns.lookup(newlink, (err) => { // make sure URL is good
    if (err) {
      res.json({ error: "Invalid URL" });
    }
    
    else {
      checkRepeat(newlink);
    }
  });
  
  async function checkRepeat(url) { // check if url is already in database
    let check = await Url.findOne({long: url});

    if (check) { // already exists, so return info
      res.json({
        long: check.long,
        short: check.short
      });
    }
    
    else { // doesn't exist, so trigger addUrl function
      addUrl(url);
    }
  };
  
  async function addUrl(url) {
    let len = await Url.findOne().sort({ short: -1 }).limit(1).exec() // gets most recent entry using short field
    let newshort;    
    
    if (len == null) { // first entry in database
      newshort = new Url({
        long: url,
        short: 1
      });
    }
    
    else {
      newshort = new Url({
        long: url,
        short: len.short + 1
      });
    }
      
    newshort.save((err, data) => {
      if (err) { console.log(err) }
      else { console.log(data) }
    });
      
    res.json(newshort);
  }

};


//----------- Do not edit below this line -----------//

exports.UrlModel = Url; // UrlModel will be how it is imported in other documents such as redirectAction.js
exports.createShort = createShort;

redirectAction.js:

'use strict';

let Url = require("./urlShortener.js").UrlModel;

let express = require('express');

async function redirect(req, res) {
  let short = req.params.short;
  let site = await Url.findOne({ short: short}).exec(); // uses short to find entry
  if (site) {
    res.redirect("http://" + site.long);
  }
  
  else {
    res.json({ Error: "No entry matches that path" });
  }
}

//----------- Do not edit below this line -----------//

exports.redirect = redirect;

I’m not so sure about what I am going to write here.

I believe you could use instead of dns lookup other npm package that checks if the url exists. Just a minor thing. For the user story this will work I think. I know I used such a function but don’t know anymore how it is called. And it is advised even so it’s good I think.

Also, in dns.lookup you don’t give an argument after err. I have looked up the package, but you could think of giving it a function with the err, adress, family arguments.

1 Like