4
\$\begingroup\$

I'm a back-end developer (in PHP and Python) and I use JavaScript mostly for DOM manipulation.

Now I'm writing a chat app in Node.js and I'm not sure if my code is the cleanest.

Here is how it works:

  • Users open the chat page;
  • We will match them randomly to another user;
  • They will chat p2p.

Here is what I have:

When the user opens the page and sends the ready signal to the server, it will add the user information to the USERS stack and add the user socket id (which I basically use as its id ) to the QUEUE object.

const USERS   = {};
const QUEUE   = {};
const MATCH   = [];
const CHATS   = {};

app.io.route('ready' , function (req){


    USERS[req.socket.id] = {
        username : req.data.username ,
        socket : req.socket.id ,
        req : req ,
    };

    QUEUE[req.socket.id] = req.socket.id ;

 })
 

Next there is a function to process the users in the queue and match them together. This function will grab the first user in the QUEUE (we call it primary user) and loop through the other users in the queue and when it finds another user (secondary user). It will add them to the MATCH array and remove them from the QUEUE and at the end calls the next function match_making.

var QUEUE_INPROG = false ;
function process_queue(){

    if(QUEUE_INPROG) return  ;
    QUEUE_INPROG = true  ;
    
    if( Object.keys(QUEUE).length < 2 )
    {
        QUEUE_INPROG = false ;
        return  ;
    }

    let primary_user = QUEUE[ Object.keys(QUEUE)[0] ];

    for (let secondary_user  in QUEUE) {

        if ( secondary_user != primary_user) {

            MATCH.push({
                primary   :  primary_user  ,
                secondary :  secondary_user,
            });

            delete QUEUE[secondary_user];
            delete QUEUE[primary_user];

            break ;
        }

    }

    QUEUE_INPROG = false ;
    match_making();

}


setInterval(()=>{
    process_queue();
} , 5000 );

The last function is pretty simple basically grabbing both users and sending their info to each other:

function match_making(){

    if(MATCH.length < 1 ) return  ;
    var match = MATCH.shift();

    let primary   = USERS[match.primary];
    let secondary = USERS[match.secondary];

    console.log('sending match data to users ');

    primary.req.io.emit('incoming_chat', { user  : secondary.username  })
    secondary.req.io.emit('incoming_chat', {user : primary.username })

    let chat_key = match.primary+match.secondary;
    CHATS[chat_key] = {

        primary   : primary ,
        secondary : secondary ,
        guest     : null

    };

}

I'm not happy about how I call functions and not sure if using an interval for calling process_queue is the best way to go. Also, I had to use a flag (QUEUE_INPROG) to avoid function being called again before the last call is done.

And how I have to use objects instead of array because I find it hard to search in an array and remove values.

I would love for someone with JS experience to take a look at this code and give me some feedback.

\$\endgroup\$
3
+25
\$\begingroup\$

The first thing that stands out is the casing on variables that are declared like globals. Syntactically there is nothing wrong with it but a common convention in many style guides is to only use all caps for (immutable) constants. This is recommended in many widely accepted style guides, not just for JavaScript (e.g. ESLint, AirBnB, Google) but also for Python (PEP-8) and other C-based languages like PHP (PSR-1).

The scope of variables can be reduced dramatically. Prefer using const over let and var. This helps anyone reading the code, including your future self, know what can be reassigned and what can’t. Refer to answers to this post for more details.

The shorthand property definition notation can be used to simplify the lines like these where the key is the same as the name of the variable being referenced:

CHATS[chat_key] = {

       primary   : primary ,
       secondary : secondary ,
       guest     : null

};

Can be simplified to:

CHATS[chat_key] = {

    primary,
    secondary,
    guest     : null

};

Also, the call to setup the interval can be simplified from wrapping the call in an anonymous function/closure:

setInterval(()=>{
    process_queue();
} , 5000 );

To just referencing the function name:

setInterval(process_queue, 5000 );

…not sure if using an interval for calling process_queue is the best way to go. Also, I had to use a flag (QUEUE_INPROG) to avoid function being called again before the last call is done.

Instead of an interval being set, setTimeout() could be used at the end of the function. That may eliminate the need for setting and checking the flag.

how I have to use objects instead of array because I find it hard to search in an array and remove values.

A Map could be used. A Map object iterates its elements in insertion order — a for...of loop returns an array of [key, value] for each iteration. 1.

