Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Overall

Overall

I think you have too much code in the main method. You should split that into more digestible segments. Perhaps some more methods in your GameLogic class (which probably should be split into one Game class and one GameBot / Player class). (At least) one method for determining the next move, one method for choosing the proper action based on the current tile, etc..

###Improving your bot

Improving your bot

Assuming the gold never moves once it is placed, you could use a Set of all the tiles you have visited. You could then try to not move to a tile you have already visited.

The tiles themselves could be represented as a Point class that contains x and y, or it could be represented as an int if you do a little mathematics to represent a column and row by one single int (I do recommend the Point class though). Or you could make a Tile class, since each tile also has some content (wall, exit, gold, empty space...)

###Random

Random

Random rn = new Random();
int min = 0;
int max = 3;
int n = max - min + 1;
int i = rn.nextInt(n);
  • Since min is zero, there's no need to do - min.
  • As max is a constant, it'd be easier to declare max as 4 from the start rather than using + 1
  • As n only is used once, you could just use int i = rn.nextInt(4);
  • Random objects are meant to be re-used. Initialize your Random object outside the loop. This is because randomization will be different when creating the object over and over again.

###Moving

Moving

You can move in four directions, this is telling me that you should use a Direction enum and replace your four methods with one move(Direction4 dir); By using this enum, you could also simplify your random process:

Direction4 chosenDirection = Direction4.values()[rn.nextInt(Direction4.values().length)];

This line grabs all the possible directions and randomizes an int from 0 (inclusive) to length (exclusive) and grabs the direction with that index.

###When something goes wrong

When something goes wrong

System.out.println("SOMETHING WENT WRONG!!!");

I can assure you, this will never happen. And even if it would, Exceptions are meant to be for... well, exceptions. throw new AssertionError("Something went wrong."); is what I would have done here. Or rather... I would have removed the entire line, as it will never happen. Especially when you use the Direction4 approach.

###Spacing

Spacing

