SlothBusters/gruppe02-slothbusters-projekt3-catan
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases 1 Packages Wiki Activity Actions

Add a Controller Class for Running the Game #18

Merged
thalmma5 merged 16 commits from feature/controller-implementation into dev 2021-12-02 23:15:32 +00:00
Conversation 7 Commits 16 Files changed 12 +688
thalmma5 commented 2021-12-01 17:30:03 +00:00 (Migrated from github.zhaw.ch)
Copy link

Changes made in this PR will add a new Controller class which is able to run and handle a SiedlerGame.

Merging this PR will close #6

Changes made in this PR will add a new `Controller` class which is able to run and handle a `SiedlerGame`. Merging this PR will close #6
aebertan (Migrated from github.zhaw.ch) reviewed 2021-12-01 17:30:03 +00:00
costajon (Migrated from github.zhaw.ch) reviewed 2021-12-01 19:40:35 +00:00
costajon (Migrated from github.zhaw.ch) left a comment
Copy link

Nicely done. I couldn't find any apparent bugs and the documentation is good. I suppose any potential issues will eventually surface when we're done writing tests for the game.

Nicely done. I couldn't find any apparent bugs and the documentation is good. I suppose any potential issues will eventually surface when we're done writing tests for the game.
app/src/main/java/ch/zhaw/catan/Controller.java
costajon (Migrated from github.zhaw.ch) commented 2021-12-01 18:18:47 +00:00
Copy link
                        .println(String.format("Team %s, please place a settlement and a road.", faction));
