-2
\$\begingroup\$

I'm working on a Node.js chat app project using the ws library. I have it up and running but I'm still making improvements and adding stuff like authentication, etc. Before I get too far, I am wondering what the best functional approach to handling messages (and other events) is. For example, right now I have the following code to handle a message from the client (I cut out most of it, just keeping in one function to use as an example):

wss.on('connection', (ws, req) => {
    // Give them the funtions (uses local ws)
    function handleMessage(msgJSON) {
        let incMsg = {};
        try {
            incMsg = JSON.parse(msgJSON); // message from client
        } catch {
            console.error("Could not parse sent JSON");
            return;
        }
        switch (incMsg.type) {
            case "message":
                 ws.send("Message sent.");
                 // send the message to other clients:
                 wss.clients.forEach(client => {
                      // send message to all open clients but not this client
                      if (client !== ws && client.readyState === WebSocket.OPEN) {
                           client.send(`${client.username}: ${incMsg.message}`);
                      }
                 }
            // some more cases, like for "meta"
        }
    }

    // more functions ...

    ws.on('message', message => {
        handleMessage(message);
    });
}

This strategy takes advantage of having the ws variable inside the inner function, but this makes the wss.on('connection') handler full of a lot of functions. Is it better practice to use this approach or to make a global sort of function that you then pass the client into? My alternate idea looks like this:

function handleMessage(msgJSON, ws) { // accepts the current client
    let incMsg = {};
    try {
        incMsg = JSON.parse(msgJSON); // message from client
    } catch {
        console.error("Could not parse sent JSON");
        return;
    }
    switch (incMsg.type) {
        case "message":
             ws.send("Message sent.");
             // send the message to other clients:
             wss.clients.forEach(client => {
                  // send message to all open clients but not this client
                  if (client !== ws && client.readyState === WebSocket.OPEN) {
                       client.send(`${client.username}: ${incMsg.message}`);
                  }
             }
             break;
        // some more cases, like for "meta" ...
    }
}

// more functions ...

wss.on('connection', (ws, req) => {
    ws.on('message', message => {
        handleMessage(message, ws); // pass along the ws client
    });
}
```
\$\endgroup\$
4
  • \$\begingroup\$ Is this question closed because it is too general and I should move it to Stack Overflow or is my code just bad? I omitted most of it because it's repetitive and I am looking for best practice advice. \$\endgroup\$ Commented Jun 3, 2020 at 22:18
  • \$\begingroup\$ Did you read the linked meta answer in the close reason? If that doesn't answer your question, perhaps help center pages like Asking How do I ask a good question? and What topics can I ask about here? would be helpful... \$\endgroup\$ Commented Jun 4, 2020 at 17:29
  • \$\begingroup\$ I did but I don't think my code is broken, hypothetical, or non-existent. As the topics help page said, my question was about "application of best practices and design pattern usage." \$\endgroup\$ Commented Jun 5, 2020 at 19:21
  • 1
    \$\begingroup\$ The users who voted to close were likely deterred by comments like // some more cases, like for "meta", and would have referred you to the partial paragraph: "Excerpts of large projects are fine, but if you have omitted too much, then reviewers are left imagining how your program works.". The user that supplied an answer didn't have much context about the other code yet to be implemented or even a broad view of what the websocket application did other than showing messages. If you have more questions then please ask on meta. \$\endgroup\$ Commented Jun 5, 2020 at 20:13

1 Answer 1

1
\$\begingroup\$

"Before going too far" - is the tip of the iceberg as I see it :)

If it's just a simple lab, nothing fancy, tutorial for the sake of testing some functionality and everything should remain small and in one file then ignore the rest of what I'm saying.

Otherwise, if that is a long ongoing project, it should be production-ready and etc.., there are lots of aspects to consider so it still a hard question to answer.

To simplify and I'm sure many would not agree with me.

I consider a single source, a monolith chat application.

Try to disconnect the WebSocket from the business logic and create a clear cut boundary.

Because you want to unit test all parts of the system you should plan your code around testing - so business logic should be seeing as a black box that you can test as unit.

Imagine that from one side of the boundary you are passing a generic message package. Inside the boundary, the message package is broken and the real action/parameters are extracted and a function is dispatched based on these values to handle the request.

This actually starts forming a request/response flow - that flow should have validation, use case handler, presentation handler along the way.

your action handler should be generic and allow adding more functionality into the communication protocol, so you should use a factory pattern to create action handlers by name.

I hope this helps

\$\endgroup\$
2
  • \$\begingroup\$ Are you saying use encapsulation with module.exports or something to keep the files separate? \$\endgroup\$ Commented Jun 3, 2020 at 17:31
  • \$\begingroup\$ 1) That's not all that I said. 2) But yes, you should use modules to break you project into multiple files. \$\endgroup\$ Commented Jun 3, 2020 at 22:01

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.