-
Notifications
You must be signed in to change notification settings - Fork 2
Dave's Branch #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 5 commits
57c8d05
3697ff9
8bf41d3
426d9d7
7ed053b
c22d826
74ea0db
3690887
05fc53c
85c3c93
05320b4
4df03ff
667ea7b
f811b83
af63a19
34a8ec7
efddf53
0068470
d71ac00
1700325
d71bf54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||
| 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): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
| 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) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if Also not a fan of every parameter being keyworded, seems excessive but might that's more down to style.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain the 'enforce it with 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
def func(*, keywordEnforcedArg:int):
...
func(1)Throws (and I get a red squiggle)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
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 | ||
|
JustCallMeRay marked this conversation as resolved.
Outdated
|
||
| else: | ||
| return None | ||
|
DaveDangereux marked this conversation as resolved.
Outdated
|
||
|
|
||
|
|
||
| def place_words(word_search: WordGrid, word_list=list[str]) -> WordGrid | None: | ||
|
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: | ||
|
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 != " ": | ||
|
DaveDangereux marked this conversation as resolved.
Outdated
|
||
| return False | ||
| return True | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| main() | ||
| 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like the "prefer composition over inheritance"!
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you clarify?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
DaveDangereux marked this conversation as resolved.
|
||
| 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 | ||
|
DaveDangereux marked this conversation as resolved.
|
||
| def __iter__(self): | ||
| yield self.row | ||
| yield self.col | ||
| 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() | ||
|
DaveDangereux marked this conversation as resolved.
Outdated
|
||
|
|
||
| def __iter__(self): | ||
| for row in self.word_grid: | ||
| yield row | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think this should iterate over a list of cells rather than rows.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason not to just iterate over the Something about
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)] | ||
|
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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've used
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cursory research suggests that However, just for clarity, the use of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I try to do this, though:
I get:
So it's not clear to me presently how to concatenate two strings with 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 | ||
|
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 | ||
|
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 | ||
| ) | ||
|
DaveDangereux marked this conversation as resolved.
|
||
Uh oh!
There was an error while loading. Please reload this page.