Game.management

[matthewphilyaw]

Working on the character and game management. Covers #21, #16 , #22

• game model
• character model
• test character and game models
• switch to character_id as the key in the player registry and this includes changing the player topic to be character id
• game controller that supports GET for now
• tests for game controller
• character controller to support CRUD
• tests for character

[matthewphilyaw]

@copenhas we may want to include user_id if we wanted to limit a user to having only one character per game. Thoughts? I'm not too concerned with it at the moment, I figured at least those two fields should be good enough.

[copenhas]

A user should be allowed to have multiple characters in a game.

[matthewphilyaw]

Cool, then this constraint satisfies this. It only fires if the same character name is used twice in a game but a user can have multiple characters. I have tests proving all that out (I think I covered the case to ensure one user and two characters are possible as long as the name is different...)

[matthewphilyaw]

I believe for the moment, the db changes will suffice and I will start writing some tests. These two models don't have a lot on them, so not a whole lot to validate at the moment. Maybe some minimum lengths for names and such would be advantageous and will address that when I start hammering out the tests.

[matthewphilyaw]

There is a setup function that can be used that runs before each test, the reason I avoided it at the moment is because it's defined in the ModelCase and starts the db transaction which is imported via the using at the top.

I have few ideas around this that I need to review, but maybe we can clean thus up a bit.

[copenhas]

ExUnit supports multiple setup functions. The documentation kind of suggests the idea, but looking at the source for the macro looks like it does. Each time you call the setup/2 macro it creates a uniquely named function and adds it to the setup list.

[matthewphilyaw]

Awesome, I should be able to adjust my setup function and then take a context in each function instead.

[matthewphilyaw]

@copenhas I think the suite of tests I have for now will suffice. May want to review and make sure I didn't miss something glaring.

In fact as I'm typing this the following come to mind

1. Character name could have spaces
2. Character name could be mixed case
3. Game name could have spaces
4. Game name could be mixed case

I'm not sure the case is an issue for either of these, but the spaces in the character name we may not want.

[copenhas]

Is there any significance to this one? Doesn't save you anything below and it's not named something like valid_character or named_character.

[matthewphilyaw]

The name is a valid length and serves as the basis for all the tests that center around valid changesets and the combos where the game_id/user_id fire constraint errors. In those cases the name needs to be correct but the until you hit the DB you don't know if the foreign keys are correct for example.

Given that I refrained from calling it a valid_character, but perhaps there is better name - open for suggestions.

[copenhas]

Oh, yeah we would want to allow mixed casing and spaces in the character and game names. Although probably would want the character's name to be case insensitive in the database so that the index wouldn't allow Copenhas and copenHas.

[matthewphilyaw]

@copenhas good call on the multiple setups. This was more inline with what I wanted.

[matthewphilyaw]

@copenhas sorry for rebase, needed master.

[matthewphilyaw]

Elixir 1.2 introduced this syntax for importing aliases, there were enough here that I went for it, but maybe we can make a pass to switch to this new syntax. I think it's a little cleaner when you have several imports.

[matthewphilyaw]

@copenhas ok so I switched the player_channel around to accept character_id instead of user id, so the client is expected to join a topic in the form of player:123 where 123 is the character_id.

I believe this alone will allow us to support multiple games, there is no need for game_id in the player registry key.

As far as when the player process starts and it interacts with the game - it has the character id, so it can find the game. A lot of details there we just don't have at the moment.

[copenhas]

Cool. So it looks like you didn't have to change any of the player registry code. You may want to still go in and change the variable names so they are consistent.

[matthewphilyaw]

Ah, yes good point faintly remember some variables in there now that you mention it, I forgot to look in the player registry when I was doing this last night.

[matthewphilyaw]

Hmmm, overlooked the "user" key here.

[matthewphilyaw]

I believe I sorted out all the naming issues, a few slipped by grep because I wasn't look for just "user" when I did this originally but should be good now.

[matthewphilyaw]

@copenhas right now only have two tests that cover the happy case.

