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

Tigers: Pavi and Luciane #74

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Tigers: Pavi and Luciane #74

wants to merge 4 commits into from

Conversation

prannulu
Copy link

No description provided.

Copy link

@chimerror chimerror left a comment

Choose a reason for hiding this comment

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

Good Work!

This code works correctly and is very readable and clean! I did leave a few very minor comments on style stuff, but in general I feel pretty confident in y'all's understanding of the material so I'm marking this Green.

@@ -1,11 +1,156 @@
from operator import le

Choose a reason for hiding this comment

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

It looks like le, which is part of this import statement never got used. Most likely this is because of a "helpful" VSCode feature that will attempt to add import statements for you automatically, but often just cause problems. I would recommend just turning it off. Ansel provided these steps:

  1. Open the Settings screen, by either going to the Code > Preferences > Settings menu pick, or using the ⌘, keyboard shortcut.
  2. In the Search settings, look for: analysis.
  3. Look for a result titled Python > Analysis: Auto Import Completions.
  4. Uncheck the option to Offer auto-import completions.

Ex: Since the value of 'A' is 9, the first 9 elements of this list will be 'A'
appearing 9 times, and so on.
bag_of_letters = ['A', 'A', ..., 'B', ...]
"""

Choose a reason for hiding this comment

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

Ooh, nice docstring, and good use of adding a helper function!

for i in range(number_of_letters):
bag_of_letters.append(letter)
return bag_of_letters

def draw_letters():

Choose a reason for hiding this comment

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

This looks good, I just have a few style notes!

def draw_letters():
pass
hand_of_letters = [] # to be filled with 10 one-letter strings

Choose a reason for hiding this comment

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

Minor style preference: I'm not sure there's much value to this type of comment. It is more about "what" the code will do rather that "why" it will do it a certain way. I generally recommend removing "what" comments because the true marker of what the code does is the code itself. Additionally, these types of comments often fall out of sync with the code they're commenting.

It's generally OK to use them when scaffolding, but once your code is there, you can remove them.

bag_of_letters = create_bag_of_letters()

for i in range(10):
#size_of_letter_bag = len(bag_of_letters)

Choose a reason for hiding this comment

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

Minor style preference: Building on to that, I would normally remove commented code just to keep things clean.

hand_of_letters.append(letter_picked)
bag_of_letters.remove(letter_picked)

return hand_of_letters

def uses_available_letters(word, letter_bank):

Choose a reason for hiding this comment

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

This looks good, one minor style comment!

#We checked to make sure that removing wouldn't affect original letter_bank array here:
# copy_of_letter_bank.remove('B')
# print("letter bank:", letter_bank)
#print("copy of letter bank:" , copy_of_letter_bank)

Choose a reason for hiding this comment

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

Minor style preference: Much as I said in the last function, there's not as much benefit to keeping this type of "exploratory" code in, even while commented after y'all figure it out. That being said, writing a bit of code like this to figure things out is a really good tendency to have! Just, don't keep it in. :3

Choose a reason for hiding this comment

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

There are some other places with commented code, but from here on out, I'll skip pointing it out to not repeat myself.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing this out! As you mentioned, this was mostly used as scaffolding for writing the code itself, but I will remove in final submissions moving forward! :)

return False

return True


def score_word(word):

Choose a reason for hiding this comment

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

This looks fine!

if len(word) > 6 and len(word) < 11:
final_score += 8
return final_score


def get_highest_word_score(word_list):

Choose a reason for hiding this comment

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

OK, I really like this implementation! I feel that some people would find the use of a dictionary here to keep track the important information for each word was overkill, but I think it serves a very good method for organizing the calculations and making it easier to work with for pretty minimal extra memory usage. (Not going to lie, by "some people" I mean me, because I was about to comment on it until I understood y'all's logic.)

Otherwise, I did have a small style comment...

max_words.clear()
max_words.append(word_dict["word"])
# CASE 1

Choose a reason for hiding this comment

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

While I am not sure y'all need this comment or the one below in general, this comment is pretty terse. That is, "CASE 1" doesn't really tell me what case 1 actually means. So something like # CASE 1: No tied scores would be better. At the same time, that is right on the border of "what" and "why" as far as comments go and I say it's more on the "what" side, and thus wouldn't bother keeping it. (Usually the tone of my "why" comments are like "I had to code it this terrible way no one should have to code it because of this annoying reason".)

Plus, this is very minor, but make sure to keep your comments indented with what they're commenting just because with Python using indentation to mark blocks, the indentation of everything is a large part of understanding code written in it.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for all the great feedback! I learned a lot about commenting etiquette 💬 😸

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.

3 participants