trjhtr

I have a method, that checks win conditions on a "Torus" board, which is a board without any borders. This means that if you place 4 diagonal stones on the top left, and 2 diagonal stones in the bottom right, if they are in the same diagonal and would connect if you ignored the border, which then would lead to a win. Basically it's a Connect 6 Game.

size() returns the size of the board which either is 18 or 20.

currentPlayer is a String like : "P1" or "P2".

r and c are the row and column where a move has just been made.

public boolean checkTorusWinner(int r, int c) 

 int count = 0;
 boolean hasWinner = false;
 String currentPlayer = board[r][c];
 int hSize = size();
 int vSize = size();

 /*
 Checks Horizontally for a Win.
 */
 for (int i = c; i < hSize; i++) 
 if (board[r][i] == currentPlayer) 
 count++;
 else 
 count = 0;
 
 if (i == size() - 1) 
 hSize = size() - 2;
 for (int j = 0; j < hSize; j++) 
 if (board[r][j] == currentPlayer) 
 count++;
 else 
 count = 0;
 
 if (count == 6) 

 boardType = "none";
 hasWinner = true;
 break;
 
 
 
 if (count == 6) 
 boardType = "none";
 hasWinner = true;
 break;
 
 
 /*
 Checks Vertically for a Win
 */
 for (int i = r; i < vSize; i++) 
 if (board[i][c] == currentPlayer) 
 count++;
 else 
 count = 0;
 
 if (i == size() - 1) 
 i = -1;
 vSize = size() - 2;
 
 if (count == 6) 

 boardType = "none";
 hasWinner = true;
 break;
 
 

 /*
 Checks Diagonally from Top left to Bottom right
 */
 if (c - r >= 0) 
 int startingC;
 startingC = c - r;
 int size = size();
 for (int i = r, j = c; j < size; i++, j++) 
 if (board[i][j] == currentPlayer) 
 count++;
 else 
 count = 0;
 
 if (j == size() - 1) 
 j = startingC - 1;
 i = -1;
 size = size() - 2;

 
 if (count == 6) 
 boardType = "none";
 hasWinner = true;
 break;
 
 
 else 
 int size = size();
 int startingR;
 startingR = r - c;
 for (int i = r, j = c; i < size; i++, j++) 
 if (board[i][j] == currentPlayer) 
 count++;
 else 
 count = 0;
 
 if (i == size() - 1) 
 j = -1;
 i = startingR - 1;
 size = size() - 2;

 

 if (count == 6) 
 boardType = "none";
 hasWinner = true;
 break;
 

 
 

 /*
 Checks Diagonally from bottom left to top right;
 */
 if (r + c <= 17) 
 int loop = 0;
 int startingR;
 startingR = r + c;
 for (int i = r, j = c; i >= 0; i--, j++) 
 if (board[i][j] == currentPlayer) 
 count++;
 else 
 count = 0;
 

 if (i == 0 && loop == 0) 
 i = startingR + 1;
 j = -1;
 loop++;
 

 if (count == 6) 
 boardType = "none";
 hasWinner = true;
 break;
 

 
 else if (r + c > 17) 
 int loop = 0;
 int startingC;
 startingC = (r + c) - (size() - 1);
 for (int i = r, j = c; i >= startingC; i--, j++) 
 if (board[i][j] == currentPlayer) 
 count++;
 else 
 count = 0;
 
 if (i == startingC && loop == 0) 
 i = size();
 j = startingC - 1;
 loop++;
 
 if (count == 6) 
 boardType = "none";
 hasWinner = true;
 break;
 
 
 

 return hasWinner;

Unfortunately this method's length doesn't meet the requirement for my university: it has to be a maximum of 80 lines. I don't know how I'm supposed to shorten this code so that it still works.

Thanks for sharing your code.

Your code is a procedural approach to the problem.

There is nothing wrong with procedural approaches in general, but Java is an object oriented (OO) programming language and if you want to become a good Java programmer then you should start solving problems in an OO way.

But OOP doesn't mean to "split up" code into random classes.

The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.

Doing OOP means that you follow certain principles which are (among others):

• information hiding / encapsulation


• single responsibility


• separation of concerns


• KISS (Keep it simple (and) stupid.)


