CodinGame / codingame-game-engine

CodinGame Engine
https://www.codingame.com
MIT License
113 stars 41 forks source link

Refactor GraphicEntityModule by exracting serializer to a smaller class. #10

Closed A4Vision closed 6 years ago

A4Vision commented 6 years ago

Hello @nantoniazzi , would you like to review this PR ?

CGjupoulton commented 6 years ago

Why use a new intermediary class instead of putting this code directly in Serializer ? Adding more classes complexifies the project's architecture, Especially when two classes have practically the same name and purpose (module.entities.Serializer & GraphicEntitySerializer).

refactormachine commented 6 years ago

I see your point, the class game.entities.serializer is used only here.

So I moved the function dumpWorldStateDiff into WorldState and renamed it as serializeDiffFromPrevWorldState. More, I refactored it to be more readable.

What do you think @CGjupoulton ?

CGjupoulton commented 6 years ago

It is indeed more readable, but I don't like seeing a "serialize" method other than in the Serializer class. I think I'll rename serializeDiffFromPrevWorldState() to diffFromOtherWorldState(). It will be more consistent with EntityStates's diffFromOtherState().

EDIT: Oh it appears this method actually needs the serializer. Everything serialization-related should go in the Serializer. We should separate this method in two: 1) creation of diff. 2) serialization

CGjupoulton commented 6 years ago

To summarize my previous comment, I think the WorldState diff function should act the same as the EntityState diff function. Namely, a new state is created with only the differences, then that is passed to the Serializer.

EDIT: Which I have now done on our side.