Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have written a script that allows a user to walk around a dungeon pick up gold and once they have picked it all up they can exit the game through an exit.

What I am doing now is writing a bot to do the same. So far I have come up with a 'stupid' bot that is based mainly on randomness. The bot has has actions to the following functions;

look() - Can see around the ASCII map in a radius of 2. The function prints it out so the bot cant actually store it in memory(I am allowed to store it memory).

move <direction>() - move in the direction these include N E S W.

pickUp() - If the bot is standing on gold this picks it up.

exitGame() - If the bot is standing on an exit, has all the gold and this command is ran he will exit the game.

Here is my bot code so far.

import java.io.IOException;
import java.util.Random;


public class Bot{
    public GameLogic bot;

    public static void main(String[] args) throws IOException{
        // TODO Auto-generated method stub
        GameLogic bot = new GameLogic("map2.txt"); 
        bot.startGame();
        int gold = bot.gold();
        System.out.println(gold);
        int turn = 0;
        char[][] myWorld = bot.getMap();
        while(true){
            int posX = bot.getPosX();
            int posY = bot.getPosY();
            Random rn = new Random();
            int min = 0;
            int max = 3;
            int n = max - min + 1;
            int i = rn.nextInt(n);
            if(myWorld[posX][posY] == 'G'){
                bot.pickUp();
            }
            if(myWorld[posX][posY] == 'E'){
                boolean y = bot.exitGame();
                if(y){break;}
            }
            turn++;
            if(i == 0){
                bot.moveN();
            }else if(i == 1){
                bot.moveE();
            }else if(i == 2){
                bot.moveS();
            }else if(i == 3){
                bot.moveW();

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

        }
        System.out.println(turn);

    }
}

What I am asking is what would be a better way to do this. How can I improve my bot so it takes less turns to complete? How can I make my bot 'smarter'?

share|improve this question

2 Answers 2

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.

share|improve this answer

+1 to @Simon and some other notes:

  1. The current design does not enforce that bots are playing fair game. A cheating bot could iterate over the whole map myWorld, find the exit point and goes there straight. The same is true for gold. You should create the mentioned look method which returns only the partial map in 2 blocks away and hide the whole map from the bot.

  2. I'd try to create a design where a bot just returns its next action (nextAction()) and the game evaluate actions in a loop and call nextAction again until the bot reaches the exit point. It would help when a dumb bot never finds the exit point. The evaluation logic could kill the bot, no need to duplicate this logic in every bot. The current code contains a similar thing: counting turns. It could be the game's responsibility to count how many turns have been used to find the exit (and how much gold have been picked up).

    It would also remove logic from bots, like this:

    final boolean y = game.exitGame();
    if (y) {
        break;
    }
    

    It shouldn't be the bot's responsibility to decide if it really reached the exit point or not.

  3. The main method contain too much logic, try to separate it. If you want to test two or more different bots what would be duplicated? If you eliminate that duplication you will have a better design.

  4. public GameLogic bot;
    

    This field is not used. Remove it if it is not necessary. (Public fields usually leads to harder maintenance. See: What's the deal with Java's public fields?)

  5. The variable for GameLogic is called bot. It's a little bit misleading.

  6. I would use more descriptive variable names and wouldn't abbreviate. Longer names would make the code more readable since readers/maintainers don't have to decode or memorize the abbreviations.

    • moveW - moveWest
    • moveN - moveNorth
    • moveE - moveEast
    • moveS - moveSouth
    • rn - random
    • i - nextDirection
    • min - minDirection
    • max - maxDirection
    • y - exitPointReached

    (I agree with @Simon that an enum is better here.)

share|improve this answer
1  
+1 for agreeing with me :) –  Simon André Forsberg Feb 23 at 13:47

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.