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.

I am new to node.js and asynchronous programming and this is my first small project to learn it. I have written a small Telnet Chat server and the program works but I want to know if this is the correct way to write programs in node.js(asynchronous programming model).

var tcp = require('net');
var Server = tcp.createServer();
var pwd=[]; // keep tracks of connected clients 
var ip=[];  // keeps tracks of connected clients IP address
var Cpwd = "Rushabh"; // password that you require to login

Server.on('connection'  ,function(conn){
    conn.setEncoding('utf-8');
    conn.write("Password:");// ask user for password on their terminal
    console.log("[" + conn.remoteAddress + "] has joined the chat");

    conn.on('data' , function(data){
        if(pwd.indexOf(conn)>-1){//check if it is an old client or a new client
            console.log("[" + conn.remoteAddress + "]:"  + data); 

            if(pwd.length > 0){ // check if atleast one client is connected
                sendMessage(conn , data);// broadcast message to all client connected 
            }
        }
        else{//if it is a new client then server should first check for password 
            data= data.toString('utf-8').trim();
            var message = " has joined the chat";
            if(Cpwd == data){ // if it is a new client than check for password
                pwd.push(conn); 
                ip.push(conn.remoteAddress);
                sendMessage(conn , message);
            }
            else {
                conn.write("Password rejected:" + data);conn.end();}// disconnect client 
            }
        }); 

      conn.on('end' , function() { // remove the client from reference array

       var i , client;
         for(i in pwd){
           client = pwd[i];
           if(!client.writable){
             pwd.splice(i,1);
             console.log(ip[i]+ " has left the chat");
             ip.splice(i,1);
           }
         }

     });

}); 

    function sendMessage(conn , message){ //function to send message to all connected client

      var i , client;
      for(i in pwd){
        client = pwd[i];
        if(client.writable){     
          if(conn === client){       
            client.write("[me]:" + message);         
          }      
          else
            client.write("[" + conn.remoteAddress +"]:" + message);
        }
        else{
          pwd.splice(i , 1);
          console.log(ip[i]+ " has left the chat");
          ip.splice(i,1);
        }   
      }
    }
    Server.listen(8000);
share|improve this question
add comment

1 Answer

up vote 2 down vote accepted

From a once over:

Naming

  • variable names should start with a lower case letter, unless they are constructors
    • Server -> server
    • Cpwd -> password
  • pwd contains all the connections, any reader would guess it contains passwords

More on variables

  • There is no good reason to have a separate array ip, since it always contains as many entries as pwd which already contains the the ip addresses in remoteAddress.
  • Having the password inside your scrip is ok for a prototype, it is not for production

Functions

You encapsulated sendMessage into a function, you should also have a leave or quit function and a joinChat function.

Flow

  • You announce that a user joins the chat before checking the password
  • In on 'end' you could just use indexOf on conn instead of manually looping over all connections.

Other than that I think you are doing fine with the asynchronous model of Node.

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.