-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up Lobby & Server in general #261
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sieht soweit gut aus.
Zwei Dinge:
- gab es keine Moeglichkeit beim Refactoring des GameRooms ein paar mehr Tests zu erstellen?
- GameResult ist relevant fuer XStream. In der Vergangenheit hatten wir Probleme mit XStream wenn annotierte Klassen nach Kotlin umgewandelt wurden. Hast du verifiziert, dass das XML korrekt erstellt und geparst wird?
0a875d3
to
4dc50db
Compare
2c2e49b
to
115bd47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good as far as I understand the changes
6032a1d
to
ca82f1a
Compare
# Conflicts: # server/src/sc/server/Lobby.kt # server/src/sc/server/network/ClientManager.java # server/src/sc/server/network/NewClientListener.java
…gh a separate method # Conflicts: # server/src/sc/server/Lobby.kt # server/src/sc/server/network/ClientManager.java # server/src/sc/server/network/NewClientListener.java
# Conflicts: # server/src/sc/server/Lobby.kt # server/src/sc/server/network/ClientManager.java # server/src/sc/server/network/NewClientListener.java
# Conflicts: # server/src/sc/server/Lobby.kt # server/src/sc/server/network/ClientManager.java # server/src/sc/server/network/NewClientListener.java
# Conflicts: # server/src/sc/server/Lobby.kt # server/src/sc/server/network/ClientManager.java # server/src/sc/server/network/NewClientListener.java
# Conflicts: # server/src/sc/server/Lobby.kt # server/src/sc/server/network/ClientManager.java # server/src/sc/server/network/NewClientListener.java
# Conflicts: # server/src/sc/server/Lobby.kt # server/src/sc/server/network/ClientManager.java # server/src/sc/server/network/NewClientListener.java
Previously it didn't have an equals method, making proper testing almost impossible. Also removes isRegular from the XML. # Conflicts: # server/src/sc/server/Lobby.kt # server/src/sc/server/network/ClientManager.java # server/src/sc/server/network/NewClientListener.java
4ffb31a
to
7d78e08
Compare
7d78e08
to
8630863
Compare
@anarchuser the GameResultTest is failing because the ITeam is not serialized like the PlayerColor by XStream. We need to investigate that. |
@SKoschnicke I am not quite sure how much there is to test in the GameRoom |
@SKoschnicke @anarchuser please do a final review, then I'll merge this with a regular commit - finally! |
|
||
fun size(): Int = parts.size | ||
|
||
val values: List<BigDecimal> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we want to get rid of BigDecimal?
Or is there a good reason to keep it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the scores will ever go out of int range. Can't think of a good reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we average them, they may not be int anymore. And if we get floating-point errors in a championship scoring, we have a problem. Better safe than sorry I'd say.
7a9d754
to
8de7a00
Compare
Many useful more or less independent changes - reviewing the commits independently might be easier, this will also be a rebase merge :)