This is one of the things I mention most often in my reviews probably, but please:

}else if(i == 1){

change to

} else if (i == 1) {

It makes the code more readable with proper spacing.

###Overall

I think you have too much code in the main method. You should split that into more digestible segments. Perhaps some more methods in your GameLogic class (which probably should be split into one Game class and one GameBot / Player class). (At least) one method for determining the next move, one method for choosing the proper action based on the current tile, etc..

###Improving your bot

Assuming the gold never moves once it is placed, you could use a Set of all the tiles you have visited. You could then try to not move to a tile you have already visited.

The tiles themselves could be represented as a Point class that contains x and y, or it could be represented as an int if you do a little mathematics to represent a column and row by one single int (I do recommend the Point class though). Or you could make a Tile class, since each tile also has some content (wall, exit, gold, empty space...)

###Random

Random rn = new Random();
int min = 0;
int max = 3;
int n = max - min + 1;
int i = rn.nextInt(n);
  • Since min is zero, there's no need to do - min.
  • As max is a constant, it'd be easier to declare max as 4 from the start rather than using + 1
  • As n only is used once, you could just use int i = rn.nextInt(4);
  • Random objects are meant to be re-used. Initialize your Random object outside the loop. This is because randomization will be different when creating the object over and over again.

###Moving

You can move in four directions, this is telling me that you should use a Direction enum and replace your four methods with one move(Direction4 dir); By using this enum, you could also simplify your random process:

Direction4 chosenDirection = Direction4.values()[rn.nextInt(Direction4.values().length)];

This line grabs all the possible directions and randomizes an int from 0 (inclusive) to length (exclusive) and grabs the direction with that index.

###When something goes wrong

System.out.println("SOMETHING WENT WRONG!!!");

I can assure you, this will never happen. And even if it would, Exceptions are meant to be for... well, exceptions. throw new AssertionError("Something went wrong."); is what I would have done here. Or rather... I would have removed the entire line, as it will never happen. Especially when you use the Direction4 approach.

###Spacing

This is one of the things I mention most often in my reviews probably, but please:

}else if(i == 1){

change to

} else if (i == 1) {

It makes the code more readable with proper spacing.

Overall

I think you have too much code in the main method. You should split that into more digestible segments. Perhaps some more methods in your GameLogic class (which probably should be split into one Game class and one GameBot / Player class). (At least) one method for determining the next move, one method for choosing the proper action based on the current tile, etc..

Improving your bot

Assuming the gold never moves once it is placed, you could use a Set of all the tiles you have visited. You could then try to not move to a tile you have already visited.

The tiles themselves could be represented as a Point class that contains x and y, or it could be represented as an int if you do a little mathematics to represent a column and row by one single int (I do recommend the Point class though). Or you could make a Tile class, since each tile also has some content (wall, exit, gold, empty space...)

Random

Random rn = new Random();
int min = 0;
int max = 3;
int n = max - min + 1;
int i = rn.nextInt(n);
  • Since min is zero, there's no need to do - min.
  • As max is a constant, it'd be easier to declare max as 4 from the start rather than using + 1
  • As n only is used once, you could just use int i = rn.nextInt(4);
  • Random objects are meant to be re-used. Initialize your Random object outside the loop. This is because randomization will be different when creating the object over and over again.

Moving

You can move in four directions, this is telling me that you should use a Direction enum and replace your four methods with one move(Direction4 dir); By using this enum, you could also simplify your random process:

Direction4 chosenDirection = Direction4.values()[rn.nextInt(Direction4.values().length)];

This line grabs all the possible directions and randomizes an int from 0 (inclusive) to length (exclusive) and grabs the direction with that index.

When something goes wrong

System.out.println("SOMETHING WENT WRONG!!!");

I can assure you, this will never happen. And even if it would, Exceptions are meant to be for... well, exceptions. throw new AssertionError("Something went wrong."); is what I would have done here. Or rather... I would have removed the entire line, as it will never happen. Especially when you use the Direction4 approach.

Spacing

This is one of the things I mention most often in my reviews probably, but please:

}else if(i == 1){

change to

} else if (i == 1) {

It makes the code more readable with proper spacing.

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

###Overall

I think you have too much code in the main method. You should split that into more digestible segments. Perhaps some more methods in your GameLogic class (which probably should be split into one Game class and one GameBot / Player class). (At least) one method for determining the next move, one method for choosing the proper action based on the current tile, etc..

###Improving your bot

Assuming the gold never moves once it is placed, you could use a Set of all the tiles you have visited. You could then try to not move to a tile you have already visited.

The tiles themselves could be represented as a Point class that contains x and y, or it could be represented as an int if you do a little mathematics to represent a column and row by one single int (I do recommend the Point class though). Or you could make a Tile class, since each tile also has some content (wall, exit, gold, empty space...)

###Random

Random rn = new Random();
int min = 0;
int max = 3;
int n = max - min + 1;
int i = rn.nextInt(n);
  • Since min is zero, there's no need to do - min.
  • As max is a constant, it'd be easier to declare max as 4 from the start rather than using + 1
  • As n only is used once, you could just use int i = rn.nextInt(4);
  • Random objects are meant to be re-used. Initialize your Random object outside the loop. This is because randomization will be different when creating the object over and over again.

###Moving

You can move in four directions, this is telling me that you should use a Direction enum Direction enum and replace your four methods with one move(Direction4 dir); By using this enum, you could also simplify your random process:

Direction4 chosenDirection = Direction4.values()[rn.nextInt(Direction4.values().length)];

This line grabs all the possible directions and randomizes an int from 0 (inclusive) to length (exclusive) and grabs the direction with that index.

###When something goes wrong

System.out.println("SOMETHING WENT WRONG!!!");

I can assure you, this will never happen. And even if it would, Exceptions are meant to be for... well, exceptions. throw new AssertionError("Something went wrong."); is what I would have done here. Or rather... I would have removed the entire line, as it will never happen. Especially when you use the Direction4 approach.

###Spacing

This is one of the things I mention most often in my reviews probably, but please:

}else if(i == 1){

change to

} else if (i == 1) {

It makes the code more readable with proper spacing.

###Overall

I think you have too much code in the main method. You should split that into more digestible segments. Perhaps some more methods in your GameLogic class (which probably should be split into one Game class and one GameBot / Player class). (At least) one method for determining the next move, one method for choosing the proper action based on the current tile, etc..

###Improving your bot

Assuming the gold never moves once it is placed, you could use a Set of all the tiles you have visited. You could then try to not move to a tile you have already visited.

The tiles themselves could be represented as a Point class that contains x and y, or it could be represented as an int if you do a little mathematics to represent a column and row by one single int (I do recommend the Point class though). Or you could make a Tile class, since each tile also has some content (wall, exit, gold, empty space...)

###Random

Random rn = new Random();
int min = 0;
int max = 3;
int n = max - min + 1;
int i = rn.nextInt(n);
  • Since min is zero, there's no need to do - min.
  • As max is a constant, it'd be easier to declare max as 4 from the start rather than using + 1
  • As n only is used once, you could just use int i = rn.nextInt(4);
  • Random objects are meant to be re-used. Initialize your Random object outside the loop. This is because randomization will be different when creating the object over and over again.

###Moving

You can move in four directions, this is telling me that you should use a Direction enum and replace your four methods with one move(Direction4 dir); By using this enum, you could also simplify your random process:

Direction4 chosenDirection = Direction4.values()[rn.nextInt(Direction4.values().length)];

This line grabs all the possible directions and randomizes an int from 0 (inclusive) to length (exclusive) and grabs the direction with that index.

###When something goes wrong

System.out.println("SOMETHING WENT WRONG!!!");

I can assure you, this will never happen. And even if it would, Exceptions are meant to be for... well, exceptions. throw new AssertionError("Something went wrong."); is what I would have done here. Or rather... I would have removed the entire line, as it will never happen. Especially when you use the Direction4 approach.

###Spacing

This is one of the things I mention most often in my reviews probably, but please:

}else if(i == 1){

change to

} else if (i == 1) {

It makes the code more readable with proper spacing.

###Overall

I think you have too much code in the main method. You should split that into more digestible segments. Perhaps some more methods in your GameLogic class (which probably should be split into one Game class and one GameBot / Player class). (At least) one method for determining the next move, one method for choosing the proper action based on the current tile, etc..

###Improving your bot

Assuming the gold never moves once it is placed, you could use a Set of all the tiles you have visited. You could then try to not move to a tile you have already visited.

The tiles themselves could be represented as a Point class that contains x and y, or it could be represented as an int if you do a little mathematics to represent a column and row by one single int (I do recommend the Point class though). Or you could make a Tile class, since each tile also has some content (wall, exit, gold, empty space...)

###Random

Random rn = new Random();
int min = 0;
int max = 3;
int n = max - min + 1;
int i = rn.nextInt(n);
  • Since min is zero, there's no need to do - min.
  • As max is a constant, it'd be easier to declare max as 4 from the start rather than using + 1
  • As n only is used once, you could just use int i = rn.nextInt(4);
  • Random objects are meant to be re-used. Initialize your Random object outside the loop. This is because randomization will be different when creating the object over and over again.

###Moving

You can move in four directions, this is telling me that you should use a Direction enum and replace your four methods with one move(Direction4 dir); By using this enum, you could also simplify your random process:

Direction4 chosenDirection = Direction4.values()[rn.nextInt(Direction4.values().length)];

This line grabs all the possible directions and randomizes an int from 0 (inclusive) to length (exclusive) and grabs the direction with that index.

###When something goes wrong

System.out.println("SOMETHING WENT WRONG!!!");

I can assure you, this will never happen. And even if it would, Exceptions are meant to be for... well, exceptions. throw new AssertionError("Something went wrong."); is what I would have done here. Or rather... I would have removed the entire line, as it will never happen. Especially when you use the Direction4 approach.

###Spacing

This is one of the things I mention most often in my reviews probably, but please:

}else if(i == 1){

change to

} else if (i == 1) {

It makes the code more readable with proper spacing.

added 608 characters in body
Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311

###Overall

I think you have too much code in the main method. You should split that into more digestible segments. Perhaps some more methods in your GameLogic class (which probably should be split into one Game class and one GameBot / Player class). (At least) one method for determining the next move, one method for choosing the proper action based on the current tile, etc..

###Improving your bot

Assuming the gold never moves once it is placed, you could use a Set of all the tiles you have visited. You could then try to not move to a tile you have already visited.

The tiles themselves could be represented as a Point class that contains x and y, or it could be represented as an int if you do a little mathematics to represent a column and row by one single int (I do recommend the Point class though). Or you could make a Tile class, since each tile also has some content (wall, exit, gold, empty space...)

###Random

Random rn = new Random();
int min = 0;
int max = 3;
int n = max - min + 1;
int i = rn.nextInt(n);
  • Since min is zero, there's no need to do - min.
  • As max is a constant, it'd be easier to declare max as 4 from the start rather than using + 1
  • As n only is used once, you could just use int i = rn.nextInt(4);
  • Random objects are meant to be re-used. Initialize your Random object outside the loop. This is because randomization will be different when creating the object over and over again.

###Moving

You can move in four directions, this is telling me that you should use a Direction enum and replace your four methods with one move(Direction4 dir); By using this enum, you could also simplify your random process:

Direction4 chosenDirection = Direction4.values()[rn.nextInt(Direction4.values().length)];

This line grabs all the possible directions and randomizes an int from 0 (inclusive) to length (exclusive) and grabs the direction with that index.

###When something goes wrong

System.out.println("SOMETHING WENT WRONG!!!");

I can assure you, this will never happen. And even if it would, Exceptions are meant to be for... well, exceptions. throw new AssertionError("Something went wrong."); is what I would have done here. Or rather... I would have removed the entire line, as it will never happen. Especially when you use the Direction4 approach.

###Spacing

This is one of the things I mention most often in my reviews probably, but please:

}else if(i == 1){

change to

} else if (i == 1) {

It makes the code more readable with proper spacing.

I think you have too much code in the main method. You should split that into more digestible segments. Perhaps some more methods in your GameLogic class (which probably should be split into one Game class and one GameBot / Player class). (At least) one method for determining the next move, one method for choosing the proper action based on the current tile, etc..

Assuming the gold never moves once it is placed, you could use a Set of all the tiles you have visited. You could then try to not move to a tile you have already visited.

The tiles themselves could be represented as a Point class that contains x and y, or it could be represented as an int if you do a little mathematics to represent a column and row by one single int (I do recommend the Point class though). Or you could make a Tile class, since each tile also has some content (wall, exit, gold, empty space...)

You can move in four directions, this is telling me that you should use a Direction enum and replace your four methods with one move(Direction4 dir); By using this enum, you could also simplify your random process:

Direction4 chosenDirection = Direction4.values()[rn.nextInt(Direction4.values().length)];

This line grabs all the possible directions and randomizes an int from 0 (inclusive) to length (exclusive) and grabs the direction with that index.

###When something goes wrong

System.out.println("SOMETHING WENT WRONG!!!");

I can assure you, this will never happen. And even if it would, Exceptions are meant to be for... well, exceptions. throw new AssertionError("Something went wrong."); is what I would have done here. Or rather... I would have removed the entire line, as it will never happen. Especially when you use the Direction4 approach.

###Spacing

This is one of the things I mention most often in my reviews probably, but please:

}else if(i == 1){

change to

} else if (i == 1) {

It makes the code more readable with proper spacing.

###Overall

I think you have too much code in the main method. You should split that into more digestible segments. Perhaps some more methods in your GameLogic class (which probably should be split into one Game class and one GameBot / Player class). (At least) one method for determining the next move, one method for choosing the proper action based on the current tile, etc..

###Improving your bot

Assuming the gold never moves once it is placed, you could use a Set of all the tiles you have visited. You could then try to not move to a tile you have already visited.

The tiles themselves could be represented as a Point class that contains x and y, or it could be represented as an int if you do a little mathematics to represent a column and row by one single int (I do recommend the Point class though). Or you could make a Tile class, since each tile also has some content (wall, exit, gold, empty space...)

###Random

Random rn = new Random();
int min = 0;
int max = 3;
int n = max - min + 1;
int i = rn.nextInt(n);
  • Since min is zero, there's no need to do - min.
  • As max is a constant, it'd be easier to declare max as 4 from the start rather than using + 1
  • As n only is used once, you could just use int i = rn.nextInt(4);
  • Random objects are meant to be re-used. Initialize your Random object outside the loop. This is because randomization will be different when creating the object over and over again.

###Moving

You can move in four directions, this is telling me that you should use a Direction enum and replace your four methods with one move(Direction4 dir); By using this enum, you could also simplify your random process:

Direction4 chosenDirection = Direction4.values()[rn.nextInt(Direction4.values().length)];

This line grabs all the possible directions and randomizes an int from 0 (inclusive) to length (exclusive) and grabs the direction with that index.

###When something goes wrong

System.out.println("SOMETHING WENT WRONG!!!");

I can assure you, this will never happen. And even if it would, Exceptions are meant to be for... well, exceptions. throw new AssertionError("Something went wrong."); is what I would have done here. Or rather... I would have removed the entire line, as it will never happen. Especially when you use the Direction4 approach.

###Spacing

This is one of the things I mention most often in my reviews probably, but please:

}else if(i == 1){

change to

} else if (i == 1) {

It makes the code more readable with proper spacing.

Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311
Loading
lang-java

AltStyle によって変換されたページ (->オリジナル) /