Skip to content

Instantly share code, notes, and snippets.

@vmeyet
Last active August 29, 2015 13:58
Show Gist options
  • Save vmeyet/288568bbcf0ad98b92bb to your computer and use it in GitHub Desktop.
Save vmeyet/288568bbcf0ad98b92bb to your computer and use it in GitHub Desktop.
Code Review
import sys
class NoSolutionError(Exception):
pass
class ProblemSolved(Exception):
pass
class Point(object):
def __init__(self, x, y):
self.x = x
self.y = y
def __add__(self, other):
""" define the addition + """
self.x += other.x
self.y += other.y
def __eq__(self, other):
""" define the equality operator == """
return self.x == other.x and self.y == other.y
class Map(object):
def __init__(self, maze):
self.cL = len(maze[0])
self.rL = len(maze)
self.maze = [[el == "." for el in line] for line in maze]
def get(self, point):
if point.x >= self.cL or point.x < 0 or point.y < 0 or point.y >= self.rL:
return False
return self.maze[point.y][point.x]
class Robot(object):
directions = ("N", "E", "S", "W")
dict_dir = {
i: j for i, j in zip(directions, (Point(0, -1), Point(1, 0), Point(0, 1), Point(-1, 0)))
}
def __init__(self, maze, pos, fin):
self.maze = maze
self.position = Point(pos[0], pos[1])
fin = fin[::-1] # pas compris
self.fin = Point(fin[0], fin[1])
self.direction = self.init_dir()
@property
def next_position(self):
return self.position + self.dict_dir[self.direction]
def init_dir(self):
""" Recherche de la premiere direction a prendre"""
if self.maze.get(self.position + Point(0, - 1)):
if self.maze.get(self.position + Point(- 1, 0)):
return 'W'
else:
return 'N'
else:
if self.maze.get(self.position + Point(- 1, 0)):
return 'S'
else:
return 'E'
def get_on_the_left(self):
left_dir = self.directions[(self.directions.index(self.direction) + 3) % 4]
left_pos = self.position + self.dict_dir[left_dir]
return left_pos, self.maze.get(left_pos), left_dir
def check_for_next_dir(self):
next_dir_ok = False
for i in xrange(4):
next_dir_ok = self.maze.get(self.next_position)
if next_dir_ok:
break
else:
self.direction = self.directions[(self.directions.index(self.direction) + 1) % 4]
if not next_dir_ok:
raise NoSolutionError()
def go(self):
self.check_for_next_dir()
self.position = self.next_position
yield self.direction
while True:
left_pos, dir_ok, tmpdir = self.get_on_the_left()
if dir_ok:
self.position, self.direction = left_pos, tmpdir
else:
self.check_for_next_dir()
self.position = self.next_position
if self.position == self.fin:
raise ProblemSolved()
yield self.direction
def solve(robot):
chemin = []
try:
for iteration, direction in enumerate(robot.go()):
if iteration >= 10000:
raise NoSolutionError()
chemin.append(direction)
return "".join(chemin), iteration
except NoSolutionError:
return "", "Edison ran out of energy."
except ProblemSolved:
return "".join(chemin), iteration
def get_robot(f):
nb_lines = int(f.readline())
maze = [f.readline().rstrip("\n") for _ in xrange(nb_lines)]
positions = [int(i) - 1 for i in f.readline().split(" ")]
return Robot(Map(maze), positions[:2], positions[2:])
if __name__ == "__main__":
pattern = "Case #{0}: {1}"
with open(sys.argv[1]) as f:
ntests = int(f.readline())
for i in xrange(1, ntests + 1):
robot = get_robot(f)
chemin, it = solve(robot)
print pattern.format(i, it)
if chemin:
print chemin
@vmeyet
Copy link
Author

vmeyet commented Apr 6, 2014

Bon voici les commentaires que j'ai à faire, il correspondent tous à une version/revision de ce Gist. (click sur revision pour voir l'historique). J'ai essayé d'être méga explicite, et te faisant chier pour des broutilles de temps en temps mais c'est comme ça qu'on progresse ;). En gros au début c'est pas mal de syntax/code formatting, et ensuite c'est plus en profondeur par rapport au code.

1. enlever les import non utilisé
  • utilise un linter pour te les montrer
  • dans ce cas l'import inutilisé est pdb (probablement pour du debug ?)
    un technique pour ne pas oublier de l'enlever est de toujours l'utiliser en one-liner:
    import pdb; pdb.set_trace()
2. utiliser le format pep8 + enlever les "trailing spaces"
  • pep8 format:
    • saut de deux lignes entre differentes classes
    • saut d'une seul ligne entre les methodes à l'inérieure d'une classe
    • ajout d'espace entre les opérateurs (==, +=, >, <, + etc):
      if x >= self.cL or x < 0 or y < 0 or y >= self.rL:
    • pas besoin de virgules à la fin des list/tuple (sauf pour les tuple d'un seul élément)
      • [1, 2, 3]
      • [4]
      • (1, 4, 65)
      • mais (1,)
    • pas de mélange d'espace et tab ;)
  • trailing spaces:
    • espace/tabs seuls sur une ligne ou à la fin d'une expression
    • utilise le plugin sublime: "Trailing spaces"
3. Aerer le code
  • ajout de ligne vide lorsque ça à du sens
4. Tu n'utilize pas la class Map (du coup on la vire ou on l'utilise):
  • on l'utilise:
    • dans l'objet Map, cL, rL, _map ne doive pas être class attribute mais instance attribute
    • on sépare la logic du robot et de la map
  • de plus "|" et equivalent à "#"
5. En python on utilise le snake_case non pas le camelCase pour les nom de variable
6. Les constantes globale de modules n'ont pas de raison de l'être
7. Use 0/1 ou True/False à la place de "#"/"|"/"." la comparaison est un peu plus efficace
8. Leve des exceptions, quand il n'y a plus de possibilités par ex.
9. Utilise des propriété pour la position courante et future
  • une idée pour la manipulation des coordonnée en Python est la creation d'une classe Point (ou Vector), cela facilitera l'addition de Point
    ex:
    class Point(object):
        def __init__(self, x, y):
            self.x = x
            self.y = y

        def __add__(self, other):
            self.x += other.x
            self.y += other.y

    p1 = Point(1, 2)
    p2 = Point(4, 25)

    p3 = p1 + p2  # Point(5, 27)
10. Utilise enumerate au lieu d'incrémenter le counter toi même dans sol. n'utilise pas +=pour concatener des string (utilise une list que tu join)
11. Un peu Hacky (ok beaucoup hacky), mais vu l'architecture, lève une exception quand c'est gagné, pour sortir du jeu.

De plus/Bonus.

  • j'ai pas compris pourquoi tu faisais: self.fin = fin[::-1]
  • pour gerer les arguments en ligne de commande du script, utilise la librarie argparse (elle te permet également de valider les inputs et d'afficher une aide).
  • Je pense qu'en modifiant un peu le code je pense qu'il y a moyen d'enlever le get_next/next_position, car en gros tu veux toujours aller à gauche donc tu essaye d'aller à gauche, si ça marche pas tu vas encore plus à gauche etc. avec un conteur et si tu est allé 4 fois à gauche ben du coup c'est perdu.

Cool stuff:
- logic plutôt clever
- bonne utilisation du context manager open
- utilisation de xrange (à la place de range)
- cool d'avoir utilisé un generator pour la method `go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment