Code review #2 #9
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ded some simple genrate methods.
…ning is available now.
Merged network into dev
Merged db into dev
… refactored GameState
There was a problem hiding this comment.
- класс Player связан с моделью только
Model.this.field.getSize(). Это можно передать в конструктор, чтобы убрать зависимость - неиспользуемые импорты
- неиспользуемые декларации
- не-final поля (везде)
- слишком сильная связность. Зачем игроку знать, что он победил (markThatPlayerWin)? Просто чтобы двумя строками ниже проверить
if (!players[index].playerWin)? Это можно превратить в локальную переменную. - слишком много int-ов. int - сам по себе - это число, которое никакого смысла не несёт. Используйте объекты.
- из-за предыдущей проблемы код трудно понимать, а ещё его трудно понимать из-за отсутствия документации, а так же потому что Turn.getId() - это не идентификатор хода, а индекс игрока в массиве игроков (внезапно!)
| borderY = new boolean[fieledSize][fieledSize + 1]; | ||
| public Field(int fieldSize) { | ||
| size = fieldSize; | ||
| field = new State[size][fieldSize]; |
There was a problem hiding this comment.
Или [size][size] или [fieldSize][fieldSize].
| public Field(int fieldSize) { | ||
| size = fieldSize; | ||
| field = new State[size][fieldSize]; | ||
| borderX = new boolean[fieldSize + 1][fieldSize]; |
| private boolean[][] borderY; | ||
| private int treasureX; | ||
| private int treasureY; | ||
| private int treasureOwner; |
There was a problem hiding this comment.
У вас тут int умеет владеть сокровищами.
Это поле должно называться treasureOwnerIndex, а ещё лучше оно должно иметь тип Player.
| private State[][] field; | ||
| private boolean[][] borderX; | ||
| private boolean[][] borderY; | ||
| private int treasureX; |
|
|
||
| private int size; | ||
| private State[][] field; | ||
| private boolean[][] borderX; |
There was a problem hiding this comment.
Вот это совсем непонятно.
Что означают индексы?
| private int hospitalX; | ||
| private int hospitalY; | ||
| private int exitBorderType; // 0 = X, 1 = Y | ||
| private int exitBorderX; |
| private int treasureOwner; | ||
| private int hospitalX; | ||
| private int hospitalY; | ||
| private int exitBorderType; // 0 = X, 1 = Y |
| } | ||
|
|
||
| public void addBorderY(int row, int column){ | ||
| public void addBorderY(int row, int column) { |
There was a problem hiding this comment.
addBorder(x,y) = setBorder(x,y,true)
There was a problem hiding this comment.
Или уберите этот метод, или выразите один через другой
| public int getPlayerNum() { | ||
| return playerNum; | ||
| } | ||
|
|
| import ru.spbau.labyrinth.model.Model.*; | ||
|
|
||
| public class GameState { | ||
| public Model getModel() { |
|
Как отличается int[] в getBorderInd от int[] в getPosChange ? Почему до сих пор методы принимают и возвращают int-ы ? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Часть с сетью ещё будем дописывать, но остальное вроде уже можно просмотреть.