Sphinx - Wei Q #45
Sphinx - Wei Q #45
Conversation
| 'Z': 1 | ||
| } | ||
|
|
||
| #iterate through the dict to convert letter_pool to a list |
There was a problem hiding this comment.
My opinion about comments is if your code is readable/concise/you used descriptively named variables and functions to convey your intent then you do not need to leave comments in the code.
As a fellow developer, I know what your code is doing if it is written in a readable way. I feel that if you need comments to explain what is happening, then the code should be refactored to be more clear.
That's my opinion and other instructors might feel differently or you may end up working on a team that is ok with comments, but they can clutter up your project. It's important to keep large code bases easy to read and understand.
There was a problem hiding this comment.
Thank you! I totally get it. I tend to write long comments when I find it hard to read just the code. I will try to refactor it.
adagrams/game.py
Outdated
| } | ||
|
|
||
| #iterate through the dict to convert letter_pool to a list | ||
| LETTER_POOL_list = [] |
There was a problem hiding this comment.
This variable name mixes upper and lower case. I would call this variable letter_pool_list. Technically it is not a constant because you create an empty list and then you add letters to it.
Here's the official style guide for variable naming convention: https://peps.python.org/pep-0008/#function-and-variable-names
There was a problem hiding this comment.
Thank you! That makes sense. I will pay more attention to the style.
adagrams/game.py
Outdated
| for letter,count in LETTER_POOL.items(): | ||
| while count > 0: | ||
| LETTER_POOL_list.append(letter) | ||
| count -=1 |
There was a problem hiding this comment.
Prefer not to use while loop here since we know how many times we need to loop. A for range loop would work better: for _ in range(count):
A Pythonic way of writing this would be to use list comprehension. Lines 35-39 can be replaced with:
letter_pool_list = [letter for letter, count in LETTER_POOL.items() for _ in range(count)]There was a problem hiding this comment.
This is exactly what I was talking about being Pythonic. Thank you! I will try to absorb it.
adagrams/game.py
Outdated
| #to get 10 letters | ||
| for i in range(10): | ||
| #draw a random index from 0 to (totoal_letter_cout - i), given there are i letters | ||
| #there are already drawn & put back as the last i items in the list | ||
| draw_index = random.randrange(0, total_letter_count - i) | ||
| hand_of_letters.append(LETTER_POOL_list[draw_index]) | ||
| # move the drawed letter to the end and won't be picked in next round | ||
| #swith two items in the list : a, b = b,a |
There was a problem hiding this comment.
See my review comment above about leaving comments in your code.
There was a problem hiding this comment.
A more descriptive name for draw_index might be random_index this way you let someone reading your code know that this index is random.
There was a problem hiding this comment.
Thank you! I was confusing myself with my terrible naming when revisiting my code.
adagrams/game.py
Outdated
| LETTER_POOL_list[draw_index], LETTER_POOL_list[total_letter_count - i - 1] = \ | ||
| LETTER_POOL_list[total_letter_count - i - 1], LETTER_POOL_list[draw_index] |
There was a problem hiding this comment.
When you have a long line of code like this that needs to be broken up, you might want to consider what can you do to make it shorter. Having long lines of code like this can reduce readability.
Using variables with descriptive names will make your code a little longer which is ok because it will make your code easier to understand.
randomly_drawn_letter = LETTER_POOL_list[draw_index]
last_letter = LETTER_POOL_list[total_letter_count - i - 1]
randomly_drawn_letter, last_letter = last_letter, randomly_drawn_letter
adagrams/game.py
Outdated
| word = word.upper() | ||
| for char in word: |
There was a problem hiding this comment.
You can directly call upper on word in the loop without reassigning the value of word`.
for char in word.upper():
# rest of your code
adagrams/game.py
Outdated
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
You can remove extra blank lines from your file to keep the project neat
adagrams/game.py
Outdated
| max_word = word | ||
|
|
||
| return (max_word,max_score) |
| if len(word) == 10 or len(word) < len(max_word): | ||
| max_word = word |
adagrams/game.py
Outdated
| @@ -1,11 +1,118 @@ | |||
| import random | |||
| import string | |||
There was a problem hiding this comment.
This is an unused import. In your IDE, you can tell because it will look grey in color and if you hover over it you should see a message:
Unused imports should be removed before code review.
| if 7 <= len(word) <= 10: | ||
| score += 8 |
There was a problem hiding this comment.
I should have called this out earlier. Prefer the literal integers to be referenced by variables to make your code more self-documenting:
BONUS_LENGTH_MIN = 7
BONUS_LENGTH_MAX = 10
BONUS_POINTS = 8
if BONUS_LENGTH_MIN <= len(word) <= BONUS_LENGTH_MAX:
total_score = total_score + BONUS_POINTS
This my version of adagrams.