current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

I know using global variables in JavaScript is bad practice. Here I am using config, host and port as global variables as I have added a watch function to watch for the changes in the config.json file. Is the use of global variables in this example a bad practice or is it ok to use them?

config.json file

{
    "host":"127.0.0.1",
    "port":8000
}

server.js file

(function(){

    var fs = require("fs");
    config = JSON.parse(fs.readFileSync("config.json")); //global variable
    host = config.host;  //global variable
    port = config.port;  //global variable
})();


var http = require("http");
var server = http.createServer(function(request,response){
    response.writeHead(200,{"Content-Type":"text/plain"});
    response.end("Hello World");
});

server.listen(port,host,function(){
    console.log("Server listening at:"+host+" on port:"+port);
});


fs.watchFile("config.json",function(){
    config = JSON.parse(fs.readFileSync("config.json"));  //global variable(overwritten)
    host = config.host;  //global variable(overwritten)
    port = config.port;  //global variable(overwritten)

    server.close();
    server.listen(port,host,function(){
          console.log("Server listening at:"+host+" on port:"+port);
       };
    }
});
share|improve this question
1  
You don't really need globals to accomplish this. Globals should be reserved to variables you want to expose to the environment. –  megawac May 25 at 17:13
add comment

2 Answers

up vote 2 down vote accepted

The "global" you mean here is the module scope. In NodeJS, each file has it's own scope, unlike browser JS where every file runs in a single global scope. This code is too small to worry about globals, and you'll be splitting your code into several files/modules anyways to keep it maintainable. TL;DR - don't worry about globals.

As for your code, here's a few tweaks. My NodeJS is a bit rusty, but I think this works:

// Declare dependencies up top. Makes them easier to see.
var fs = require("fs");
var http = require("http");
var configFile = 'config.json';

function readConfig(file){
  return JSON.parse(fs.readFileSync(file));
}

function runServerWithConfig(server,configFile){
  // close server if it exists
  if(server) server.close();

  // read config
  var config = readConfig(configfile);

  // listen using new config
  server.listen(config.port,config.host,function(){
    console.log("Server listening at:"+config.host+" on port:"+config.port);
  });
}


var server = http.createServer(function(request,response){
    response.writeHead(200,{"Content-Type":"text/plain"});
    response.end("Hello World");
});

// Run and watch
runServerWithConfig(server,configFile);
fs.watchFile(configFile,function(){
  runServerWithConfig(server,configFile);
});
share|improve this answer
add comment
up vote 2 down vote

You say you're using globals because "I have added a watch function to watch for the changes in the config.json file". You don't need globals for this. If you declare the variables at the top level, like you've done with http and server, they'll be accessible everywhere in your script, including inside the anonymous function where you're using them.* You could get rid of the (function() { ... })(); wrapper round the first bit (not sure why you've wrapped that bit) and then declare them there.

"I know using global variables in JavaScript is bad practice." Using global variables in any language is bad practice if you can avoid it.

*A function like this, which uses variables declared outside the function, is called a "closure".

share|improve this answer
add comment

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.