Fix-bugs #41

2021-12-09T21:17:39Z

zumbrseb commented

2021-12-09 21:17:39 +00:00

(Migrated from github.zhaw.ch)

This PR fixes random bugs I found while testing

thalmma5 (Migrated from github.zhaw.ch) reviewed

2021-12-09 22:54:58 +00:00

thalmma5 (Migrated from github.zhaw.ch) left a comment

Thanks for the effort!
This PR looks good to me.

However - is it really a good idea to have a separate SiedlerGameHelper class instead of placing its methods directly into the SiedlerGame class?

Thanks for the effort! This PR looks good to me. However - is it really a good idea to have a separate `SiedlerGameHelper` class instead of placing its methods directly into the `SiedlerGame` class?

app/src/main/java/ch/zhaw/catan/SiedlerBoard.java

					
				@ -33,2 +33,3 @@

				                || hasAdjacentStreet(startPoint, endPoint, owner)) && hasEdge(startPoint, endPoint)

				                && getEdge(startPoint, endPoint) == null;

				                && getEdge(startPoint, endPoint) == null && isCornerTouchingLand(startPoint)

				                && isCornerTouchingLand(endPoint);

thalmma5 (Migrated from github.zhaw.ch) commented

2021-12-09 22:52:08 +00:00

Awesome fix! Thanks a ton for the effort

app/src/main/java/ch/zhaw/catan/SiedlerGameHelper.java

thalmma5 (Migrated from github.zhaw.ch) commented

2021-12-09 22:53:24 +00:00

I'm not quite sure but isn't this sort of misplaced in a separate class when taking cohesion into account?

costajon (Migrated from github.zhaw.ch) requested changes

2021-12-09 22:57:51 +00:00

app/src/main/java/ch/zhaw/catan/SiedlerGameHelper.java

costajon (Migrated from github.zhaw.ch) commented

2021-12-09 22:56:24 +00:00

            textTerminal.println("Your inventory contains:");

```suggestion textTerminal.println("Your inventory contains:"); ```

app/src/main/java/ch/zhaw/catan/interaction/TradeAction.java

					
				@ -22,9 +23,21 @@ public class TradeAction extends Action {

				    @Override

				    public boolean execute() {

				        InventoryPrinter helper = new InventoryPrinter(getGame());

costajon (Migrated from github.zhaw.ch) commented

2021-12-09 22:55:21 +00:00

                    "The trade was not successful. You don't have enough resources to trade 4 %s for 1 %s\n",

```suggestion "The trade was not successful. You don't have enough resources to trade 4 %s for 1 %s\n", ```

zumbrseb commented

2021-12-09 23:08:53 +00:00

(Migrated from github.zhaw.ch)

Thanks for the effort!
This PR looks good to me.

However - is it really a good idea to have a separate SiedlerGameHelper class instead of placing its methods directly into the SiedlerGame class?

I agree 100% with you, but as far as I know, we aren't allowed to change the public api of SiedlerGame. So...this is the work around...

> Thanks for the effort! > This PR looks good to me. > > However - is it really a good idea to have a separate `SiedlerGameHelper` class instead of placing its methods directly into the `SiedlerGame` class? I agree 100% with you, but as far as I know, we aren't allowed to change the public api of SiedlerGame. So...this is the work around...

zumbrseb (Migrated from github.zhaw.ch) reviewed

2021-12-09 23:09:49 +00:00

app/src/main/java/ch/zhaw/catan/SiedlerGameHelper.java

zumbrseb (Migrated from github.zhaw.ch) commented

2021-12-09 23:09:49 +00:00

absolutly, Especially the getPlayerInventory method would belong into SiedlerGame, but we aren't allowed to change the public api, so....

Sign in to join this conversation.

No reviewers

costajon