Dave's Branch #3
Conversation
JustCallMeRay
left a comment
JustCallMeRay
left a comment
There was a problem hiding this comment.
Really good work! If you haven't had a code review this may be a bit much, (I am known for being a bit too thorough but even normal people reviewing professional devs tend to have about one comment per file.) Also I intentionally left more comments than I would normally would.
It's easy to get imposter syndrome after the first review (I did) so remember that comments are not "bad" or "errors" they are just the jumping off point for conversation and improvements to your thinking.
I tend to average about two comments per file at work so 4 is still pretty good. Also I like the design which is really impressive for a junior developer. It is very functional but don't think it needs to be any more complex. If you wanted to take it further I would suggest iteratively adding words via a builder and stopping when you get a custom exception like FullGrid.
Again Really good!
| yield value.value | ||
|
|
||
|
|
||
| class Direction(Enum, metaclass=DirectValueMeta): |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
2023-Oct-18/Dave/main.py
Outdated
| word_search = generate_word_search( | ||
| rows=4, cols=4, word_list=word_list) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Could you explain the 'enforce it with , *,' part?
def func(*, keywordEnforcedArg:int):
...
func(1)Throws (and I get a red squiggle)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| output = "" | ||
| for row in self.word_grid: | ||
| for char in row: | ||
| output += char + " " | ||
| output += "\n" | ||
| return output |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 :)
2023-Oct-18/Dave/main.py
Outdated
| 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 != " ": | ||
| if (char_to_write is not target_char) and (target_char != " "): |
There was a problem hiding this comment.
A mix of is not and != here again.
There was a problem hiding this comment.
Ah yeah, this was because I had intended to use is / is not with everything, but received a SyntaxWarning about using it with a literal.
Turns out this is because is / is not and == / != are not interchangeable and the former checks type as well, as far as I can tell.
Seems like the answer is to make everything == / !=.
That's enough backticks for now.
There was a problem hiding this comment.
Should this be committed?
There was a problem hiding this comment.
Following my last two commits, yes I think so.
I still intend to change the WordGrid class to provide a generator for legal placements and I think I'm also going to empty the contents of main into modules, package things up nicely with a src layout and add the necessary configuration files to make the package installable with pip install -e so that it can be tested with sensible import statements. Then add some tests.
But I think it makes sense for that to be a separate pull request (that I can own!).
As for now, I'm happy to commit.
Also would you suggest a squash-and-merge to minimise spam in the Git history here?
No description provided.