Positional change program in Java

Question 1

I've written a program and it needs a review by skilled programmers.

The programs goal is to change0-s toX andX-s to0. The rule isX orO can move only to an empty place and x or o can only "jump" over oneX orO. Program looks like:

------------------|x|x|x|x| |o|o|o|o|------------------

User input

move from? (0-8)3move to? (0-8)4

Output

 -----------------|x|x|x| |x|o|o|o|o|------------------

package tornasz;import java.util.Scanner;public class Tornasz {    public static void main(String[] args) {        Scanner sc = new Scanner(System.in);        char[] table = {'x', 'x', 'x', 'x', ' ', 'o', 'o', 'o', 'o'};        int [] move = new int[2];        int result;        int counter = 0;        do        {            drawPlayfield(table, counter);            move = getValidMove(sc,table,move);            makeMove(table,move);            result = checkWin(table);            counter ++;        }        while (result == 0);        drawPlayfield(table,counter);        displayWinner(result);    }    public static void drawPlayfield(char [] table, int counter){        System.out.println("Lépésszám: "+counter);        System.out.println("------------------");        System.out.println("|"+table[0]+"|"+table[1]+"|"+table[2]+                "|"+table[3]+"|"+table[4]+"|"+table[5]+"|"+table[6]+"|"+table[7]        +"|"+table[8]+"|");        System.out.println("------------------");        counter++;    }    public static int [] getValidMove(Scanner sc, char [] table, int [] move)    {        System.out.println("Melyik mezővel lépsz? (0-8)");        int start = sc.nextInt();        if (start == -1)        {            System.exit(0);        }        System.out.println("Melyik mezőre lépsz? (0-8)");        int destination = sc.nextInt();        if (destination == -1)        {            System.exit(0);        }        while (start < 0 || start > 8 || destination < 0 || destination > 8)        {            System.out.println("Érvénytelen lépés!");            System.out.println("Melyik mezővel lépsz? (0-8)");            start = sc.nextInt();            if (start == -1)            {                System.exit(0);            }            System.out.println("Melyik mezőre lépsz? (0-8)");            destination = sc.nextInt();            if (destination == -1)            {                System.exit(0);            }        }        while (table[destination] != ' ')        {            System.out.println("Érvénytelen lépés!");            System.out.println("Melyik mezővel lépsz? (0-8)");            start = sc.nextInt();            System.out.println("Melyik mezőre lépsz? (0-8)");            destination = sc.nextInt();        }        int [] movement = new int[2];        movement[0] = start;        movement[1] = destination;        return movement;    }    public static void makeMove(char [] table, int [] move)    {        table[move[1]] = table[move[0]];        table[move[0]] = ' ';     }    public static int checkWin(char [] table)    {        if (table[0] == 'o' && table[1] == 'o' && table[2] == 'o' &&                table[3] == 'o' && table[4] == ' ' && table[5] == 'x' &&                table[6] == 'x' && table[7] == 'x' && table[8] == 'x')        {        return 1;        }        return 0;    }    public static void displayWinner(int result)    {        if (result == 1)        {            System.out.println("Gratulálok, vége a játéknak!");        }    }}

Now I only need to set the movement rules. I think it will be an another method with boolean.

Question 2

Welcome to Code Review! Code Review is a platform where you can get feedback onworking code to make it better. As you say, your code does not work as intended. Please read thehow to ask section in the Help Center.

Question 3

I changed the table from String to char because strings comparison is not the == operator.

Question 4

As far as I can understand your description, each piece has always zero or one possible move, so you needn't ask both questions. However, if you ask the first question only (which piece to move, but not where it has to land), you'd have to devise the appropriate move yourself.

Question 5

The code is incomplete and doesn't look like running and working correctly – themakeMove routine can only place "x" and not "o" into the table, anyway it's never invoked. I'm deleting my answer and voting to migrate the question to StackOverflow.

Question 6

The code seems improved now with all substantial errors fixed. IMHO the question deserves to be reopened.

Question 7

~~Themove variable seems unnecessary - you pass it togetValidMove() but never use it inside the function.~~
The language has loops for repetitive tasks. In thedrawPlayfield() you may do
```
for(int i=0; i<9; i++)    System.out.println("|"+table[i]);System.out.println("|");
```
instead of
```
System.out.println("|"+table[0]+"|"+table[1]+"|"+table[2]+        "|"+table[3]+"|"+table[4]+"|"+table[5]+"|"+table[6]+"|"+table[7]+"|"+table[8]+"|");
```
This costs 10 calls instead of two, but 1) saves strings addition, 2) makes the code easy expandable (you just need to replace the9 with another expression).
~~ThegetValidMove() function returns just oneint value. Why do you allocate a table for it? declare the return type asint instead ofint [] and save unnecessary work.~~
There is some problem with reponsibilities of functions. FunctiongetValidMove() not only gets a valid move, as its name says, but it also removes the chosen piece from the table with assignmenttable[start] = " "; That is a part of performing the actual move. However I can't see where the move is completed. Where do you dotable[destination] = "x"; ortable[destination] = "o";? And where do you store the shape of the piece removed bytable[start] = " "; to be reinserted atdestination?