right now with only allowing get the only other major one we may want to cover is the case where there are no games, which should return an empty list to the client. I'm sure there are others as well,

[matthewphilyaw]

@copenhas may want to look this over a little. This may not be perfect yet, mainly trying to bang out the controllers that are "good enough" to give us characters and games and then come back to polish it at some point.

I need tests for character controller as well. Will try to work on that tomorrow.

[matthewphilyaw]

@copenhas in-light of the changes you did with splitting the app, I don't think I want to do this in the controller.

I'm going to have to do some clean up and reorganize anyway with this change now in master so do you have maybe some suggestions on how we cleanly present these sorts of things to the web layer?

# Almost in favor of having Game
Yggdrasil.Game.list_games()
Yggdrasil.Game.get_game(123)
Yggdrasil.Game.list_characters()
Yggdrasil.Game.get_character(123)
Yggdrasil.Game.add_character(...)

We do have a game module for the game struct, but either we can call that something like GameModel or use module names to differentiate (not sure how I feel about that yet, but may be an option) so like Yggdrasil.Model.Game for the game model, and Yggdrasil.Game for the api exposed to the web... Again GameModel maybe a better a choice there but suggesting both here.

[copenhas]

What's the aversion to using the Ecto repo here? I'm not sure what the best practice should be so it might take some discussion.

The example you gave above is like making the Game module a repository doing nothing more but hiding your data access. I know with NHibernate that became an anti-pattern because it makes it more difficult to take full advantage of your data mapper and query library, in this case Ecto. The other issue is that you end up with a single module that have to keep changing as things need to retrieve the model differently as well as you have logic to tie to it. You can argue that it breaks SRP. I know in .NET land we might use query objects when something was reusable or to complex to stay inline. Then straight NHibernate usage otherwise. Experience has shown that only the most basic queries end up being reused, most seem to be specialized for a particular case.

I think using Ecto directly here is fine, especially to start with.

That doesn't handle the question of what's our convention for naming the data representation versus the process. I kind of like using Yggdrasil.Models.* for all the ecto schema stuff, or maybe call it Yggdrasil.Schema.*. Then you can organize the rest normally, but I think we'll want to be careful what we name things either way. Yggdrasil.Schema.Room and Yggdrasil.Room will be easy to confuse with an alias in the file. Maybe the data structure should win and be called the thing, then try to name the process something special? Maybe:

Yggdrasil.Schema.Character
Yggdrasil.Schema.Room
Yggdrasil.ActiveRoom # or RoomServer

[matthewphilyaw]

Ah no the goal isn't a repo but a game engine Api. This branch hasn't been updated yet but we split apps.

I'd rather not let the game engine internsls bleed out of the Yggdrasil app so why I'm suggesting we introduce an api.

So my goal is to let the game engine manage the life cycle of its core assets like character. I'm not sure the web needs to.

So while I said Yggdrasil.Game the intent is a module that lets you interact with the game engine.

So for example in my examples of chracater any modification or removing probably should be checked to ensure the character isn't in a running game or active and if so either error or notify the room etc. Also if there are any other things like creating a character internally updates several tables to place in a default room etc the game engine is best suited for this not the web.

So add_character in my example above maybe a direct mapping temporarily it will likely expand. Thus we can do that and not modify the web.

All that said internal to the game engine if you need a character im all for using a repo and totally agree that that is best. I just don't think we want to use repo directly outside of the app. Harder to test and spreads you game engine logic out beyond the game engine app. Also the web will have limited set of queries I think no need for full queryiny as it will only have very specific things it needs. I think.

Naming wise I'm good with what you suggested.

[copenhas]

Yeah but the game engine will never need to worry about removing characters and such, that'll be a web thing. Also the web will need abilities to search but the game engine will most likely only perform id based lookups. Other administration things like deactivating old accounts and such, the game engine doesn't care about any of that.

