0
\$\begingroup\$

I wold like some feedback on this mazefactory, it creatse a maze in a 2D array starting in the middle (the treasure room) and creating new rooms until it reaches the outside of the maze. The maze is used in a textbased game. The code works, I'm interested in learning.

public class MazeFactory {

    private Room[][] rooms;
    private int size;
    private Room outside;

    public MazeFactory(int size) {          
        this.size = size;
        rooms = new Room[size][size];           
    }

    public Maze LabyrinthBuilder() {            
        Room tresureRoom = createRoom(size/2, size/2, null, null);
        tresureRoom.setTresureRoom();
        System.out.println("TresureRoom: "+ tresureRoom.getX()+" "+tresureRoom.getY());         
        return new Maze(rooms, outside);
    }

    private Room createRoom(int x, int y, Room current, Wall wall) {
        if(x >= size || x < 0 || y >= size || y < 0) {
            return createOutside(current, wall);
        }           
        Room theRoom;           
        if(rooms[x][y] == null) { // If it is not a room there              
            theRoom = new Room(x, y);
            rooms[x][y] = theRoom;              
            // If there is not a room next to theRoom create a new wall, else use the wall from next room               
            if(y - 1 < 0 || rooms[x][y - 1] == null) {
                theRoom.setNorthWall(new Wall());
            } else {
                theRoom.setNorthWall(rooms[x][y - 1].getSouthWall());
            }
            if(y + 1 >= size || rooms[x][y + 1] == null) {
                theRoom.setSouthWall(new Wall());                   
            } else {
                theRoom.setSouthWall(rooms[x][y + 1].getNorthWall());
            }
            if(x - 1 < 0 || rooms[x - 1][y] == null) {
                theRoom.setEastWall(new Wall());
            } else {
                theRoom.setEastWall(rooms[x - 1][y].getWestWall());
            }
            if(x + 1 >= size || rooms[x + 1][y] == null) {
                theRoom.setWestWall(new Wall());
            } else {
                theRoom.setWestWall(rooms[x + 1][y].getEastWall());
            }               
        } else {
            theRoom = rooms[x][y];
        }
        addDoor(theRoom, x, y);
        return theRoom;
    }

    private void addDoor(Room current, int x, int y) {
        Wall wall;
        int rand = randomNumber();
        switch(rand) {
        case 0:
            wall = current.getNorthWall();
            y--;
            break;
        case 1:
            wall = current.getEastWall();
            x--;
            break;
        case 2:
            wall = current.getSouthWall();
            y++;
            break;
        case 3:
            wall = current.getWestWall();
            x++;
            break;
        default:
            wall = current.getNorthWall();
            y--;
        }
        if(wall.hasDoor()) {
            Room otherRoom = wall.getDoor().goThrough(current);
            createRoom(otherRoom.getX(), otherRoom.getY(), current, wall);
        } else {    
            wall.addDoor(new Door(current, createRoom(x, y, current, wall)));
        }           
    }       

    private Room createOutside(Room current, Wall currentWall) {            
        Room temp =  new Room(-1, -1);          
        if(current.getNorthWall() == currentWall) {
            temp.setSouthWall(currentWall);             
            temp.setNorthWall(new Wall());
            temp.setEastWall(new Wall());
            temp.setWestWall(new Wall());       
        }
        else if(current.getEastWall() == currentWall) {
            temp.setWestWall(currentWall);              
            temp.setNorthWall(new Wall());
            temp.setEastWall(new Wall());
            temp.setSouthWall(new Wall());
        }
        else if(current.getSouthWall() == currentWall) {                
            temp.setNorthWall(currentWall);             
            temp.setEastWall(new Wall());
            temp.setSouthWall(new Wall());
            temp.setWestWall(new Wall());
        }
        else if(current.getWestWall() == currentWall) {
            temp.setEastWall(currentWall);              
            temp.setNorthWall(new Wall());
            temp.setSouthWall(new Wall());
            temp.setWestWall(new Wall());           
        }
        temp.setOutside(true);
        outside = temp;
        return temp;            
    }       
    private int randomNumber() {
        return (int)(Math.random() * 10) / 3;
    }
}
\$\endgroup\$
1
\$\begingroup\$

one minor readability issue would be to init the temp room with all walls and then later override the adjecting wall:

private Room createOutside(Room current, Wall currentWall) {            
    Room temp =  new Room(-1, -1); 
    //code from Room.createWalls():
    //temp.setSouthWall(new Wall());  
    //temp.setNorthWall(new Wall());
    //temp.setEastWall(new Wall());
    //temp.setWestWall(new Wall());        
    temp.createWalls();
    if(current.getNorthWall() == currentWall) {
        temp.setSouthWall(currentWall);             
    }
    else if(current.getEastWall() == currentWall) {
        temp.setWestWall(currentWall);              
    }
    else if(current.getSouthWall() == currentWall) {                
        temp.setNorthWall(currentWall);             
    }
    else if(current.getWestWall() == currentWall) {
        temp.setEastWall(currentWall);              
    }
    temp.setOutside(true);
    outside = temp;
    return temp;            
} 

the same applies the method private Room createRoom(...):

private Room createRoom(int x, int y, Room current, Wall wall) {
    if(x >= size || x < 0 || y >= size || y < 0) {
        return createOutside(current, wall);
    }
    Room theRoom;
    if(rooms[x][y] == null) { 
        rooms[x][y]= new Room(x, y) //FIXME skip local variable 'theRoom' - it's use is inkonsequent
        rooms[x][y].createWalls(); //see above, creates all four walls
        if(y - 1 < 0 || rooms[x][y - 1] == null) { 
            //rooms[x][y].setNorthWall(new Wall()); - can be skipped now
        }
        else {
            rooms[x][y].setNorthWall(rooms[x][y - 1].getSouthWall());
        }
        if(y + 1 >= size || rooms[x][y + 1] == null) {
            // rooms[x][y].setSouthWall(new Wall()); - can be skipped *again*
        } else {
            ...
        }
        ...
}

another minor issue would be to use directions instead of ints...

private final enum Direction {NORTH, EAST, SOUTH, WEST};

private Direction getRandomDirection(){
    //untested code, it's straight from mind into CodeReview without compile check
    return new ArrayList<Direction>(Directions.values()).shuffle().get(0);
}

private void addDoor(Room current, int x, int y) {
    Wall wall;
    Direction dir = getRandomDirection();
    switch(dir) {
    case NORTH: 
        wall = current.getNorthWall();
        y--;
        break;
    case EAST: //and so on...
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Since you always do this right after creating a new Room you might as well suggest to put it into the constructor of the Room. So a new Room will always be surrounded with 4 walls until one of them is replaced with a new connection. \$\endgroup\$ – Imus Feb 12 '18 at 14:46
  • \$\begingroup\$ i'm not sure if room is ever used in any other code pieces, but providing such a constructor would be indeed useful! @Imus \$\endgroup\$ – Martin Frank Feb 12 '18 at 14:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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