Conversation
|
These changes look alright |
| end | ||
|
|
||
| def add_host(name, host) do | ||
| GenServer.cast(ref(name), host) |
There was a problem hiding this comment.
Instead of using host I would make this an explicit tuple (like the one you are sending {:host, host})
That prevents calling this API with improper data. Currently I can potentially override the host variable to hit all the handle_cast callbacks. That should be restricted.
|
|
||
| def start do | ||
| name = Nanoid.generate() | ||
| case Supervisor.supervise({Monitor, name: ref(name)}) do |
There was a problem hiding this comment.
MINOR: Supervisor here shadows the Supervisor module. Maybe rename the alias to GameSupervisor or something else
| %PencilSpaceServer.Game.State{host: 1} | ||
| """ | ||
| def update(name, %{"host" => host}) do | ||
| GenServer.call(ref(name), {:host, host}) |
There was a problem hiding this comment.
This maybe a leaky abstraction. So now the Game module knows there is a GenServer running somewhere which handles this message. A more ideal approach would be to change this to Monitor.update(ref(name), {:host, host})
That way later on if you wish to move away from a GenServer to some other behaviour, the change would just be in the Monitor module.
So I would rather do it by moving this line to Monitor module and expose it as a function, which can then be called from Game module
| case Game.start do | ||
| {:ok, name} -> | ||
| Game.update(name, host) |
There was a problem hiding this comment.
If host is guaranteed to be present at the time of Game gen_server creation, maybe we can include this in the Monitor's init function instead.
Currently we are waiting to create a GenServer, and once it is created we are sending it a message again.
This is okay, but not optimal in the strictest sense.
| Game.update(name, Map.take(params, ["participant"])) | ||
| render_json(conn, :accepted, %{name: name}) |
There was a problem hiding this comment.
The Game.update will invoke a handle_call callback in `Monitor, which is sync. If sufficiently large number of people join the same group at the same time, the laggards will see high latency in http response. Maybe we can change the participant list update to be async by moving it to handle_cast.
This again is alright, I am just trying to highlight the scenarios in which the Pencil server may run into trouble.
2 participants
RoomChanneltoGameChannel