Code Review

jam.py

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

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