The issue with a character requiring updating multiple tables might be handled for us when Ecto hits 2.0, but either way I get that we might need to write a module to hide that stuff behind. I do question if everything the web API will need should be in Yggdrasil. So maybe I should ask do you see Yggdrasil encompassing everything?

[matthewphilyaw]

So I see Yggdrasil encompassing the game engine. The otp app to me isn't
just for code organization it symbolizes an independent running app.

I don't see it containing a user though. I think YggdrasilWeb should
contain the user and all administrative stuff. Possibly even having its own
repo.

So if we do this we achieve two isolated apps where the web interacts with
game engine through a module but doesn't know or care about its internals.

For example a character is something that I would say is a game engine
component. Lots of things in the game engine will depend on it and use it.
Thus to me it should own it and mange its life cycle.

To your point yes the web exposes an API to remove a character but I'll be
shocked if removing a character will not need some sort of logic wrapped
around it to ensure its not in an active room this being used if so either
error or notify the room. In addition there maybe other clean up and checks.

So if the web API handles this in the controller for example then the
controller has to know all this logic if it's going to directly manipulate
the repo. This also means tests for ensuring the stability on the running
game character are now in the controller. Because the game engine app can't
test it. It has no idea a character will be removed.

However if this is in an API like I suggested above Yggdrasil can ensure a
vet the management of a characters life cycle that it depends on. Makes it
easier to test I think. It keeps the test local to the pieces that depend
on it heavily.

It's my view anyway kind of how I have this split up. I think it's best to
think of otp apps as black boxes that expose its functionality.
On Thu, Feb 18, 2016 at 10:24 PM Sean Copenhaver [email protected]
wrote:

In web/controllers/game_controller.ex
#23 (comment):

@@ -0,0 +1,11 @@
+defmodule Yggdrasil.GameController do

• use Yggdrasil.Web, :controller

• alias Yggdrasil.Game

• def index(conn, _params) do
• games = Repo.all Game

Yeah but the game engine will never need to worry about removing
characters and such, that'll be a web thing. Also the web will need
abilities to search but the game engine will most likely only perform id
based lookups. Other administration things like deactivating old accounts
and such, the game engine doesn't care about any of that.

The issue with a character requiring updating multiple tables might be
handled for us when Ecto hits 2.0, but either way I get that we might need
to write a module to hide that stuff behind. I do question if everything
the web API will need should be in Yggdrasil. So maybe I should ask do
you see Yggdrasil encompassing everything?

—
Reply to this email directly or view it on GitHub
https://github.com/voluspa/yggdrasil/pull/23/files#r53418983.

[matthewphilyaw]

Yeah, more of a fan of a suite of functions that do one thing well that can be composed than overly specific functions where it's like oh you need to call this one for that action or this other for that action and they may be disjoint where you need something in between.

A user for example I would rather the web completely own and have direct repo access, not that I'm shy or subtle about how I want to throw that back into the web 😉.

