Another quick look for something needing improvement.

I still have PyCharm open on Component, so let’s see what we can notice. Here, to scan, is how it stands now. See what pops out for you when you look over the code. And let me know, if you see something that I miss.

class Component:
    sub_grid_starts = [
         0,  0,  0,  3,  3,  3,  6,  6,  6,
         0,  0,  0,  3,  3,  3,  6,  6,  6,
         0,  0,  0,  3,  3,  3,  6,  6,  6,
        27, 27, 27, 30, 30, 30, 33, 33, 33,
        27, 27, 27, 30, 30, 30, 33, 33, 33,
        27, 27, 27, 30, 30, 30, 33, 33, 33,
        54, 54, 54, 57, 57, 57, 60, 60, 60,
        54, 54, 54, 57, 57, 57, 60, 60, 60,
        54, 54, 54, 57, 57, 57, 60, 60, 60,
              ]

    def __init__(self, puzzle, start, steps):
        self.puzzle = puzzle
        self.start = start
        self.positions = [start + step for step in steps]

    @classmethod
    def all_impacted_positions(cls, puzzle, position):
        row = cls.row(puzzle, position).positions
        col = cls.column(puzzle,position).positions
        sub = cls.sub_grid(puzzle, position).positions
        return set(row) | set(col) | set(sub) - {position}

    @classmethod
    def row(cls, puzzle, position):
        if puzzle.line_size == 9:
            steps = [0, 1, 2, 3, 4, 5, 6, 7, 8]
        else:
            steps = [0, 1, 2]
        start = position - position % puzzle.line_size
        return cls(puzzle, start, steps)

    @classmethod
    def column(cls, puzzle, position):
        if puzzle.line_size == 9:
            steps = [0, 9, 18, 27, 36, 45, 54, 63, 72]
        else:
            steps = [0, 3, 6]
        start = position % puzzle.line_size
        return cls(puzzle, start, steps)

    @classmethod
    def sub_grid(cls, puzzle, position):
        if puzzle.line_size == 9:
            steps = [0, 1, 2, 9, 10, 11, 18, 19, 20]
        else:
            steps = []
        start = cls.sub_grid_starts[position]
        return cls(puzzle, start, steps)

    @classmethod
    def with_notes(cls, notes_list):
        # special constructor for testing
        from puzzle import Puzzle
        assert len(notes_list) == 9
        puzzle = Puzzle.empty()
        instance = cls(puzzle, 0, range(9))
        for i, notes in enumerate(notes_list):
            instance.set_candidates_at(i, notes)
        return instance

    def __iter__(self):
        return iter(self.used_numbers)

    def all_candidates(self):
        return [self.candidates_at(pos) for pos in range(9)]

    def notes(self):
        return [Notes(self, pos) for pos in range(9)]

    def candidates_at(self, position):
        puzzle_cell_index = self.positions[position]
        if self.puzzle.cell_at(puzzle_cell_index) != '0':
            return []
        else:
            return self.puzzle.candidates.at(puzzle_cell_index)

    def set_candidates_at(self, position, candidates):
        puzzle_cell_index = self.positions[position]
        self.puzzle.set_candidates_at(puzzle_cell_index, candidates)

    def remove_candidates_at(self, position, to_remove):
        candidates = self.candidates_at(position)
        candidates = [candidate for candidate in candidates if candidate not in to_remove]
        self.set_candidates_at(position, candidates)

    def find_candidates_pairs(self, condition):
        groups = combinations(self.all_candidates(), 2)
        return [p1 for p1, p2 in groups if condition(p1, p2)]

    def get_lonely_ones(self):
        result = []
        for value in '123456789':
            positions = [pos for pos in range(9) if value in self.candidates_at(pos)]
            if len(positions) == 1:
                result.append([value, self.positions[positions[0]]])
        return result

    def remove_naked_pair(self, pair):
        changed = self.puzzle.remove_naked_pair(pair, self.positions)
        if changed:
            Logger.log().count("NakedPair")
        return changed

    @property
    def used_numbers(self):
        return [self.puzzle.cell_at(index) for index in self.positions if self.puzzle.cell_at(index) != '0']

    @property
    def is_valid(self):
        for c in '123456789':
            if c not in self.used_numbers:
                return False
        return True

I see some “global” issues, by which I mean they have a more broad impact than things we can change right here.

  • The word “notes” has begun to be used where we have had “candidates”. “Notes” seems better to me, pehaps because it is shorter and less obtrusive, though “candidates” might be more apt.
  • I’ve never liked “sub-grid” as a name and “box” might be better. That one we might be able to improve simply by renaming the sub_grid class method.

You know, maybe it won’t be that bad. Let’s do some renames.

Rename sub_grid to box. Green. Commit: rename sub_grid to box.

How about all_candidates? What even is that?

    def all_candidates(self):
        return [self.candidates_at(pos) for pos in range(9)]

Arguably we shouldn’t start renaming here: it might be better to rename starting at the bottom, like with candidates_at. But now I want to know what this method is about. It appears that it will produce a list of lists, since:

    def candidates_at(self, position):
        puzzle_cell_index = self.positions[position]
        if self.puzzle.cell_at(puzzle_cell_index) != '0':
            return []
        else:
            return self.puzzle.candidates.at(puzzle_cell_index)

How is this baby used?

There are seven tests, which I’ll ignore because I figure they are probably just checking transformations on the candidates, er, notes. In actual play:

class Component:
    def find_candidates_pairs(self, condition):
        groups = combinations(self.all_candidates(), 2)
        return [p1 for p1, p2 in groups if condition(p1, p2)]

Ah, we use this, probably about once, probably to find naked pairs or hidden pairs. Right, we have this:

class NakedPairsTechnique:
    def __init__(self, puzzle):
        self.puzzle = puzzle

    def apply(self):
        changed = False
        for component in self.puzzle.all_components():
            changed |= self.apply_for_component(component)
        return changed

    def apply_for_component(self, component):
        def cond(p1, p2):
            return len(p1) == 2 and p1 == p2

        changed = False
        for naked_pair in component.find_candidates_pairs(cond):
            changed |= component.remove_naked_pair(naked_pair)
        return changed

Right, we use it to find naked pairs, pairs of notes of length two and with the same contents. We had in mind that we could use that method or similar ones, passing in different conditions. So far that has not come to pass.

However: we do see in the Component, some very knowedgable methods, such as get_lonely_ones, and remove_naked_pair. Those two methods are part of the operation of some solution techniques. I would argue that Component should be about relatively simple access to the rows, columns, and boxes of the puzzle’s solution and notes, and should not include detailed knowledge of Sudoku strategies or game-play.

In that light, the find_candidates_pairs makes sense, I think, in a way that, at least as named, get_lonely_ones and certainly remove_naked_pair may not.

What we have here is a cohesion problem. The Component object has at least two kinds of behavior. It has simple access behavior, without much semantic content, and it has some behavior with more intelligence, in the methods we’ve been talking about here. By my lights, a better design would have the basic behavior in one object and the smarter behavior in another. When we need help from the basic object, we would prefer generalized methods, like the find_candidates_pairs, which imports its game-oriented intelligence.

We will quite often, in real life, find imperfect situations like this one, and we may decide to live with them. Often they are not terribly harmful. That said, I think we do well to clean up as much imperfection as we reasonably can: it pays off in faster development.