Refactor game service

[NiallEgan]

In order to prepare for #734, I have made the following changes:

• Refactored service.py to remove global state and default arguments
• Removed provider classes
• (I also cleaned up the serialisation utilities)

These changes will make it easier to add and test a new socket event, as well as making the code generally clearer.

A disadvantage of this is that we can no longer use decorators to register the flask routes and events for socketio (we instead have to add them manually). While this is not quite as pretty, I think it is a necessary trade off as we add more functionality to service.py.

---

This change is 

[CelineBoudier]

Reviewed 13 of 19 files at r1.
Reviewable status: 13 of 19 files reviewed, 3 unresolved discussions (waiting on @NiallEgan)

---

aimmo-game/service.py, line 65 at r1 (raw file):

        return world_update_on_connect

    def make_disconnect_fun(self):

what's this name? :)

---

aimmo-game/service.py, line 97 at r1 (raw file):

    def _send_logs(self):
        def should_send_logs(logs):

any particular reason for using an inner function here?

---

aimmo-game-worker/simulation/world_map.py, line 16 at r1 (raw file):

            self.avatar = AvatarState(location=avatar['location'],
                                      score=avatar['score'],
                                      health=avatar['health'])

if some of the defaults are mutable make sure it does what you expect

[NiallEgan]

Reviewable status: 13 of 19 files reviewed, 3 unresolved discussions (waiting on @NiallEgan)

---

aimmo-game/service.py, line 65 at r1 (raw file):

Previously, CelineBoudier (Celine Boudier) wrote…

what's this name? :)

I think that it does in some way describe the method, although I guess it doesn't describe the inner method. Do you think that make_remove_session_id_from_mappings would be better?

---

aimmo-game/service.py, line 97 at r1 (raw file):

Previously, CelineBoudier (Celine Boudier) wrote…

any particular reason for using an inner function here?

This was at the recommendation of Niket in the last PR I made: we agreed that abstracting the branch condition into a function made for clearer reading, and as it is only used in this method I think an inner function makes sense.

---

aimmo-game-worker/simulation/world_map.py, line 16 at r1 (raw file):

Previously, CelineBoudier (Celine Boudier) wrote…

if some of the defaults are mutable make sure it does what you expect

I don't think these are defaults, but rather just normal keyword args.

[CelineBoudier]

Reviewed 6 of 19 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CelineBoudier)

---

aimmo-game/service.py, line 65 at r1 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

I think that it does in some way describe the method, although I guess it doesn't describe the inner method. Do you think that make_remove_session_id_from_mappings would be better?

i just don't think the current name makes sense as the way i read it it's making disconnect fun, fun, fun lol. Anyway it's a function so no need for fun in the name

---

aimmo-game-worker/simulation/world_map.py, line 16 at r1 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

I don't think these are defaults, but rather just normal keyword args.

no I mean the defaults of the parameters: for instance if avatar['location'] is mutable (I haven't checked sorry)

[NiallEgan]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @NiallEgan)

---

aimmo-game/service.py, line 65 at r1 (raw file):

Previously, CelineBoudier (Celine Boudier) wrote…

i just don't think the current name makes sense as the way i read it it's making disconnect fun, fun, fun lol. Anyway it's a function so no need for fun in the name

Oh well the reason that fun is in the name because it returns a function; it literally makes the function for disconnect. But I will change the name.

---

aimmo-game-worker/simulation/world_map.py, line 16 at r1 (raw file):

Previously, CelineBoudier (Celine Boudier) wrote…

no I mean the defaults of the parameters: for instance if avatar['location'] is mutable (I haven't checked sorry)

I'm not sure what you mean by defaults, I don't think there are any? Anyway all of the parameter values are immutable.

[NiallEgan]

Reviewable status: 17 of 19 files reviewed, 1 unresolved discussion (waiting on @CelineBoudier)

---

aimmo-game/service.py, line 65 at r1 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

Oh well the reason that fun is in the name because it returns a function; it literally makes the function for disconnect. But I will change the name.

Done.

[CelineBoudier]

Reviewable status: 17 of 19 files reviewed, 1 unresolved discussion (waiting on @CelineBoudier)

---

aimmo-game-worker/simulation/world_map.py, line 16 at r1 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

I'm not sure what you mean by defaults, I don't think there are any? Anyway all of the parameter values are immutable.

ok :) I mean that here avatar['location'] is the default value of the parameter location

[NiallEgan]

Reviewable status: 17 of 19 files reviewed, all discussions resolved (waiting on @CelineBoudier)

---

aimmo-game-worker/simulation/world_map.py, line 16 at r1 (raw file):

Previously, CelineBoudier (Celine Boudier) wrote…

ok :) I mean that here avatar['location'] is the default value of the parameter location

Maybe I'm misunderstanding something, but I don't think so? Default parameters are defined in the method definition, this is just a standard keyword argument that is passed in normally?

[CelineBoudier]

Reviewable status: 17 of 19 files reviewed, all discussions resolved (waiting on @CelineBoudier)

---

aimmo-game-worker/simulation/world_map.py, line 16 at r1 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

Maybe I'm misunderstanding something, but I don't think so? Default parameters are defined in the method definition, this is just a standard keyword argument that is passed in normally?

ah yeah no i hadn't read things properly my bad

[CelineBoudier]

Reviewed 3 of 3 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

[OlafSzmidt]

Reviewable status: 18 of 19 files reviewed, 1 unresolved discussion (waiting on @CelineBoudier and @NiallEgan)

---

aimmo-game/service.py, line 134 at r3 (raw file):

flask_app.add_url_rule('/player/<player_id>', 'player_data', game_api.make_player_data_view())

Why? Why not a route decorator?

[NiallEgan]

Reviewable status: 18 of 19 files reviewed, 1 unresolved discussion (waiting on @CelineBoudier and @NiallEgan)

---

aimmo-game/service.py, line 134 at r3 (raw file):

Previously, OlafSzmidt (Olaf Szmidt) wrote…

flask_app.add_url_rule('/player/<player_id>', 'player_data', game_api.make_player_data_view())

Why? Why not a route decorator?

Well I tried something like:

@app.route('/player/<player_id>')
def player_data_view(self, player_id):
...

The problem is that self won't be passed in as an argument when the method is called (only player_id will)

[OlafSzmidt]

Reviewable status: 16 of 19 files reviewed, 1 unresolved discussion (waiting on @CelineBoudier and @NiallEgan)

---

aimmo-game/service.py, line 135 at r4 (raw file):

    logs = Logs()

Remove irrelevant spacing here pls

[OlafSzmidt]

Reviewed 1 of 1 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @NiallEgan)

[NiallEgan]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @OlafSzmidt)

---

aimmo-game/service.py, line 135 at r4 (raw file):

Previously, OlafSzmidt (Olaf Szmidt) wrote…

Remove irrelevant spacing here pls

Done.

[OlafSzmidt]

Reviewed 14 of 19 files at r1, 1 of 3 files at r2, 3 of 3 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved