Let’s see what else might use a little improvement.

I’m still thinking about how to improve the half-dozen lines that do the recursion in Solver, but so far nothing has drifted into my mind. Meanwhile, let’s see what else we might want to improve. Probably one good criterion for picking something to explore is file size. Large files, by my lights, are suspect because they are likely to have accumulated some junk. They’re likely to have cohesion problems, and often have code in them that would probably be better somewhere else.

Component is 144 lines and Puzzle is 135. A quick look at Component tells me that there are two classes in that file, Component and its helper class, Notes. It is often my practice to build a class in a test file if I am test-driving it, or to build it in a production file if it relates to the class already in that file. I started doing the class in the test file, trying to work in the spirit of Keith Braithwaite’s “TDD As If You Really Mean It” notion. As for two production classes in one production file, I suspect that’s just laziness. Maybe lack of commitment to an idea? Let’s have a quick look at Notes and then see about moving it to its own file.

class Notes:
    def __init__(self, component, pos):
        self.owner = component
        self.pos = pos

    def __len__(self):
        return len(self.owner.candidates_at(self.pos))

    def __iter__(self):
        return iter(self.owner.candidates_at(self.pos))

    def puzzle_address(self):
        return self.owner.positions[self.pos]

    def become(self, values):
        if self.owner.candidates_at(self.pos) == values:
            return False
        self.owner.set_candidates_at(self.pos, values)
        return True

Right, well. It has a component (a row, column or box) and a pos, which, I imagine, is the position in that component. And it knows a lot about the component. The puzzle_address method is particularly interesting. That is returning the position 0-80 of the actual cell in the puzzle that the component is covering. That seems a bit invasive and subject to misuse. How is that used?

It is not used. Whew! Remove it! Commit: remove unused method.

We see from __len__ and __iter__ that Notes acts as an iterable, and that when iterated it gives us the candidates (now called notes) at a given position in the component (and therefore in the puzzle). And it has the become method. Let’s look at the uses for that.

class HiddenPairs:
    def check_component(self, component):
        changed = False
        component_notes = component.notes()
        for notes_a, notes_b in combinations(component_notes, 2):
            complement = [notes for notes in component_notes if notes is not notes_a and notes is not notes_b]
            pairs = self.hidden_pairs(notes_a, notes_b, complement)
            if pairs:
                notes = pairs[0]
                changed |= notes_a.become(notes)
                changed |= notes_b.become(notes)
                if changed:
                    Logger.log().count("Hidden Pair Count")
        return changed

The Hidden Pairs technique discovers cells in a component such that both cells contain the same two numbers, and those numbers do not occur elsewhere in the component. When that’s the case, those two cells must each contain one of those numbers in the final answer, and the other must contain the other. So those two Notes sets (candidates sets) can be set to just those two numbers.

Our code for discovering hidden pairs can find the pairs again, because it does not check to see if the cells in question already contain just the pair. So the become method returns True if it has changed the cell’s notes and False if it has not. We use the changed flag to decide whether to rerun our techniques: we run them until nothing changes.

I think we should move Notes to its own file, because that is how we generally do things. In a larger application, we would probably be setting up packages, but we have not had occasion to do that with our small applications. Be that as it may, we should be consistent where possible. F6 will move the class.

Commit: move Notes to own file.

Looking at Component, I notice that it implements __iter__, which implies that it could be, well, iterated:

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

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

There appear to be no uses of the __iter__: I can remove it and nothing fails. There are 35 (!) references to used_numbers. That’s interesting.

Almost all the references are in tests, checking to see that we’ve calculated the right values into the right places. There is but one production application of the method:

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

What even is that? It’s used three times, in methods for row, column and box, like this:

class Puzzle:
    @property
    def columns_are_valid(self):
        return all(column.is_valid for column in self.columns)

That’s used here:

class Puzzle:
    @property
    def is_correctly_solved(self):
        return self.rows_are_valid\
            and self.columns_are_valid\
            and self.sub_grids_are_valid

Whee. We collect up the used_numbers in the component and check whether they include all of ‘1’, ‘2’, and so on. Fine.

OK, those are just things that I flashed on. Let’s look at the whole Puzzle class together:

