patternpythonMinor
Sliding tiles solver in Python
Viewed 0 times
tilesslidingpythonsolver
Problem
I've implemented the sliding blocks puzzle in Python to solve it using different algorithms.
I'd like to know if the class "Sliding_blocks" is nicely designed or if I am missing
concepts of OOP.
I think that the indices of the blocks are a bit obfuscated, because I mix tuples,
lists and arrays (arrays are better to add but then I have to convert them to tuples
at some point).
I use the A* algorithm with the number of misplaced blocks as heuristic function.
It's quite slow and I think's it's because I have to create many copies of objects.
I'm more concerned with best practices than speed.
The code is:
```
#!/usr/bin/env python
# -- coding: utf-8 --
from __future__ import division
import numpy as np
from copy import copy, deepcopy
class Sliding_blocks():
def __init__(self):
self.size = 4
self.block = self.generate_block()
def generate_block(self):
"""Goal state"""
block = np.arange(1,self.size**2)
block.resize(self.size,self.size)
return block
def move(self, piece):
"""Moves the piece with index "piece" to free place, if possible"""
if list(piece) in self.find_moves():
self.block[tuple( self.find_free() )] = self.block[tuple(piece)]
self.block[tuple(piece)] = 0
return "success"
else:
return "error"
def find_free(self):
"""Returns array of indices of the free cell"""
free_position = np.where(self.block == 0)
free_position = np.array(free_position).flatten()
return free_position
def find_moves(self):
"""Returns list of allowed indices to move"""
from itertools import product
free_position = self.find_free()
return [list(free_position+i) for i in [[0,1],[1,0],[-1,0],[0,-1]] if tuple(i+free_position) in product(range(self.size),repeat=2)]
def shuffle(self):
steps = 30
for i in xrange(steps):
self.rand_move()
def r
I'd like to know if the class "Sliding_blocks" is nicely designed or if I am missing
concepts of OOP.
I think that the indices of the blocks are a bit obfuscated, because I mix tuples,
lists and arrays (arrays are better to add but then I have to convert them to tuples
at some point).
I use the A* algorithm with the number of misplaced blocks as heuristic function.
It's quite slow and I think's it's because I have to create many copies of objects.
I'm more concerned with best practices than speed.
The code is:
```
#!/usr/bin/env python
# -- coding: utf-8 --
from __future__ import division
import numpy as np
from copy import copy, deepcopy
class Sliding_blocks():
def __init__(self):
self.size = 4
self.block = self.generate_block()
def generate_block(self):
"""Goal state"""
block = np.arange(1,self.size**2)
block.resize(self.size,self.size)
return block
def move(self, piece):
"""Moves the piece with index "piece" to free place, if possible"""
if list(piece) in self.find_moves():
self.block[tuple( self.find_free() )] = self.block[tuple(piece)]
self.block[tuple(piece)] = 0
return "success"
else:
return "error"
def find_free(self):
"""Returns array of indices of the free cell"""
free_position = np.where(self.block == 0)
free_position = np.array(free_position).flatten()
return free_position
def find_moves(self):
"""Returns list of allowed indices to move"""
from itertools import product
free_position = self.find_free()
return [list(free_position+i) for i in [[0,1],[1,0],[-1,0],[0,-1]] if tuple(i+free_position) in product(range(self.size),repeat=2)]
def shuffle(self):
steps = 30
for i in xrange(steps):
self.rand_move()
def r
Solution
Whether the code is well designed or not -- is hard to tell without using it. But I have several points:
-
It's good that you have small functions/methods which do one thing and do well. It's good that you wrote docstrings and comments.
-
You should rethink this.
-
It's good that you have small functions/methods which do one thing and do well. It's good that you wrote docstrings and comments.
-
shortest=[i for i,l in enumerate(lengths) if l
-
Comply with PEP-8. def cost(path): return len(path) -> have two different lines. np.arange(1,self.size**2) use one space after commas. Game = Sliding_blocks() -- only classes should named CamelCased -> game = SlidingBlocks(). Use pylint to see all warnings.
-
Use enumerate:
for i in range(len(possible)):
actions.append(deepcopy(obj))
actions[i].move(possible[i])
Will become:
for i, _possible in enumerate(possible):
action = deepcopy(obj)
actions.append(action)
action.move(_possible)
-
Move the imports to the top of the file
def rand_move(self):
from random import choice
-
Import module instead of objects from it.
from itertools import product
...
from random import choice
Will become:
import copy
import itertools
import random
-
Using constants? Name them uppercase and make global. Is it a setting? Pass it as an argument with a default value or store as a class attribute.
def shuffle(self):
steps = 30
for i in xrange(steps):
self.rand_move()
-
Fail early.
if list(piece) in self.find_moves():
self.block[tuple( self.find_free() )] = self.block[tuple(piece)]
self.block[tuple(piece)] = 0
return "success"
else:
return "error"
This can be rewritten as
if list(piece) not in self.find_moves():
return "error"
...
return "success"
This will also reduce indentation.
return "error" -- this is a hardcoded string. Use a constant.
-
Indeed, using tuples and list`s is hard to read and is slowif list(piece) in self.find_moves():
self.block[tuple( self.find_free() )] = self.block[tuple(piece)]
self.block[tuple(piece)] = 0You should rethink this.
Code Snippets
for i in range(len(possible)):
actions.append(deepcopy(obj))
actions[i].move(possible[i])for i, _possible in enumerate(possible):
action = deepcopy(obj)
actions.append(action)
action.move(_possible)def rand_move(self):
from random import choicefrom itertools import product
...
from random import choiceimport copy
import itertools
import randomContext
StackExchange Code Review Q#125881, answer score: 4
Revisions (0)
No revisions yet.