• DRY (Don't repeat yourself.)


• "Tell! Don't ask."


• Law of demeter ("Don't talk to strangers!")

How might that help to improve your code?

From an OO point of view you have the current position and you have to check if that position is part of a line of at least 5 other (excluding itself) equal elements.

The first implication is that you only have to look at the current positions neighbors and that there is no need to scan the whole board.

The easieast way is to go in each direction and count the consecutive neighbors belonging to the current player. Afterwards you add the opposit directions and check the sum.

I use a trick to safely calculate an index in "wrap around" arrays:

(arrayLength + currentIndex + differece) % arrayLength 

where % is the modulo operator.

Here is how I would implement that:

 class FiledPosition
 final int r, c;
 FiledPosition(int r, int c)
 this.r=r;
 this.c=c;
 
 
 interface NeighborCalculator
 FiledPosition getFor(FiledPosition current);
 }

 enum Direction NORTH,NORTH_EAST,EAST,SOUTH_EAST,SOUTH,SOUTH_WEST,WEST,NORTH_WEST

the code above may live in separate classes. What follows must be in your solution class

 private final Direction opposits = new Direction
 NORTH,SOUTH, 
 NORTH_EAST,SOUTH_WEST,
 NORTH_WEST,SOUTH_EAST,
 EAST,WEST
 

 private final int WIN_COUNT_EXCLUDUNG_CURRENT = 5;
 Map<Direction, NeighborCalculator> neigborSelector = new HashMap<>();

public boolean checkTorusWinner(int r, int c) 

 neigborSelector.put(NORTH, new NeighborCalculator() // pre java8 anonymous inner class
 public FiledPosition getFor(FiledPosition currentPoint )
 return new Point((vSize+currentPoint.r-1)%vSize, currentPoint.c));
 
 );
 neigborSelector.put(NORTH_EAST,currentPoint -> new Point((vSize+currentPoint.r-1)%vSize, (hSize+currentPoint.c+1)%hSize)); // java8 lambda
 neigborSelector.put(EAST,currentPoint -> new Point(currentPoint.r, (hSize+currentPoint.c+1)%hSize));
 neigborSelector.put(SOUTH_EAST,currentPoint -> new Point((vSize+currentPoint.r+1)%vSize, (hSize+currentPoint.c+1)%hSize)); 
 // similar for all directions, should be in the classes constructor.

 Map<Direction, Counter> lineSectionCounts = new HashMap<>();
 String currentPlayer = board[r][c];
 int hSize = size();
 int vSize = size();

 // count consecutive same in each direction without current
 for(Direction direction : Direction.values())
 int consecutiveSame = 0;
 FiledPosition neigborPosition = neigborSelector.get(direction).getFor(new FiledPosition(r,c));
 while(currentPlayer.equals(board[neigborPosition.r][neigborPosition.c]))
 consecutiveSame++;
 neigborPosition = neigborSelector.get(direction).getFor(neigborPosition);
 
 lineSectionCounts.put(consecutiveSame); // auto boxed
 

 // sum up opposit directions
 for(Direction opposit : opposits)
 if(WIN_COUNT_EXCLUDUNG_CURRENT < lineSectionCounts.get(oposit[0]) + lineSectionCounts.get(oposit[1])) // auto unbox
 return true; // current Player won.
 
 return false; // no winner yet

This complete code has 57 lines (24 without the configuration). There are 4 lines missing to completly configure neigborSelector map (if you use jav8 lambdas).

This code uses basic Java concepts like classes, interfaces and enums you should already have heared of.

Some tips:

• obviously, break into smaller methods if allowed




• There are many magic numbers and strings. 17? 6? "none"? Put them in variables or constants to make the code more readable and easier to adjust.




• you are changing hSize value within a loop where hSize is the bound of the loop. This is very error prone!




• hasWinner = true; break; can be replaced by return true;




• modulo operator is fantastic for "looping" arrays. Example:
  length = 18;
  position = 17;
  array[(position) % length] == array[(position+1) % length]
  This will compare position 17 with position 0.




• code can probably be simplified and more generic by taking less regard to the position of the last move.




• the method sets board type to "none". I assume this resets the board. This makes the name of the method (checkTorusWinner) missleading




• pseudo code for very easy approach

For each x:
 For each y:
 Set xwin = ywin = diagFwdWin = diagBackwdWin = true. 
 For each n = 1... winLength-1:
 If board [x] [y] != board [(x+n) % boardwidth] [y] then
 Xwin =false
 End if
 // Same if, but instead, y+n % boardheight, Ywin
 // same if, but both x+n AND y+n, diagbackwdWin
 // same if, but both x+n and y MINUS n, diagfwdWin
 End for
 If Xwin or ywin or diagFwdWin or diagBackwdWin == true then
 Return true
 End if
 End for
End for
Return false

For better performance, x and y in outer loop can be limited to position of last added +/- winLength