class Puzzle:

    @classmethod
    def evolve_with_change(cls, puzzle, change, position):
        old_string = puzzle.game
        new_string = old_string[:position] + change + old_string[position+1:]
        copy = puzzle.copy()
        copy.game = new_string
        positions_needing_update = Component.all_impacted_positions(copy, position)
        for position in positions_needing_update:
            values = [v for v in puzzle.candidates.at(position) if v != change]
            copy.candidates.set_at(position, values)
        return copy

    @classmethod
    def from_list(cls, a_list):
        joined = ''.join(a_list)
        return cls(joined)

    @classmethod
    def empty(cls):
        return cls.from_list(['0' for i in range(81)])

    def __init__(self, a_game_string):
        Logger.log().count("Puzzle Count")
        if len(a_game_string) == 9:
            self.available_values = '123'
            self.line_size = 3
        elif len(a_game_string) == 81:
            self.available_values = '123456789'
            self.line_size = 9
        else:
            raise ValueError('problem must have length 9 or 81')
        self._candidates = None
        self.game = a_game_string

    @property
    def candidates(self):
        if not self._candidates:
            self._candidates = Candidates(self.game)
        return self._candidates

    @candidates.setter
    def candidates(self, value):
        self._candidates = value

    @property
    def is_filled_in(self):
        return self.first_unsolved_position() == -1

    @property
    def columns(self):
        return [Component.column(self, col) for col in range(0, 9)]

    @property
    def columns_are_valid(self):
        return all(column.is_valid for column in self.columns)

    @property
    def rows(self):
        return [Component.row(self, row) for row in range(0, 81, 9)]

    @property
    def rows_are_valid(self):
        return all(row.is_valid for row in self.rows)

    @property
    def sub_grids(self):
        return [Component.sub_grid(self, pos) for pos in [0, 3, 6, 27, 30, 33, 54, 57, 60]]

    @property
    def sub_grids_are_valid(self):
        return all(sub_grid.is_valid for sub_grid in self.sub_grids)

    @property
    def is_correctly_solved(self):
        return self.rows_are_valid\
            and self.columns_are_valid\
            and self.sub_grids_are_valid

    def __repr__(self):
        lines = []
        for line in range(9):
            lines.append(self.game[9*line:9*(line+1)])
        return '\n'.join(lines)

    def unknown_cells(self):
        return [cell for cell in range(len(self.game)) if self.game[cell] == '0']

    def possible_answers(self, position):
        if self.game[position] != '0':
            return [self.game[position]]
        return self.candidates.at(position)

    def cell_at(self, position):
        return self.game[position]

    def all_components(self):
        rows = [Component.row(self, pos) for pos in range(0, 81, 9)]
        cols = [Component.column(self, pos) for pos in range(9)]
        subs = [Component.sub_grid(self, pos) for pos in [0, 3, 6, 27, 30, 33, 54, 57, 60]]
        return rows + cols + subs

    def set_candidates_at(self, position, values):
        self.candidates.set_at(position, values)

    def find_some_guesses_to_make(self):
        position = self.first_unsolved_position()
        guesses = self.possible_answers(position)
        Logger.log().count("Puzzle Guesses", len(guesses))
        puzzles = (Puzzle.evolve_with_change(self, guess, position) for guess in guesses)
        return puzzles

    def first_unsolved_position(self):
        try:
            return self.game.index('0')
        except ValueError:
            return -1

    def remove_naked_pair(self, pair, positions):
        changed = self.candidates.remove_naked_pair(pair, positions)
        return changed

    def copy(self):
        new_puzzle = Puzzle(self.game)
        new_puzzle.candidates = self.candidates.copy()
        return new_puzzle

Everything in there, except __repr__, shows one or more users. The __repr__, of course, is used when tests or the debugger want to print the object, so that’s fine.

The class methods aren’t bothering me. Two of them are utility constructors and the interesting one, of course, is evolve_with_change, which is the method we use to create a new puzzle when the Solver takes a guess. Let’s give it a closer look since we’re here:

class Puzzle:
    @classmethod
    def evolve_with_change(cls, puzzle, change, position):
        old_string = puzzle.game
        new_string = old_string[:position] + change + old_string[position+1:]
        copy = puzzle.copy()
        copy.game = new_string
        positions_needing_update = Component.all_impacted_positions(copy, position)
        for position in positions_needing_update:
            values = [v for v in puzzle.candidates.at(position) if v != change]
            copy.candidates.set_at(position, values)
        return copy

    def copy(self):
        new_puzzle = Puzzle(self.game)
        new_puzzle.candidates = self.candidates.copy()
        return new_puzzle
Note
I said they weren’t bothering me … but in fact the evolve one does bother me as soon as I let myself think about it!

The method is rather long, and seems to go through three or four steps.

The strings are weird, because we represent the game by a string of 81 characters. We could rename them to old_game_string perhaps. We could wrap a class around them. But this is kind of the bottom of things … unless we want to break out some smaller object.

Hm.
This object is kind of large. Maybe it needs to be broken up. We’ll let that idea perk: right now I don’t see much to break out.

Let’s try an Extract Method. Select the code, then Option-Command-M … Oh! Not gonna happen!

I was thinking I’d like to extract this bit and call it update_candidates or something:

        positions_needing_update = Component.all_impacted_positions(copy, position)
        for position in positions_needing_update:
            values = [v for v in puzzle.candidates.at(position) if v != change]
            copy.candidates.set_at(position, values)

But when I start the extract PyCharm points out that it would need four parameters to do the extract. Let me run it anyway just to see what horror it creates:

    @classmethod
    def evolve_with_change(cls, puzzle, change, position):
        old_string = puzzle.game
        new_string = old_string[:position] + change + old_string[position+1:]
        copy = puzzle.copy()
        copy.game = new_string
        cls.update_copy_candidates(puzzle, change, copy, position)
        return copy

    @classmethod
    def update_copy_candidates(cls, puzzle, change, copy, position):
        positions_needing_update = Component.all_impacted_positions(copy, position)
        for position in positions_needing_update:
            values = [v for v in puzzle.candidates.at(position) if v != change]
            copy.candidates.set_at(position, values)

