jdmonin / JSettlers2

Java Settlers project home, downloads, and GPLv3 source code. To download the latest version as a JAR, see https://github.com/jdmonin/JSettlers2/releases/latest .
http://nand.net/jsettlers/
GNU General Public License v3.0
157 stars 63 forks source link

Split SOCBoard into Standard4p and Standard6p. Tidy up SOCBoard. #38

Closed generateui closed 6 years ago

generateui commented 6 years ago

SOCBoard is a "God-class". It knows too much about different types of boards and it does too much. Furthermore, there's a lot of unnecessary complexity.

This PR tries to move responsibilities into different classes: Standard4p and Standard6p. This can be considered a first step into breaking down complexity. The next step is to remove magic numbers and/or move grid logic into vertex/edge/location classes.

I set the target branch to v3, so v1/v2 is not affected.

jdmonin commented 6 years ago

Thanks for looking into refactoring this big class. I'm still reviewing your changes, but please watch that refactoring changes and style changes aren't mixed together too much; that can be distracting.

Please remember these things from the project's coding style:

Thanks again!

generateui commented 6 years ago

Sorry for mixing style changes and structural changes. That does not help reviewing the changeset indeed.

For both the boolean parentheses and /* javadoc / inside methods, I made these changes since they are advised by all my IDE's (Visual Studio, ReSharper, IntelliJ).

Javadoc (/*, 2-star start aka begin-comment delimiter) is used by the Javadoc tool to generate documentation. usually that's for public members, and not used inside a method scope. For this, we should probably use / (single star). Furthermore, we should probably breakup longer methods into smaller ones. YMMV.

Any reason to mandate parentheses in boolean evaluation? I think the reason my IDE's advise against it is because it is shorter and therefore more clear. In my personal style I try to always do single evaluations, like so:

boolean isSettlement = player.settlementToPlace != null;
boolean canPlace = board.canPlace(piece);
boolean placeSettlement = isSettlement && canPlace;
if (placeSettlement) { ... }

instead of

if (player.settlementToPlace != null && board.canPlace(piece)) { ... }

because the first example is much easier to read. Using that style also prevents boolean logic errors caused by incorrect operator precedence placement.

I'll update the changeset.