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

Hangman assignment #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Hangman assignment #1

wants to merge 1 commit into from

Conversation

jescht
Copy link

@jescht jescht commented May 22, 2018

For code review

Copy link

@cheeyim cheeyim left a comment

Choose a reason for hiding this comment

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

great coding overall.. looks like you're good to go for OOP!


# Complete with your own, just for fun :)
LIST_OF_WORDS = []
LIST_OF_WORDS = ['cookies', 'cream', 'love', 'vinegar']
Copy link

Choose a reason for hiding this comment

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

great words !

if list_of_words == []:
raise InvalidListOfWordsException
else:
return random.choice(list_of_words)
Copy link

Choose a reason for hiding this comment

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

very clean code here ... random library is perfect for this...

if word == "":
raise InvalidWordException
else:
return len(word)*"*"
Copy link

Choose a reason for hiding this comment

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

another piece of clean code!

raise InvalidGuessedLetterException(Exception)
if len(masked_word) != len(answer_word):
raise InvalidWordException
if answer_word =="" or masked_word =="" :
Copy link

Choose a reason for hiding this comment

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

it's possible to combine all conditions in line 41 with "or" for the same InvalidWordException

#convert answer_word and character to lower case
lower_character = str.lower(character)
lower_answer = str.lower(answer_word)
if lower_character not in lower_answer:
Copy link

Choose a reason for hiding this comment

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

we can remove line 48 and 49 since there's no change to masked_word ... notice line 55 will return masked_word anyway ...

if lower_character in lower_answer:
indices=find_substring(lower_answer, lower_character)
for index in indices:
masked_word = masked_word[:index] + lower_character + masked_word[index + 1:]
Copy link

Choose a reason for hiding this comment

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

great restructuring of word there!

raise GameFinishedException
else:
#make game case insensitive by converting all strings to lower case
letter = str.lower(letter)
Copy link

Choose a reason for hiding this comment

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

good idea to lowercase the letter and answer_word here.. which means you can remove the lowercase at _uncover_word() function ...!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants