Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions 2023-Oct-18/Dave/direction.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
from enum import Enum, EnumMeta
from point import Point


class DirectValueMeta(EnumMeta):
# This allows us to unpack enum members directly, in the format:
# row, col = Direction.RIGHT
def __getattribute__(cls, name):
value = super().__getattribute__(name)
if isinstance(value, cls):
value = value.value
Comment thread
DaveDangereux marked this conversation as resolved.
Outdated
return value

# This allows us to iterate through enum members in a for loop and access
# the Point value directly
def __iter__(cls):
for value in super().__iter__():
yield value.value


class Direction(Enum, metaclass=DirectValueMeta):
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming from a C style background I think this enum does too much but a python/java/not C/C++ engineer would probably think it's fine.
(See a later comment about this enum where it is used in main.py)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be interested to hear a suggestion for an alternative. I definitely like the dot syntax. I had previously thought about using a named tuple, but something about that feels wrong, since there will only ever be one of these with fixed values and it seems odd to have the option to instantiate something that will only ever be one version of itself.

Open to suggestions, though.

Copy link
Copy Markdown
Contributor Author

@JustCallMeRay JustCallMeRay Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C solution is macros (not in python thank god) The solution in other languages might be an enum or a hashmap. C++ has compile time constants (constexpr) so you can create a constexpr map or even a runtime map as everything is so fast it's fine.

RIGHT = Point(0, 1)
DOWN = Point(1, 0)
UP = Point(-1, 0)
LEFT = Point(0, -1)
UP_LEFT = Point(-1, -1)
UP_RIGHT = Point(-1, 1)
DOWN_LEFT = Point(1, -1)
DOWN_RIGHT = Point(1, 1)
94 changes: 94 additions & 0 deletions 2023-Oct-18/Dave/main.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import random
from word_grid import WordGrid
from point import Point
from direction import Direction
from placement import Placement


def main():
word_list = ["MEOW", "CAT", "WOOF", "DOG"]

word_search = generate_word_search(
rows=4, cols=4, word_list=word_list)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if generate_word_search only returns None (or a type that is convertible to false) when it fails, have it throw/raise instead. The later if else feels like clutter (doesn't feel like something main should have to do. (Unless the language can't throw)).

Also not a fan of every parameter being keyworded, seems excessive but might that's more down to style.
If you want every parameter to be keyworded enforce it with , *,.
If you want to make sure the user puts the right thing into your function I would do that with typing rather than key word arguments. For example generate_word_search could take a WordGrid and a WordList and return (or edit inline) the initialised and valid WordGrid.

Copy link
Copy Markdown
Collaborator

@DaveDangereux DaveDangereux Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain the 'enforce it with , *,' part?

Re: keyword arguments: I will play the 'down to style' card here, because I personally prefer the at-a-glance readability of the arguments here and instantiating the word grid in main feels like haemorrhaging functionality that I would prefer to keep contained.

Absolutely agree about throwing instead of the if statement, though. Will fix that up.

Copy link
Copy Markdown
Contributor Author

@JustCallMeRay JustCallMeRay Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain the 'enforce it with , *,' part?

def func(*, keywordEnforcedArg:int):
    ...

func(1)

Throws (and I get a red squiggle)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful, but I don't think it's what I want here. The keyword operators are just for clarity as I'm passing literals rather than named variables and it helps me to see that rows and columns aren't getting mixed up.

But I take your point from yesterday about word_list=word_list being too verbose, so I think that's fine to get rid of.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, just realised that this isn't straight-forward. Positional arguments can't come after keyword arguments. Means either rewriting the function signature or just accepting a little repetition. For now, I think I prefer repetition. Seems too minor to be caught up on.


if word_search:
print(word_search)
print("Find these words:")
print(", ".join(word_list))
else:
print("Failed to generate word search.")


def generate_word_search(rows: int, cols: int, word_list: list[str]) -> WordGrid | None:
word_search = WordGrid(rows, cols)

attempts = 0

while attempts < 10:
Comment thread
JustCallMeRay marked this conversation as resolved.
Outdated
word_search.initialise_word_grid()
filled_word_search = place_words(
word_search=word_search, word_list=word_list)
if filled_word_search:
filled_word_search.fill_blank_space()
return filled_word_search
else:
attempts += 1
continue
Comment thread
JustCallMeRay marked this conversation as resolved.
Outdated
else:
return None
Comment thread
DaveDangereux marked this conversation as resolved.
Outdated


def place_words(word_search: WordGrid, word_list=list[str]) -> WordGrid | None:
Comment thread
DaveDangereux marked this conversation as resolved.
Outdated
filled_word_search = word_search

for word in word_list:
placements = get_all_legal_placements_for_word(
word_grid=filled_word_search, word=word
)
if placements:
position, direction = random.choice(placements)
filled_word_search.write_line(
position=position, orientation=direction, data=word
)
else:
return None

return filled_word_search


def get_all_legal_placements_for_word(
word_grid: WordGrid, word: str
) -> list[Placement] | None:
Comment thread
DaveDangereux marked this conversation as resolved.
Outdated
legal_placements = []

# Iterate through all possible grid locations and orientations
for row_index, row in enumerate(word_grid):
for col_index, col in enumerate(row):
for direction in Direction:
position = Point(row_index, col_index)

target_line = word_grid.read_line(
position, direction, len(word))
if not target_line:
continue

line_can_be_placed = is_legal_placement(
target_line=target_line, line_to_write=word
)
if not line_can_be_placed:
continue

