Socket.io Node.js Object Oriented Code Structure Approach

Question

following application: Users can join Rooms. A Gameroom inherits from Room and has a queue where Users can queue for a game. If enough people queue they get thrown into a game lobby. Lobby also inherits from Room. Rooms implement chat functionality which should be used in Gamerooms and Lobbies aswell.

Right now I am passing the io Object to the instantiations and use it to listen to events specific to their class features.

What improvements can I do? Why is this a bad or good design choice? What alternatives are there? What is there to consider? Should I split the classes "Room", "Gameroom" and "Lobby" into a more distinct Model & Controller?

UML Diagram

Server instantiates the Rooms.
Server.js

const io = socket.listen(
    http.createServer(app).listen(8080)
  );

console.log('Server listening..');

app.get('/', function (req, res) {
    res.sendFile(__dirname + '/test.html');
  });

var main = new Room('', io);
var a = new Gameroom('a', io);
var b = new Gameroom('b', io);

Room is the parent class, but can also be instantiated. It should be a root channel where everyone joins when they login.
Room.js

class Room {
  constructor(name, io) {
    this.name = name;
    this.users = [];
    this.namespace = io.of('/' + name);
    this.listenOnRoom();
  }

  listenOnRoom() {
    this.namespace.on('connection', (socket) => {
      ...

      socket.on('disconnect', (msg) => {
        ...
      });

      socket.on('chatMessage', (msg) => {
        ...
      });
    });
  }
}
module.exports = Room;

Gameroom adds the ability to queue for a game.
Gameroom.js

class Gameroom extends Room {
  constructor(name, io) {
    super(name, io);
    this.io = io;
    this.queue = [];
    this.lobbies = [];
    this.listenOnGameroom();
  }

  listenOnGameroom() {
    this.namespace.on('connection', (socket) => {

      socket.on('userJoinQueue', () => {
        ...
        this.lobbies.push(new Lobby(this.name ,lobbyId, participants, this.io));
      });

      socket.on('userLeaveQueue', () => {
        ...
      });

      socket.on('userLeaveLobby', () => {
        ...
      });
    });
  }
}

module.exports = Gameroom;

Lobby is instantiated by the Gameroom when there are enough players in the queue.
Lobby.js

class Lobby extends Room {
  constructor(gameroom, name, users, io) {
    super(gameroom + '/' + name, io);
    this.users = users;
    this.readyUsers = [];
    this.listenOnLobby();
  }

  listenOnLobby() {
    this.namespace.on('connection', (socket) => {

      socket.on('userReady', () => {
        ...
      });

      socket.on('userUnready', () => {
        ...
      });
    });
  }
}

module.exports = Lobby;

Answer 1

Just a few comments:

1: The Room class can quickly become a place to throw in all sorts of functionality, even when it's not needed by all its children. I would be careful to not put much there at all, and rather compose the different rooms with needed functionality. For example, a GameRoom may be composed by a chat, game queue, lobbies, and even user management. Using composition like this can make it much easier to build different types of rooms, without having to throw every bit of common code into Room. I would even consider removing Room altogether if possible.

2: In your socket callbacks, I would only put in single function calls to methods in your class. This way, if you want to test one of these functions, you don't need to go via the socket callbacks. Also, when instantiating your classes, I would consider not running the listener immediately. If you're testing this, you don't want the hassle of mocking up io if you don't need to.

But I think an even better way to do this would be to abstract away the socket stuff from these classes entirely. This could be a bit too much though, depending on the complexity of the app.

3: Consider replacing

this.lobbies.push(new Lobby(this.name ,lobbyId, participants, this.io));

with something like

this.lobbies.push(this.lobbyFactory.create(this.name, lobbyId, participants));

This is useful for testing, since you don't force the usage of one particular lobby. It's also useful if you decide to have different types of lobbies later on. Then you can just inject another type of lobbyFactory.

Answer 2

Good comments from @Magnus Jeffs Tovslid that I would like to add to.

It seems like you have at least three logical layers to the user experience. First, you have what you call the Room, which doesn't seem to have any "room" behaviors at all and whose relationship to Gamerooms is unclear. What is the function of the room? What is the meaning to the end user? Why does it even exist? Is this really more of an abstract class that you will use as the prototype for concrete rooms in the application which inherit form this prototype? Is Room simply a bad name to indicate what this class is for?

Then you have the Gameroom. I don't understand the difference between queue and lobbies properties on the Gameroom. Your written description that users fill the queue until there are enough to fill a game, at which point a lobby for that game is formed, does not seem to be matched by your code, which seems to spawn a Lobby every time a user joins the queue? Perhaps my confusion is because you are not showing your complete code (something frowned upon here actually).

Finally, you have the Lobby. I don't understand why Lobby would extend Room at all. This seems to be a totally different construct used for a totally different purpose (of actually managing game state?), and in fact holds a one to many relationship to a given Gameroom and has no apparent reason to extend from Room at all. The Gameroom truly owns the Lobby and even injects dependencies into it, so I actually like the instantiation pattern that Magnus suggests which more truly reflects that Gamerooms own the logic for instantiating the Lobby.

---

I find it odd that Lobby holds logic for cobbling together it's socket namespace given a gameroom name and lobby id. Why wouldn't the owning Gameroom (perhaps though a factory method) own this logic and provide the value directly into the Lobby? Why does the Lobby get to determine this? It seems like a Lobby cannot exist in the system without an owning Gameroom, so why let it be instantiated directly with "arbitrary" namespace values?

---

I have similar concern for namespacing between Room and Gameroom. You are doing nothing to prevent namespace collision. Should Room own providing namespace to Gamerooms? Should Room actually contain an array of Gamerooms that it owns similar to the relationship between Gameroom and Lobby so that it can truly own/manage/instantiate Gamerooms within it's top level domain space?

---

I do want to emphasize Magnus' second point as well and add a tangible example, in case you miss the meaning. Having a bunch of code in a closure in your callback is going to make testing extremely difficult. So instead of this:

this.namespace.on('connection', (socket) => {
    // lots of code here
}

You should aim for something like:

this.namespace.on('connection', this.handleConnection);

and have class methods like

handleConnection = (socket) => {
    // your code to execute on connection
    // for example
    socket.on('userReady', this.handleUserReady);
    ...
    socket.on('userUnready', this.handleUserUnready);
    ...
}

handleUserReady = () => {
    ...
}

handleUserUnready = () => {
    ...
}

This allows you to unit test these class methods more easily. It also allows you to better override specific handlers in inheriting classes in a much more straightforward way.

---

A few thoughts on variable/method naming:

• Why listenOnRoom(), listenOnGameroom(), listenOnLobby() instead of just listen()? If you were properly using inheritance, this would allow you to override the methods in inheriting classes (calling super() as needed). Again, since I think the case for inheritance is weak here anyway, this would at least give consistency to the models in the system.
• Be consistent in your naming of properties in your system. You seem to be using user and participant interchangeably to represent the same thing in the system. Should this terminology be made consistent?