patternpythonMinor
Maze BFS in Python
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
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+=1Solution
In terms of readability:
On
With these modifications:
And on the
As modified:
If you implemented __eq__
- 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
booltype, sowhile 1:is generally writtenwhile True:.
- You mostly follow the style guide, but a bit more whitespace (e.g. around methods, assignments) would be helpful.
On
Point itself:directionsis a constant, so should beUPPERCASE_WITH_UNDERSCORESto indicate as much:DIRECTIONS.
- It seems odd to take an
otherPointinstance to__init__. This is particularly awkward when you havep = [Point()]; p[0].x, p[0].y = start_x, start_y- contrast with[Point(start_x, start_y)]. Move the functionality for creating aPointfrom an existing instance into a@classmethod, so you would callx = Point.from_point(self, char).
road, the path of directions taken to get to the current point, could be alistinstead of astr.
- Rather than
found_it, refactor tois_at(self, x, y), so you don't have to havestop_xandstop_yin scope (see above) andif p[begin].found_it():becomesif p[begin].is_at(stop_x, stop_y):, which is a little more explicit.
- Similarly,
hitandnew_pointshould take themaze_mapas a parameter, rather than relying on scope.
hitis only called within the class, so should probably be private-by-convention (i.e. named_hitinstead).
Point.directionsis also accessible viaself.directions, which will handle inheritance better.
new_pointyields more than onePoint, 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 == yAnd on the
checkio function:- You can neaten
while begin
- The names are pretty bad - p
should bepoints,beginshould becurrent(it's onlybeginat the beginning!).
- The while 1:
loop would be much neater if you just iterated overnew_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 += 1If 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 == ydef 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 += 1Context
StackExchange Code Review Q#92279, answer score: 6
Revisions (0)
No revisions yet.