legal_placements.append(Placement(position, direction))

return legal_placements


def is_legal_placement(target_line: str, line_to_write: str) -> bool:
for target_char, char_to_write in zip(target_line, line_to_write):
if char_to_write != target_char and target_char != " ":
Comment thread
DaveDangereux marked this conversation as resolved.
Outdated
return False
return True


if __name__ == "__main__":
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer

if __name__ != "__main__":
    raise someError("Running main as module is not supported")
main()

or

if __name__ != "__main__":
     print("Running main as module is not supported")
     exit(not_zero)
main()

As they force the correct usage on the user but it's more just style so up to you. These (not the main() call) can be added to the top of the file instead of the bottom to make you encounter this error quicker (and python has to do less compiling)

Copy link
Copy Markdown
Collaborator

@DaveDangereux DaveDangereux Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Would this influence testing?

As in would this not raise an error if I imported main for testing? Might be completely off-target here, but keen to know.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't require main in my tests as it is normally very small and a full end to end integration test you may want to do with command line instead.
Other languages have one entry point called main so you can't EVER compile it in (unless you change the entry point which is kinda hacky)

main()
13 changes: 13 additions & 0 deletions 2023-Oct-18/Dave/placement.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from dataclasses import dataclass
from point import Point
from direction import Direction


@dataclass
class Placement:
position: Point
direction: Direction
Comment on lines +8 to +9
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the "prefer composition over inheritance"!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a design pattern thing here is a good explanation but it's a very common phrase.


def __iter__(self):
yield self.position
yield self.direction
Comment thread
DaveDangereux marked this conversation as resolved.
13 changes: 13 additions & 0 deletions 2023-Oct-18/Dave/point.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from dataclasses import dataclass


@dataclass
class Point:
row: int
col: int

# This allows us to unpack a point with the syntax:
# row, col = point
Comment thread
DaveDangereux marked this conversation as resolved.
def __iter__(self):
yield self.row
yield self.col
76 changes: 76 additions & 0 deletions 2023-Oct-18/Dave/word_grid.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import random
import string
from point import Point
from direction import Direction


class WordGrid:
def __init__(self, rows: int, cols: int):
self.rows = rows
self.cols = cols
self.initialise_word_grid()

def __repr__(self):
return self.get_stringified_word_grid()
Comment thread
DaveDangereux marked this conversation as resolved.
Outdated

def __iter__(self):
for row in self.word_grid:
yield row
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this should iterate over a list of cells rather than rows.
Feel free to implement get_rows() or similar instead

Copy link
Copy Markdown
Collaborator

@DaveDangereux DaveDangereux Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to just iterate over the word_grid attribute?

Something about get_rows() feels non-Pythonic to me, but I have just watched a talk by Raymond Hettinger emphasising this idea.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well what does a gird iterate over? Cells? Rows? Columns? Answer is Read the source code! less reading = good! Getting one and iterating over that solves this problem, (you want every line to be readable without reading the rest.


def initialise_word_grid(self):
self.word_grid = [[" " for col in range(
self.cols)] for row in range(self.rows)]
Comment thread
DaveDangereux marked this conversation as resolved.
Outdated

def get_stringified_word_grid(self) -> str:
output = ""
for row in self.word_grid:
for char in row:
output += char + " "
output += "\n"
return output
Comment on lines +29 to +33
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've used join in some places and += here. I suggest keeping consistent. (Also python is weird with strings so keep in mind that one version is probably much slower (either sometimes or always))

Copy link
Copy Markdown
Collaborator

@DaveDangereux DaveDangereux Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursory research suggests that join is faster than iterating with +=, so the for char in row block is an easy (and agreeable) fix.

However, just for clarity, the use of += to concatenate two strings is still fine outside of an iterator?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both are fine just a lack of consistency looks odd (plus if I don't know what join and += do you have to google both)

Copy link
Copy Markdown
Collaborator

@DaveDangereux DaveDangereux Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I try to do this, though:

"first".join("second")

I get:

sfirstefirstcfirstofirstnfirstd

So it's not clear to me presently how to concatenate two strings with join(). Python documentation seems to suggest that my code above is correct and the inconsistency is because they're doing different things.

Let me know if you know something I don't :)


def read_line(
self, position: Point, orientation: Direction, length: int
) -> str | None:
result = ""

current_row, current_col = position
next_row, next_col = orientation
grid_rows = len(self.word_grid)
grid_cols = len(self.word_grid[0])

for i in range(length):
if not 0 <= current_row < len(self.word_grid):
return None

if not 0 <= current_col < len(self.word_grid[0]):
return None
Comment thread
DaveDangereux marked this conversation as resolved.
Outdated

result += self.word_grid[current_row][current_col]
current_row += next_row
current_col += next_col

return result

def write_line(self, position: Point, orientation: Direction, data: str) -> bool:
can_write_line = self.read_line(position, orientation, len(data))
if not can_write_line:
return False
Comment thread
DaveDangereux marked this conversation as resolved.
Outdated

current_row, current_col = position
next_row, next_col = orientation

for char in data:
self.word_grid[current_row][current_col] = char
current_row += next_row
current_col += next_col

return True

def fill_blank_space(self):
for row_index, row in enumerate(self.word_grid):
for col_index, char in enumerate(row):
if char == " ":
self.word_grid[row_index][col_index] = random.choice(
string.ascii_uppercase
)
Comment thread
DaveDangereux marked this conversation as resolved.