Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Here's my code for a consise node.js HTTP server. It does work as intended, but I'd like to ask if there's any glitch or to be improved.

   var httpServer = function(dir)
    {
      var component = require('http')
        .createServer(function(req, res)
        {
          var fs = require('fs');
          var path = require("path");
          var url = require('url');

          var mimeTypes = {
            "html": "text/html",
            "jpeg": "image/jpeg",
            "jpg": "image/jpeg",
            "png": "image/png",
            "js": "text/javascript",
            "css": "text/css"
          };

          var uri = url.parse(req.url)
            .pathname;
          var filename = path.join(dir, unescape(uri));
          var indexFilename = path.join(dir, unescape('index.html'));
          var stats;

          console.log(filename);

          try
          {
            stats = fs.lstatSync(filename); // throws if path doesn't exist
          }
          catch (e)
          {
            res.writeHead(404,
            {
              'Content-Type': 'text/plain'
            });
            res.write('404 Not Found\n');
            res.end();
            return;
          }


          if (stats.isFile())
          {
            // path exists, is a file
            var mimeType = mimeTypes[path.extname(filename)
              .split(".")[1]];
            res.writeHead(200,
            {
              'Content-Type': mimeType
            });

            var fileStream =
              fs.createReadStream(filename)
              .pipe(res);
          }
          else if (stats.isDirectory())
          {
            // path exists, is a directory
            res.writeHead(200,
            {
              'Content-Type': "text/html"
            });
            var fileStream =
              fs.createReadStream(indexFilename)
              .pipe(res);
          }
          else
          {
            // Symbolic link, other?
            // TODO: follow symlinks?  security?
            res.writeHead(500,
            {
              'Content-Type': 'text/plain'
            });
            res.write('500 Internal server error\n');
            res.end();
          }

        });

      return component;
    };


    var port = 9999;
    var dir = 'www';
    var HTTPserver =
      httpServer(require('path')
        .join(__dirname, dir))
      .listen(port, function()
      {
        console.log('HTTP listening ' + port);
      });
share|improve this question

3 Answers 3

You should move all require(...) calls to the very beginning of your code, instead of running require on each place locally.

In its current form, you won't see require errors until your server runs and receives its first requests. This is unfortunate. You usually want a program to fail as early as possible if it clearly won't work.

Rule of Repair (TAOUP): Repair what you can — but when you must fail, fail noisily and as soon as possible.

share|improve this answer
    
Thanks you are right, great advice. –  Ken OKABE Jan 28 at 20:56

I modified as follows, in addition to ensure require error possibility checking, structurally better I guess. Plus, I modified to full async where it should be.

var httpServer = function(dir)
{
  var fs = require('fs');
  var path = require("path");
  var url = require('url');

  var mimeTypes = {
    "html": "text/html",
    "js": "text/javascript",
    "css": "text/css",
    "jpeg": "image/jpeg",
    "jpg": "image/jpeg",
    "png": "image/png",
    "gif": "image/gif",
    "svg": "image/svg"
    // more
  };

  var component = require('http')
    .createServer(function(req, res)
    {
      var uri = url.parse(req.url).pathname;
      var filepath = path.join(dir, unescape(uri));
      var indexfilepath = path.join(dir, unescape('index.html'));

      console.info('filepath', filepath);

      var f = function(err, stats)
      {
        if (stats === undefined) // path does not exit 404
        {
          res.writeHead(404,
          {
            'Content-Type': 'text/plain'
          });
          res.write('404 Not Found\n');
          res.end();

          return;
        }
        else if (stats.isFile()) // path exists, is a file
        {
          var mimeType = mimeTypes[path.extname(filepath).split(".")[1]];
          res
            .writeHead(200,
            {
              'Content-Type': mimeType
            });

          var fileStream =
            fs
            .createReadStream(filepath)
            .pipe(res);

          return;
        }
        else if (stats.isDirectory()) // path exists, is a directory
        {
          res
            .writeHead(200,
            {
              'Content-Type': "text/html"
            });
          var fileStream =
            fs
            .createReadStream(indexfilepath)
            .pipe(res);

          return;
        }
        else
        {
          // Symbolic link, other?
          // TODO: follow symlinks?  security?
          res
            .writeHead(500,
            {
              'Content-Type': 'text/plain'
            })
            .write('500 Internal server error\n')
            .end();

          return;
        }
      };

      fs.stat(filepath, f);

    });

  return component;
};


var port = 9999;
var dir = 'www';
var HTTPserver =
  httpServer(require('path')
    .join(__dirname, dir))
  .listen(port, function()
  {
    console.info('HTTP listening', port);
  });
share|improve this answer
    
What exactly were you simplifying? Could you explain rather than just showing? A whole new version of the code won't create much learning effect, especially since there is no easy diff mechanism here. –  vog Jan 29 at 13:10

simpler

var port = 9999;
var directory = 'www';

var http = require('http');
var url = require('url');
var path = require("path");
var fs = require('fs');

var mimeTypes = {
  "html": "text/html",
  "js": "text/javascript",
  "css": "text/css",
  "jpeg": "image/jpeg",
  "jpg": "image/jpeg",
  "png": "image/png",
  "gif": "image/gif",
  "svg": "image/svg"
    // more
};

var request = function(req, res)
{
  var uri = url.parse(req.url).pathname;
  var dir = path.join(__dirname, directory);
  var filepath = path.join(dir, unescape(uri));
  var indexfilepath = path.join(dir, unescape('index.html'));

  console.info('filepath', filepath);

  var f = function(err, stats)
  {
    if (stats === undefined) // path does not exit 404
    {
      res.writeHead(404,
      {
        'Content-Type': 'text/plain'
      });
      res.write('404 Not Found\n');
      res.end();

      return;
    }
    else if (stats.isFile()) // path exists, is a file
    {
      var mimeType = mimeTypes[path.extname(filepath).split(".")[1]];
      res
        .writeHead(200,
        {
          'Content-Type': mimeType
        });

      var fileStream =
        fs
        .createReadStream(filepath)
        .pipe(res);

      return;
    }
    else if (stats.isDirectory()) // path exists, is a directory
    {
      res
        .writeHead(200,
        {
          'Content-Type': "text/html"
        });

      var fileStream =
        fs
        .createReadStream(indexfilepath)
        .pipe(res);

      return;
    }
    else
    {
      // Symbolic link, other?
      // TODO: follow symlinks?  security?
      res
        .writeHead(500,
        {
          'Content-Type': 'text/plain'
        })
        .write('500 Internal server error\n')
        .end();

      return;
    }
  };

  fs.stat(filepath, f);
  return;
};

var serverUp = function()
{
  console.info('HTTP server listening', port);
  return;
};

var component = http
  .createServer(request)
  .listen(port, serverUp);
share|improve this answer
    
I propose to edit your other anser, instead of creating a new answer for each code improvement. –  vog Jan 29 at 12:10

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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