Critical problems are fixed now - but one important remains:

The part to input user's move is unsafe. The first loop tests for both numbers being in the required range and repeats asking for input until data are correct – or until-1 appears, which terminates the program.

Then, however, another loop starts, which tests if the chosen destination position is free – and this loop does no longer validate values for being between 1 and 8. It also does not validate data for conforming the game rules (i.e. whether the jump is no more than 2 positions long).

IMHO there should be one loop, looking more or less like this:

do{    do    {        System.out.println("Input start position (0-8)");        start = sc.nextInt();        if (start == -1)        {            System.exit(0);        }    } while (start < 0 || start > 8);    do    {        System.out.println("Input destination position (0-8)");        destination = sc.nextInt();        if (destination == -1)        {            System.exit(0);        }    } while (destination < 0 || destination > 8);    if (destination == start)    {        System.out.println("Destination must differ from start!");        continue;    }    if (destination < start - 2 || destination > start + 2)    {        System.out.println("Destination must not differ from start more than by 2!");        continue;    }    if (table[destination] != ' ')    {        System.out.println("The destination position is not empty!");        continue;    }} while (false);   // the input is OK now

Of course the two inner loops are asking for replacing them with a call to some helper function...

Question 8

The drawPlayfield() method is a requirement in the task thats why I used it and not making a for loop for the task.

Question 9

@Falcon83 AFAIK I didnot suggest you shouldn't usedrawPlayfield(). I just proposed to write it in another way, which IMO is a bit more readable and more flexible.

CiaPan 1,9281 gold badge12 silver badges17 bronze badges · Answer 1 · 2019-03-27 14:51:33Z

~~Themove variable seems unnecessary - you pass it togetValidMove() but never use it inside the function.~~
The language has loops for repetitive tasks. In thedrawPlayfield() you may do
```
for(int i=0; i<9; i++)    System.out.println("|"+table[i]);System.out.println("|");
```
instead of
```
System.out.println("|"+table[0]+"|"+table[1]+"|"+table[2]+        "|"+table[3]+"|"+table[4]+"|"+table[5]+"|"+table[6]+"|"+table[7]+"|"+table[8]+"|");
```
This costs 10 calls instead of two, but 1) saves strings addition, 2) makes the code easy expandable (you just need to replace the9 with another expression).
~~ThegetValidMove() function returns just oneint value. Why do you allocate a table for it? declare the return type asint instead ofint [] and save unnecessary work.~~
There is some problem with reponsibilities of functions. FunctiongetValidMove() not only gets a valid move, as its name says, but it also removes the chosen piece from the table with assignmenttable[start] = " "; That is a part of performing the actual move. However I can't see where the move is completed. Where do you dotable[destination] = "x"; ortable[destination] = "o";? And where do you store the shape of the piece removed bytable[start] = " "; to be reinserted atdestination?

Critical problems are fixed now - but one important remains:

The part to input user's move is unsafe. The first loop tests for both numbers being in the required range and repeats asking for input until data are correct – or until-1 appears, which terminates the program.

Then, however, another loop starts, which tests if the chosen destination position is free – and this loop does no longer validate values for being between 1 and 8. It also does not validate data for conforming the game rules (i.e. whether the jump is no more than 2 positions long).

IMHO there should be one loop, looking more or less like this:

do{    do    {        System.out.println("Input start position (0-8)");        start = sc.nextInt();        if (start == -1)        {            System.exit(0);        }    } while (start < 0 || start > 8);    do    {        System.out.println("Input destination position (0-8)");        destination = sc.nextInt();        if (destination == -1)        {            System.exit(0);        }    } while (destination < 0 || destination > 8);    if (destination == start)    {        System.out.println("Destination must differ from start!");        continue;    }    if (destination < start - 2 || destination > start + 2)    {        System.out.println("Destination must not differ from start more than by 2!");        continue;    }    if (table[destination] != ' ')    {        System.out.println("The destination position is not empty!");        continue;    }} while (false);   // the input is OK now

Of course the two inner loops are asking for replacing them with a call to some helper function...

The drawPlayfield() method is a requirement in the task thats why I used it and not making a for loop for the task.
@Falcon83 AFAIK I didnot suggest you shouldn't usedrawPlayfield(). I just proposed to write it in another way, which IMO is a bit more readable and more flexible.

Movatterモバイル変換

Stack Exchange Network

Positional change program in Java

1 Answer1

You mustlog in to answer this question.

Related

Hot Network Questions

Subscribe to RSS