Skip to content
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

Sunitha N -Adagrams-ADA-C22-Sphinx #46

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all 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
115 changes: 111 additions & 4 deletions adagrams/game.py

Choose a reason for hiding this comment

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

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.

Copy link
Author

@nelasunitha nelasunitha Sep 24, 2024

Choose a reason for hiding this comment

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

Thanks for your valuable feedback. I came to know small little things which are very important for a programmer to know and to maintain the neat and clean code base. You explained clearly about how unnecessary code, extra lines, white spaces matters. The usage of rand_index instead of rand_int, lookup and the other variable taught me how to name different variables

Original file line number Diff line number Diff line change
@@ -1,11 +1,118 @@
import random

Choose a reason for hiding this comment

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

Usually we leave a blank line after imports

You can review the official Python style guide about blank line conventions.


def draw_letters():
pass
Letter_POOL = {
'A': 9,
'B': 2,
'C': 2,
'D': 4,
'E': 12,
'F': 2,
'G': 3,
'H': 2,
'I': 9,
'J': 1,
'K': 1,
'L': 4,
'M': 2,
'N': 6,
'O': 8,
'P': 2,
'Q': 1,
'R': 6,
'S': 4,
'T': 6,
'U': 4,
'V': 2,
'W': 2,
'X': 1,
'Y': 2,
'Z': 1
}
#taking an empty list to store drawn letters
drawn_letters =[]

Choose a reason for hiding this comment

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

Missing white space after equal sign, should look like drawn_letters = []

It's a small thing, but as you write more code (and eventually contribute to a large codebase) it's important to be consistent with whitespaces so that the whole project stays neat. Here's the official Python style guide that covers whitespaces.

total_letters = sum(Letter_POOL.values())

HAND_SIZE = 10
while len(drawn_letters) < HAND_SIZE:
# Generating a random number between 1 and the total number of letters
rand_index = random.randint(1, total_letters)

# Finding the corresponding letter
cumulative_count = 0
for letter, frequency in Letter_POOL.items():
cumulative_count += frequency
if rand_index <= cumulative_count:
drawn_letters.append(letter)
Letter_POOL[letter] -= 1
total_letters -= 1
break

Choose a reason for hiding this comment

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

I think one blank line after break is ok to help separate the logical parts of this function, but having 2 is not needed. Line 62 can be removed.

return drawn_letters


def uses_available_letters(word, letter_bank):
pass

# converting all the letters in letter bank to upper case
letter_bank = [letter.upper() for letter in letter_bank]

Choose a reason for hiding this comment

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

👍 nice job using list comprehension

# making a dictionary for a letter_bank to check the frequency of letters
letter_frequency_dict = {}
for letter in letter_bank:
if letter in letter_frequency_dict:
letter_frequency_dict[letter] += 1
else:
letter_frequency_dict[letter] = 1
#converting all the letters of word into uppercase and checking the frequebcy of the letter, if found decrementing the value or else returning false

for letter in word.upper():
if letter not in letter_frequency_dict or letter_frequency_dict[letter] == 0:
return False
else:
letter_frequency_dict[letter] -= 1
return True

def score_word(word):
pass
LETTER_SCORES = {
'A': 1, 'E': 1, 'I': 1, 'O': 1, 'U': 1, 'L': 1, 'N': 1, 'R': 1, 'S': 1, 'T': 1,
'D': 2, 'G': 2,
'B': 3, 'C': 3, 'M': 3, 'P': 3,
'F': 4, 'H': 4, 'V': 4, 'W': 4, 'Y': 4,
'K': 5,
'J': 8, 'X': 8,
'Q': 10, 'Z': 10
}
word = word.upper()
total_score = 0

total_score += sum(LETTER_SCORES.get(letter, 0) for letter in word )

BONUS_LENGTH_MIN = 7
BONUS_LENGTH_MAX = 10
BONUS_POINTS = 8

if BONUS_LENGTH_MIN <= len(word) <= BONUS_LENGTH_MAX:
total_score += BONUS_POINTS

return total_score


def get_highest_word_score(word_list):
pass
max_word = word_list[0]
max_word_score = score_word(max_word)
#iterating from the second word
for word in word_list[1:]:

Choose a reason for hiding this comment

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

👍 nice job slicing off the first word since it's already defined as the max_word on line 132.

word_score = score_word(word)
if word_score > max_word_score:
max_word = word
max_word_score = word_score

# words with same score
elif word_score == max_word_score:
# Prefer the word with the fewest letters, unless one has 10 letters
if len(word) == 10 and len(max_word) != 10:

Choose a reason for hiding this comment

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

We could use a constant variable to reference 10, maybe something like TIE_BREAKER_LENGTH = 10 or something else to describe why 10 is significant in this logic.

max_word = word
elif len(word) < len(max_word) and len(max_word) != 10:
max_word = word

max_word_score = max(max_word_score, word_score)

Choose a reason for hiding this comment

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

Do we need line 149 when you reassign the value of max_word_score to be the current highest score on lines 124 - 126?

return (max_word, max_word_score )