```suggestion .println(String.format("Team %s, please place a settlement and a road.", faction)); ```
app/src/main/java/ch/zhaw/catan/Controller.java
@ -0,0 +49,4 @@
/**
* Runs a Catan game.
*/
public void run() {
costajon (Migrated from github.zhaw.ch) commented 2021-12-01 18:35:21 +00:00
Copy link

Overall very good. Inline comments would drastically improve readability in my opinion. Especially around the big for-blocks.

Overall very good. Inline comments would drastically improve readability in my opinion. Especially around the big for-blocks.
app/src/main/java/ch/zhaw/catan/Controller.java
@ -0,0 +51,4 @@
*/
public void run() {
int playerCount = io.newIntInputReader().withMinVal(2).withMaxVal(4)
.read("What's the number of players you'd like to play with?");
costajon (Migrated from github.zhaw.ch) commented 2021-12-01 18:15:01 +00:00
Copy link

This works nicely. However, TextIO's default response (Expected an integer value between 2 and 4) isn't too user friendly in my opinion. I think we should change it something more natural if it isn't too much work. This would be low priority, of course.

This works nicely. However, TextIO's default response (`Expected an integer value between 2 and 4`) isn't too user friendly in my opinion. I think we should change it something more natural if it isn't too much work. This would be low priority, of course.
app/src/main/java/ch/zhaw/catan/interaction/BuildAction.java
costajon (Migrated from github.zhaw.ch) commented 2021-12-01 18:44:39 +00:00
Copy link
public abstract class BuildAction<T> extends Action {
```suggestion public abstract class BuildAction<T> extends Action { ```
costajon (Migrated from github.zhaw.ch) commented 2021-12-01 18:47:09 +00:00
Copy link

Missing documentation?

Missing documentation?
zumbrseb commented 2021-12-01 19:45:05 +00:00 (Migrated from github.zhaw.ch)
Copy link

Generally i like the structure of the code. The TradeAction and BuildSelectionAction could also be implemented in just a simple method. This however would break the consistency of actions. So I would probably just leave it like that.

Generally i like the structure of the code. The `TradeAction` and `BuildSelectionAction` could also be implemented in just a simple method. This however would break the consistency of actions. So I would probably just leave it like that.
zumbrseb (Migrated from github.zhaw.ch) requested changes 2021-12-01 19:46:12 +00:00
zumbrseb (Migrated from github.zhaw.ch) left a comment
Copy link

I thought I started a review but apparently not...

I thought I started a review but apparently not...
app/src/main/java/ch/zhaw/catan/Controller.java
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 18:38:49 +00:00
Copy link
        for (Direction direction :  Direction.values()) {
```suggestion for (Direction direction : Direction.values()) { ```
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 18:50:53 +00:00
Copy link

The getIO().getTextTerminal()... spiel could be either abstracted into a separate class or into a method print(message, color). This would also reduce boilerplate code quite a bit.

The `getIO().getTextTerminal()...` spiel could be either abstracted into a separate class or into a method `print(message, color)`. This would also reduce boilerplate code quite a bit.
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 18:51:44 +00:00
Copy link

Maybe with an overloaded method that just takes a message and uses the previously defined color.

Maybe with an overloaded method that just takes a message and uses the previously defined color.
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 18:54:37 +00:00
Copy link

This does the same thing but without a variable. A bit less complexity.

                switch (actionType) {
                    case Build:
                        new BuildSelectionAction(game).execute();
                        break;
                    case Trade:
                        new TradeAction(game).execute();
                        break;
                    default:
                        break;
                }
This does the same thing but without a variable. A bit less complexity. ```suggestion switch (actionType) { case Build: new BuildSelectionAction(game).execute(); break; case Trade: new TradeAction(game).execute(); break; default: break; } ```
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 18:56:27 +00:00
Copy link

This for loop could also be extracted into a separate method which could be used in the runTurn() method. This would probably also increase readability.

This for loop could also be extracted into a separate method which could be used in the `runTurn()` method. This would probably also increase readability.
app/src/main/java/ch/zhaw/catan/Controller.java
@ -0,0 +49,4 @@
/**
* Runs a Catan game.
*/
public void run() {
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 18:50:41 +00:00
Copy link

This method is really long and could surly be split into runInitialTurn() and runTurn(). The variables game and textView would become a field.

This method is really long and could surly be split into `runInitialTurn()` and `runTurn()`. The variables `game` and `textView` would become a field.
app/src/main/java/ch/zhaw/catan/interaction/BuildAction.java
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 19:31:12 +00:00
Copy link

Instead of having to override getX/YCoordinatePrompt(), fetchCoordinate() could just have two arguments where the those prompts are given. This would make the classes smaller.

Instead of having to override `getX/YCoordinatePrompt()`, `fetchCoordinate()` could just have two arguments where the those prompts are given. This would make the classes smaller.
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 19:32:23 +00:00
Copy link

This is never really used apart from giving TData to buildStructure. buildStructure() could ask the user itself and then the generics wouldn't be necessary and the classes are smaller

This is never really used apart from giving TData to buildStructure. `buildStructure()` could ask the user itself and then the generics wouldn't be necessary and the classes are smaller
app/src/main/java/ch/zhaw/catan/interaction/BuildSelectionAction.java
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 19:37:14 +00:00
Copy link

We are even allowed to use this as it was discussed in the presentation "04-2_do-while_switch_IT21ta_ZH.pdf" ;)

        Structure structureKind = getIO().newEnumInputReader(Structure.class)
                .read("What kind of building do you want to buy?");
        return switch(structureKind) {
            case SETTLEMENT -> new SettlementAction(getGame()).execute();
            case ROAD -> new StreetAction(getGame()).execute();
            case CITY -> new CityAction(getGame()).execute();
            default -> false;
        };
    }
We are even allowed to use this as it was discussed in the presentation "04-2_do-while_switch_IT21ta_ZH.pdf" ;) ```suggestion Structure structureKind = getIO().newEnumInputReader(Structure.class) .read("What kind of building do you want to buy?"); return switch(structureKind) { case SETTLEMENT -> new SettlementAction(getGame()).execute(); case ROAD -> new StreetAction(getGame()).execute(); case CITY -> new CityAction(getGame()).execute(); default -> false; }; } ```
app/src/main/java/ch/zhaw/catan/interaction/BuildingAction.java
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 19:16:31 +00:00
Copy link

This class could be completly omited as it isn't really needed as a type in the type hierarchy. And if getStructureInfo() is removed as well, then it doesn't add much code.

This class could be completly omited as it isn't really needed as a type in the type hierarchy. And if `getStructureInfo()` is removed as well, then it doesn't add much code.
app/src/main/java/ch/zhaw/catan/interaction/InitialSettlementAction.java
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 19:40:48 +00:00
Copy link

This constructor is never used. If it is necessary later, it is easy to add.

This constructor is never used. If it is necessary later, it is easy to add. ```suggestion ```
app/src/main/java/ch/zhaw/catan/interaction/StreetAction.java
@ -0,0 +10,4 @@
/**
* Provides information about a street.
*/
public static class StreetInfo {
zumbrseb (Migrated from github.zhaw.ch) commented 2021-12-01 19:41:57 +00:00
Copy link

If the getStructureInfo() method is omitted, then this could be made private.

If the `getStructureInfo()` method is omitted, then this could be made private.
costajon (Migrated from github.zhaw.ch) reviewed 2021-12-02 15:05:27 +00:00
app/src/main/java/ch/zhaw/catan/interaction/BuildAction.java
costajon (Migrated from github.zhaw.ch) commented 2021-12-02 15:05:27 +00:00
Copy link

Change all instances of TData to T

Change all instances of `TData` to `T`
costajon (Migrated from github.zhaw.ch) requested changes 2021-12-02 15:09:47 +00:00
costajon (Migrated from github.zhaw.ch) left a comment
Copy link

Request changes

Request changes
zumbrseb (Migrated from github.zhaw.ch) approved these changes 2021-12-02 16:30:47 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
aebertan
costajon
zumbrseb
Labels
Clear labels   
bug

Something isn't working

  
ci

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
question

Further information is requested

  
wontfix

This will not be worked on

No labels
bug
ci
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Task 1: Base game done
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: SlothBusters/gruppe02-slothbusters-projekt3-catan#18
No description provided.
Delete branch "feature/controller-implementation"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?