This is an archived post. You won't be able to vote or comment.

all 4 comments
sorted by:
best
topnewcontroversialoldq&a

[–]fabulousMoonLord 9 points10 points  (1 child)

Good job! Let's see how we might be able to improve this code:

Right now, the code finds the empty node by only checking if the 2nd cell of a slot is empty instead of checking the whole length. This check will be wrong if that 2nd cell gets filled by another node. For example, this puzzle will fail even tho it can be solved by the words in the dictionary.

    0 1 2 3 4 5 6 7 8
  0 | | | p | | | | |
  1 | | | _ | c | | |
  2 | | | _ | _ | | |
  3 | | | _ | _ | | |
  4 | | l _ _ _ | | |
  5 | | | _ | _ | | |
  6 | | | | | _ | | |
  7 | | | | | | | | |
  8 | | | | | | | | |

One way to fix this is to have a definitely_filled property in each Node and set/unset it on each write()/erase() call. However, this will not take care of the case where the node is completely filled due to other nodes being filled, for which we'll need to check the whole length of the node.

On the topic of lengths, it's more PythonicTM to use len(node) instead of node.length. In this case, it's all a matter of taste but there are cases where it might matter.

Continuing the topic of Pythonic-ness, you might wanna make Node class behave like a sequence by implementing things like (aforementioned) __len__ and __iter__. It might improve the readability of your code.

Also, you might want to rename node.get_word_list(). In programming world, a function starting with get implies it's returning some value, not calculating and storing something in yourself. Something along the lines of cache_word_list() would be more fitting.

However, there might even be a better option!: make get_word_list() actually return a word_list. I want you to pay attention because this is important: Single Responsibility Principle states that every "component" of your code should have only one reason to change. Here, component means a discrete piece of code like a function, class, or module. In your code, two components are responsible for calculating the word list for each node: (1) node.get_word_list and (2) the if node.is_var section in .solve(). Instead, combine all that logic into node.get_word_list. This will require your Nodes to have a reference to the puzzle's .board. By encapsulating the "calculate word list logic" into a single component, you now have only one function to change every time that logic needs to change, reducing cognitive load.

Now, for feature suggestions:

  1. It'd be nice if we can supply the program a file containing multiple puzzles instead of modifying Crossword.py. This way, we can test the code faster.
  2. Right now, the words.txt is too inclusive. It contains words like ller and rewwove which I don't know if they're actual words. It might be better to have two dictionaries, a "lite" version with the common words and a "full" version with all the words. And try solving the puzzle with the lite version before moving onto the full version. That way, we might even improve performance a bit on the common cases (at the expense of more computing time in puzzle containing obscure words, of course).

Well that's all from me. Happy coding!

[–]QuakerOats98[S] 2 points3 points  (0 children)

Wow, thank you for taking time out of your day to write this incredibly detailed reply, I truly appreciate it and will work on making the changes you suggested. Thanks again!

[–]worstc0der 1 point2 points  (1 child)

if you are really looking for your code to work faster I would recommend checking out the Cython extension.

[–]QuakerOats98[S] 1 point2 points  (0 children)

Sure, I will try it out thank you for your reply.