I do have hard time thinking of any one example where you would be doing straight CUD (doesn't read very well without the R...haha) on any game related table though that the game isn't better suited in doing.

My opposition to direct access to the Game's repo is that it allows unchecked abilities to remove or update records that the web will most likely never know if it can safely do. That said I do see where the query abilities would be needed definitely.

That said, it's not the repo that offers the querying, it's the Ecto.Query module, so there is no reason the APIs can't take in a ecto query to execute and return results. I'm not opposed to exposing the ecto schemas for such things or use else where.

Even still I have hard time picturing any game related tables that will be as simple as getting only data from the tables. Like if you want an admin to see characters in a game, but you want to allow the admin to say show me only characters actively playing the game. That is no longer just a db question, and maybe the it's a Character.query() function at that point where you give ecto query and it augments the results coming back to include an active flag which the UI can use to filter for example or can filter before the UI.

I was sort taking the stance that it's easier to slowly open up and expose access to underlying things than start wide open and trying to reign it back in. Like any case where what we provide doesn't satisfy a certain need we can step back and say ok what combo is needed what is the impact of that, and we can test it out isolated instead of through controllers etc. Then we end up with the bare minimum api we need as we go.

All that said, I'm ok if you prefer to start with direct repo access and iteratively pull it back into yggdrasil as well. I see your points and concerns as well.

[matthewphilyaw]

Btw I'm also not in favor of this any more

Yggdrasil.Game.list_games()
Yggdrasil.Game.get_game(123)
Yggdrasil.Game.list_characters()
Yggdrasil.Game.get_character(123)
Yggdrasil.Game.add_character(...)

What ever this turns into, I think it's ok to have several modules that are composed instead of one like this. If we determine a need to put a process around character creation for example Yggdrasil.Character.create() or something along those lines but point being that I think having task specific modules are probably better than one module where the world of api calls hangs off of.

[copenhas]

I'm thinking at first a lot of things will probably be straight CRUD and eventually several things will be more and more complex. That's probably why I favor starting with direct repo calls.

I do agree the big thing is that we'll want full querying abilities, so the least we that exposed. It's possible that there are other things.

I know with NHibernate a big reason you want to push the session access up in the layers is because you can take advantage of batched queries, eager/lazy loading, but I can't think of anything around CUD that benefits from direct access since the models and the mapping take care of that (which Ecto 2.0 will improve).

Anyway, I'm cool with your idea of wrapping up the CRUD with the module but allowing the "Read" function to take an ecto query. You addressed my primary concerns in previous comments.

I do want to avoid using primitives everywhere though. Maybe our modules should work off of the structs, so you first gotta query what you want.

[matthewphilyaw]

Good point about eager/lazy loading, I actually think that can be entirely handled in a query according to this. If you scroll down a bit it mentioned the use directly in a query.

Ecto seems to be split up nicely, so like for example Sessions in Nhibernate provide the tracking of changes and batching of those changes, where as Ecto relies on changesets and I don't think the Repo keeps track of any of that. Same with querying, the Repo doesn't provide it just executes the query passed in.

So it will be interesting to see how we organize compared to our experiences with Nhibernate. I'm really curious to see and see what they bring in 2.0.

I also completely agree with the use of primitives, that should be restricted to only places where it's needed which hopefully is rare.

[copenhas]

👍

[matthewphilyaw]

@copenhas rebased on master to pull in the new dir structure.

Going to try to knock out that last test case for character controller. For now I left the direct repo access in the controllers. Ideally would like to work on taking that out at some point, but it was already there so and would rather get this merged in to give us characters and games.

[matthewphilyaw]

Also coverage drop is due to character controller that has no tests, which I'm working on.

[matthewphilyaw]

@copenhas you may want to read over this if you get time. I will put some more comments in later tonight there are somethings that I'm sure you will have questions about and want to bring up.

Anyway I have added two tests so far one for listing out all the characters for a user and one for getting one character by id. When I get some time tonight I will explain the route I went.

Still need to cover create/delete. I don't have update at the moment but will add that as we need it.

There are also some tests around ensuring that the characters coming back are for the user that is logged in and not just any user. So lots of good stuff left to test but it's a start and also already has shaken out a few things around our auth process.

[matthewphilyaw]

After studying how JaSerializer tests their stuff noticed that they just use the view directly like this.

That said I think JaSerializer has format method that it uses internally that accepts a connection and formater.

Seeing this example showing that connection can be empty I think we could use it directly and pass the view in as the formatter. Not sure what is best here but this works better than hand coding a solution like I have in other tests.

I think after this branch I need to make a sweep at our tests and make them more consistent.

[matthewphilyaw]

Server knows what user is posting this, so it assumes the user_id needs to be that and just clobbers what the client sent and sets it on the server. I also need another test to prove this out btw.

The obvious concern here is an admin wanted to create a character for a user he can't. The counter to that, is that we don't want a user creating characters for other users.

The larger question here, which also applies to the other actions in this /characters endpoint in general is should we have two? I'll point this out in the character file may be more clear there.

[coveralls]

Coverage increased (+6.7%) to 80.751% when pulling f595d80 on game.management into 5482910 on master.