I’m glad I did that, despite that I’m going to roll it back right now. The main thing to notice about that extract is that it made another class method. Of course it has to, we are in a class method. But maybe we shouldn’t be. Let’s look at this again:

    @classmethod
    def evolve_with_change(cls, puzzle, change, position):
        old_string = puzzle.game
        new_string = old_string[:position] + change + old_string[position+1:]
        copy = puzzle.copy()
        copy.game = new_string
        positions_needing_update = Component.all_impacted_positions(copy, position)
        for position in positions_needing_update:
            values = [v for v in puzzle.candidates.at(position) if v != change]
            copy.candidates.set_at(position, values)
        return copy

First let’s make a simple change, providing an optional parameter to copy, like this:

No. Belay that. Don’t we already have code, somewhere, that does that same candidate updating? Like when we decide on a value?

Look into SudokuTechniques. Bah. We do this:

class SudokuTechniques:
    def only_one_possible_value(self) -> bool:
        overall_change = False
        try_again = True
        while try_again:
            try_again = False
            for cell in self.puzzle.unknown_cells():
                available = self.puzzle.possible_answers(cell)
                if len(available) == 1:
                    overall_change = True
                    try_again = True
                    Logger.log().count("Techniques only_one")
                    self.puzzle = Puzzle.evolve_with_change(self.puzzle, available[0], cell)
        return overall_change

Down there at the bottom: we use our class method to do the job. I was hoping we already had instance methods for this.

Reflection

Need to stop and think here.

    @classmethod
    def evolve_with_change(cls, puzzle, change, position):
        old_string = puzzle.game
        new_string = old_string[:position] + change + old_string[position+1:]
        copy = puzzle.copy()
        copy.game = new_string
        positions_needing_update = Component.all_impacted_positions(copy, position)
        for position in positions_needing_update:
            values = [v for v in puzzle.candidates.at(position) if v != change]
            copy.candidates.set_at(position, values)
        return copy

This method’s job is to create a new Puzzle. It’s OK that it is a class method. However, it does its job in a very invasive way, creating a copy and then jamming a new string into it and then reaching down into it and updating its candidates.

Action

I think it should be able to say this:

    @classmethod
    def evolve_with_change(cls, puzzle, change, position):
        new_puzzle = puzzle.copy()
        new_puzzle.assign_cell(position, change)
        return new_puzzle

PyCharm suggested assign_cell. I have no idea why. It doesn’t exist. Excellent, that means I can write it:

    def assign_cell(self, position, change):
        self.game = self.game[:position] + change + self.game[position+1:]
        positions_needing_update = Component.all_impacted_positions(self, position)
        for position in positions_needing_update:
            values = [v for v in self.candidates.at(position) if v != change]
            self.candidates.set_at(position, values)

And, we are green. Out-[CENSORED]-standing! Commit: Do work of evolving on instance side not class side.

We have a long article here. Let’s reflect and sum up.

Reflection

Everywhere we look, we have questions. That’s not necessarily bad, since if we dip randomly into the code it should be no surprise that we have no context. But we also found things to improve … like an unused __iter__. So we found a few little things, no bit deal.

Except that sometimes we only have a few minutes, or are on another mission. But we might well check to see who uses that __iter__, and finding it unused … we could remove it. Now, whether we should remove it is a local decision. My own practice, in general, would be to remove it. It’s taking up space in the code and in my mind, so removing it simplifies my job a bit. So especially if it’s simple, let’s get rid of it. If it’s not simple, we might ask around … but still get rid of it. Do it in a separate commit in case anyone wants to pick it up later. But they never will, I’d bet.

Then we turned our attention to the class method evolve_with_change. At first, I just thought we could extract a method and rename some things for clarity … but the extract showed clearly that the method was doing things from outside an instance that could be done inside. The result was a much simpler class method—so simple that we could actually remove it and replace it with an instance method to evolve. But maybe, since it’s a creator, a class method is better.

But all the work is actually done by just creating new instance and sending it an update message.

Summary

There is always room for improving the design. The changes are quite often simple, just a method or two, and the results are that the code stays alive and tends to remain understandable.

There is one interesting “drawback”: the next time someone visits evolve_with_change, they might possibly recognize that it is different from what it was, if their memory works that way. If we always work in this way, improving things that need it, and if we work together, not in competition, these things are not troublesome. I’ve seen this in a team room:

Alex:
“Hey, evolve_with_change seems different!”
Blake:
“Yeah, Cameron and I worked on that last week. Shall I sit with you to work on it?”
Alex:
(May say yes or no, depending on need.)

As GeePaw Hill puts it, we’re in the business of changing code. When we change it to make it better, as often as we can, change isn’t troubling, it’s welcome. It may take a little getting used to. But it’s well worth it to keep the code alive.

Everywhere we look … there’s room for improvement. I think that’s delightful and fun. What do you think?

See you next time!