HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Maze BFS in Python

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mazebfspython

Problem

The program below was made for a game named CheckIO, and it works. I'm just wondering if there is any way to make the code more readable.

edit: hi luke15g

def checkio(maze_map):
    #the walls of the maze are already surrounded by 1's
    start_x,start_y=1,1
    stop_x,stop_y=10,10
    class Point:
        directions={'N':(-1,0),'W':(0,-1),'E':(0,1),'S':(1,0)}
        def __init__(self,other=None,direction=None):
            if other!=None:
                self.road=other.road+direction
                self.x=other.x+Point.directions[direction][0]
                self.y=other.y+Point.directions[direction][1]
            else:
                self.x,self.y=None,None
                self.road=''
        def hit(self):
            return maze_map[self.x][self.y]==1
        def new_point(self):
            for char in Point.directions:
                x=Point(self,char)
                if not x.hit():
                    yield x
        def found_it(self):
            return self.x==stop_x and self.y==stop_y
    p=[Point()]
    p[0].x,p[0].y=start_x,start_y
    begin=0
    maze_map[start_x][start_y]=1
    while begin<=len(p)-1:
        if p[begin].found_it():
            return p[begin].road
        x=p[begin].new_point()
        while 1:
            try:
                new=next(x)
            except StopIteration:
                break
            p.append(new)
            maze_map[new.x][new.y]=1
        begin+=1

Solution

In terms of readability:

  • There is no reason to nest the class inside the function (your current need to do so is caused by bad design, see below); doing so just distracts the reader from the actual business of the function.



  • Both function and class (and the class's methods) should have docstrings explaining what they do.



  • Python has a bool type, so while 1: is generally written while True:.



  • You mostly follow the style guide, but a bit more whitespace (e.g. around methods, assignments) would be helpful.



On Point itself:

  • directions is a constant, so should be UPPERCASE_WITH_UNDERSCORES to indicate as much: DIRECTIONS.



  • It seems odd to take an other Point instance to __init__. This is particularly awkward when you have p = [Point()]; p[0].x, p[0].y = start_x, start_y - contrast with [Point(start_x, start_y)]. Move the functionality for creating a Point from an existing instance into a @classmethod, so you would call x = Point.from_point(self, char).



  • road, the path of directions taken to get to the current point, could be a list instead of a str.



  • Rather than found_it, refactor to is_at(self, x, y), so you don't have to have stop_x and stop_y in scope (see above) and if p[begin].found_it(): becomes if p[begin].is_at(stop_x, stop_y):, which is a little more explicit.



  • Similarly, hit and new_point should take the maze_map as a parameter, rather than relying on scope.



  • hit is only called within the class, so should probably be private-by-convention (i.e. named _hit instead).



  • Point.directions is also accessible via self.directions, which will handle inheritance better.



  • new_point yields more than one Point, so should be plural (new_points).



With these modifications:

class Point:

    DIRECTIONS = {'N': (-1, 0), 'W': (0, -1), 'E': (0, 1), 'S': (1, 0)}

    def __init__(self, x, y, road=None):
        self.x, self.y = x, y
        self.road = road or []

    @classmethod
    def from_point(cls, other, direction):
        """Create a new point from an existing point and direction."""
        d_x, d_y = cls.DIRECTIONS[direction]
        return cls(
            other.x + d_x, 
            other.y + d_y, 
            other.road + [direction],
        )

    def _hit(self, map):
        """Would this point hit a wall in the map?"""
        return map[self.x][self.y] == 1

    def new_points(self, map):
        """Generate new points from the current point."""
        for char in self.directions:
            new_point = Point.from_point(self, char)
            if not new_point._hit(map):
                yield new_point

    def is_at(self, x, y):
        """Whether the point is at the specified location."""
        return self.x == x and self.y == y


And on the checkio function:

  • You can neaten while begin



  • The names are pretty bad - p should be points, begin should be current (it's only begin at the beginning!).



  • The while 1: loop would be much neater if you just iterated over new_points: for point in points[current].new_points():.



As modified:

def checkio(maze_map):
    """Solve the maze.

    The walls of the maze are already surrounded by 1s.

    """
    start_x, start_y = 1, 1
    stop_x, stop_y = 10, 10
    points = [Point(start_x, start_y)]
    current = 0
    maze_map[start_x][start_y] = 1
    while current < len(points):
        if points[current].is_at(stop_x, stop_y):
            return ''.join(points[current].road)
        for new_point in points[current].new_points(maze_map):
            points.append(new_point)
            maze_map[new_point.x][new_point.y] = 1
        current += 1


If you implemented
__eq__ and __hash__ on the Point class, you could keep a set` of the points you've already visited, rather than adding a "wall" into the map when you visit a particular location.

Code Snippets

class Point:

    DIRECTIONS = {'N': (-1, 0), 'W': (0, -1), 'E': (0, 1), 'S': (1, 0)}

    def __init__(self, x, y, road=None):
        self.x, self.y = x, y
        self.road = road or []

    @classmethod
    def from_point(cls, other, direction):
        """Create a new point from an existing point and direction."""
        d_x, d_y = cls.DIRECTIONS[direction]
        return cls(
            other.x + d_x, 
            other.y + d_y, 
            other.road + [direction],
        )

    def _hit(self, map):
        """Would this point hit a wall in the map?"""
        return map[self.x][self.y] == 1

    def new_points(self, map):
        """Generate new points from the current point."""
        for char in self.directions:
            new_point = Point.from_point(self, char)
            if not new_point._hit(map):
                yield new_point

    def is_at(self, x, y):
        """Whether the point is at the specified location."""
        return self.x == x and self.y == y
def checkio(maze_map):
    """Solve the maze.

    The walls of the maze are already surrounded by 1s.

    """
    start_x, start_y = 1, 1
    stop_x, stop_y = 10, 10
    points = [Point(start_x, start_y)]
    current = 0
    maze_map[start_x][start_y] = 1
    while current < len(points):
        if points[current].is_at(stop_x, stop_y):
            return ''.join(points[current].road)
        for new_point in points[current].new_points(maze_map):
            points.append(new_point)
            maze_map[new_point.x][new_point.y] = 1
        current += 1

Context

StackExchange Code Review Q#92279, answer score: 6

Revisions (0)

No revisions yet.