A WeakMap may also be useful. It may be able to help eliminate memory leaks.


There is a loop in process_queue():

for (let secondary_user  in QUEUE) {

From the MDN documentation for for…in:

A for...in loop iterates over the properties of an object in an arbitrary order (see the delete operator for more on why one cannot depend on the seeming orderliness of iteration, at least in a cross-browser setting). 2

If the code was run on the client-side and IE was supported then it would be possible that insertion order wouldn’t be guaranteed3 . However this code is run on the node server, but in general it would be better to use a for…of loop to iterate over the keys.

for (const secondary_user of Object.keys(QUEUE)) {
\$\endgroup\$
2
\$\begingroup\$

In the ready function, don't add to queue, call queue-handler with id. pop your queue-list and if not undefined you've got a match. else add the user to the queue.

Don't know if you need a MATCH-object at all. You're using it as temporary storage between functions instead of just passing parameters. Maybe you need it later?

You shouldn't need setInterval when every new, ready user triggers the process. And that might remove the need for QUEUE_INPROG variable.

You uppercase variables makes me as a javascript developer uncomfortable but that's ok :)

\$\endgroup\$
2
  • \$\begingroup\$ thanx , the thing is i might have multiple users online and ready at the same time , thats why i've put them in an queue object/array and have another function process them to avoid some sort of race condition bug that might happen if i call the queue-handler directly in the ready event \$\endgroup\$
    – hretic
    Nov 11 at 2:11
  • \$\begingroup\$ but it's a good idea to call the queue handler when user is ready and pass the id to handler , pop the queue object in the handler function for match and if not found just add the new id to queue object ... this way i dont need to use interval \$\endgroup\$
    – hretic
    Nov 11 at 2:13
2
\$\begingroup\$

A longer review;

  • Keep full uppercase names only for constant values like numbers
  • In the ready function you access req.socket.id enough that you could store it locally
  • To me, it is a code smell that you have the socket id in the key and in the value of USERS
  • You mix var with const and let, I would stick with const and let
  • JavaScript variables should be in lowerCamelCase so primary_user -> primaryUser
  • delete was at some point considered obsolete, I try to avoid its use
  • For a queue, I would use a list, not an object, I think the code could be so much simpler
  • Magic numbers (like 5000) should be in a nicely named constant
  • A match is just 2 objects, that seems unlikely to change, I would use an array instead of an object
  • You should have a logging framework with log levels
  • Actually, globals are evil, you should really look into that
  • I prefer function names to follow <verb><thing> so match_making -> makeMatches
  • YOu should realize that ()=>processQueue(); and processQueue are functionally equivalent, but the 2nd approach is cleaner and more readable
  • You should read about the async keyword, I am fairly certain that you don't need QUEUE_INPROG. I left it in the rewrite, because I cant test the code

Taking that into account;

Part 1;

const socketUsers = {};
const queue   = {};
const matches = [];
const chats   = {};

app.io.route('ready' , function (request){

  id = request.socket.id;
    socketUsers[id] = {
        username : request.data.username,
        socket : id ,
        req : request,
    };
    //Drawback of a list, it does not natively weed out dupes
    if(!queue.includes(id)){
      queue.push(id);
    }
 })

Part 2;

let queueLocked = false ;
const queueProcessInterval = 5000;
function processQueue(){

    if(queueLocked || queue.length < 2){
      return;
    }
    queueLocked = true;

    matches.push([queue.pop(), queue.pop()]);

    queueLocked = false ;
    makeMatches();
}


setInterval(processQueue, queueProcessInterval);

Part 3;

function makeMatches(){

    while(matches.length){
      const match = matches.pop();
      const primary = users[match.pop()];
      const secondary = users[match.pop()];

      //You should have a logging framework with log levels
      //console.log('sending match data to users ');

      primary.req.io.emit('incoming_chat', { user  : secondary.username  })
      secondary.req.io.emit('incoming_chat', {user : primary.username })

      const chatKey = primary.socket + secondary.socket;
      //This will make guest `undefined`, not null, I am not a fan of null
      chats[chatKey] = {primary, secondary, guest};

    }
}
\$\endgroup\$
1
  • \$\begingroup\$ about the function names <verb><thing> like ` match_making -> makeMatches` i think starting with thing kinda works like namespace , makes it easier to find/group the functions about that thing \$\endgroup\$
    – hretic
    Nov 29 at 12:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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