taraspyndykivskiy / hangman_project

Hangman game on Python.
0 stars 0 forks source link

Some issues #1

Closed GasperPaul closed 4 years ago

GasperPaul commented 4 years ago

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L20-L21

This will not work in most Windows terminals.

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L16 https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L35

This can be done without regexp. This is basically text.isalpha() and len(text) == 1 which will work much faster. Also, the general specification for the task allows this kind of input and require us to subtract warnings in this case.

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L65-L69

This is basically result = result and (symbol in letters_guessed). This can be done using function all or using map.

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L75-L97

This is way too complicated:

  1. You don't need to preconstruct word_being_guessed with Nones.
  2. This function must return string as per specification, not pair.
  3. There is no functional need in constructing two strings. This can be done using not_formated = formated_word.replace('_', ' ').

This whole function is basically a map.

Also, you should use _ instead of _ or . This violates specification 1B and 2.B.4.

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L85-L86

Why two sifferent styles of initialization?

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L99

So, during this function you have your data converted from string to list then back to string. Why do this? If you are converting types already, why not choose something more useful, like set?

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L105-L108

This is final_result = ' '.join(available_letters).

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L110

This is the same as return final_result.

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L120

Few issues here:

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L175-L188

Few issues here:

  1. my_word should use _ as a wildcard, not .
  2. This function fails on a test cases like this:
    assert match_with_gaps('ap le', 'apple') == False
    assert match_with_gaps('a ple', 'apple') == False
    assert match_with_gaps('a  le', 'apple') == True

    If you already guessed letter it can not be in the wildcard.

Reread specification 3A.

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L190-L195

You changed show_possible_matches signature, it should only take one argument.

Checking equality with True is redundant, match_with_gaps already returns bool value. This is not endemic to this function, you do this all around your code.

Also, this function should not return anything. It should print the values found. This violates specification 3B.

https://github.com/taraspyndykivskiy/hangman_project/blob/a89dcc233b957f26f7a6f3ad6d404ead526594c5/hangman.py#L199

Here you have the same problems with score, warnings and guesses, as above.

GasperPaul commented 4 years ago

https://github.com/taraspyndykivskiy/hangman_project/blob/dd0c8388d395a1230d69e35f98500d87e9b71f5a/hangman.py#L39

True if(i in letters_guessed) else False is the same as just i in letters_guessed.

https://github.com/taraspyndykivskiy/hangman_project/blob/dd0c8388d395a1230d69e35f98500d87e9b71f5a/hangman.py#L60

This is while not is_word_guessed(secret_word, user_symbols) and guesses_counter>0. Comparing bool to a constant bool is esentially a tautology:

A not A A == False A == True
False True True False
True False False True
A == False => not A
A == True  => A

https://github.com/taraspyndykivskiy/hangman_project/blob/dd0c8388d395a1230d69e35f98500d87e9b71f5a/hangman.py#L74

This should not reset.

https://github.com/taraspyndykivskiy/hangman_project/blob/dd0c8388d395a1230d69e35f98500d87e9b71f5a/hangman.py#L90-L92

Why is this here exactly? You have only one case where you subtract warnings, and there you already have this code.

https://github.com/taraspyndykivskiy/hangman_project/blob/dd0c8388d395a1230d69e35f98500d87e9b71f5a/hangman.py#L106-L125

The idea is right, but you can write this in less code.

A bunch of word transformations you do are unnecessary. For example, you don't need a word without spaces and underscores. You also don't need to count each symbol. You just neet to know if that symbol is already guessed in that word.

False if dict_my_word[symbol]!=dict_other_word[symbol] else True is just dict_my_word[symbol]==dict_other_word[symbol].

else: return False can be just return False.

https://github.com/taraspyndykivskiy/hangman_project/blob/dd0c8388d395a1230d69e35f98500d87e9b71f5a/hangman.py#L192

You can just add it later, and move this code in the if above it.

taraspyndykivskiy commented 4 years ago

Concerning match_with_gaps() issue, I suppose, that we have to count and compare amount of every letter symbol, cause match_withgaps("a ple", "apple") must return False (according to 3A point), although "p" is actually in two strings.

GasperPaul commented 4 years ago

Why though? For each _ you just need to know if the corresponding letter in the other_word is present elsewhere in my_word. If so, it was guessed and _ can't be matched against it, ohterwise we're good.

taraspyndykivskiy commented 4 years ago

Yeah, I got this. It looks genius in comparing with previous method. P.S. Already committed these changes.

GasperPaul commented 4 years ago

https://github.com/taraspyndykivskiy/hangman_project/blob/0856d4dbf623f59a393c669f7cce6fcb221bec81/hangman.py#L109

This is exactly the place when writing one-liner is doing more harm than good. Better not do this.

Let's dissect this:

all([(symbol in other_word
      and all([True if ( (my_word[i]==" ") 
                          and (other_word[i].isalpha() 
                               and not (other_word[i] in my_word) )  
                          or (my_word[i].isalpha())) 
               else False 
               for i in range(len(my_word))]) 
      and symbol.isalpha()
      and all([(my_word[i]==other_word[i] and my_word[i].isalpha()) 
               or not my_word[i].isalpha() 
               for i in range(len(my_word))])
      ) 
      or not symbol.isalpha()
      for symbol in my_word]
   )
  1. This construction:
    True if ( (my_word[i]==" ") 
           and (other_word[i].isalpha() 
                and not (other_word[i] in my_word) )  
           or (my_word[i].isalpha())) 
    else False 

    This is same problem as before:

    True if P else False => P

    What you trying to say is:

    my_word[i] == " "
    and other_word[i].isalpha()
    and other_word[i] not in my_word  # note the position of 'not' 
    or my_word[i].isalpha()
  2. other_word[i].isalpha() is always true, because it's the word from the list, they are all proper words.
  3. Both inner all are not dependent on symbol so they can be moved out:
    A = all([my_word[i] == " " and other_word[i] not in my_word or my_word[i].isalpha()
         for i in range(len(my_word))])
    B = all([my_word[i]==other_word[i] and my_word[i].isalpha() 
         or not my_word[i].isalpha() 
         for i in range(len(my_word))])
    C = all([symbol in other_word and symbol.isalpha() or not symbol.isalpha()  for symbol in my_word])
    return A and B and C
  4. Let's consider a following expression: P = (X and Y) or not Y
X Y not Y X and Y P
False False True False True
False True False False False
True False True False True
True True False True True

After some time you'll see that the actual condition we are trying to express is X or not Y. Which means, that in C we don't need to check symbol.isalpha(), it should be:

C = all([symbol in other_word or not symbol.isalpha() for symbol in my_word])

By the same logic:

B = all([my_word[i]==other_word[i] or not my_word[i].isalpha() for i in range(len(my_word))])

Now, if we consider that not my_word[i].isalpha() is, essentially, a way of saying my_word[i]==" " — because there are no other possible symbols in this strings — we can apply this to A:

A = all([my_word[i].isalpha() or other_word[i] not in my_word for i in range(len(my_word))])
  1. Let's now consider the meaning of this expressions:

A means "all symbols in my word are either letters, or corresponding letter in other word is not present in my word (i.e. can be in the wildcard place)".

B means "all symbols in my word either are equal to the corresponding letters in other word, or are wildcards".

What means C? It means "all symbols in my word can either be found somewhere in the other word, or are wildcards". Does it make any sense to you? Moreover, it is redundant. If B is true, this statement will be true too, because my_word[i]==other_word[i] is a more restricting way to say symbol in other_word. The former requires a match in a certain place i, the latter requires just match in any place. We can say, that this condition follows from B:

B C B -> C
False False True?
False True True?
True False False
True True True

Note that in bold are the cases when we cannot say if C is true based on B, it may or may not be so, and we must assume the expressin true. But what it actually means to us is this:

B C B and C
False False False
False True False
True False False (and impossible)
True True True

Which basically means that B and C and B -> C => B. So C is unnecessary and can be removed.

What you are doing with A and B is this: you check for one possible condition in each of this statements and substitute True for the cases of other.

Finally, we have this:

A = all([my_word[i].isalpha() or other_word[i] not in my_word for i in range(len(my_word))])
B = all([my_word[i]==other_word[i] or not my_word[i].isalpha() for i in range(len(my_word))])
return A and B

Now, because they have the same quantification over my_word by i you can combine the conditions under that quantificator. You'll have something of the form (X or Y) and (Z or not X), which can be simplified further. Try it!

https://github.com/taraspyndykivskiy/hangman_project/blob/0856d4dbf623f59a393c669f7cce6fcb221bec81/hangman.py#L104 https://github.com/taraspyndykivskiy/hangman_project/blob/0856d4dbf623f59a393c669f7cce6fcb221bec81/hangman.py#L178 One of this replaces is redundand.

https://github.com/taraspyndykivskiy/hangman_project/blob/0856d4dbf623f59a393c669f7cce6fcb221bec81/hangman.py#L150 entered_character.isalpha()==False is just not entered_character.isalpha().

taraspyndykivskiy commented 4 years ago

So, yeah, it's way more simple. My final result is next: return all([((my_word[i]==other_word[i]) or not my_word[i].isalpha()) and (other_word[i] not in my_word or my_word[i].isalpha()) for i in range(len(my_word))]) Assuming that (my_word[i]==other_word[i]) == A and not my_word[i].isalpha() ==not B and other_word[i] in my_word == C, I tried to simplify the expression "(A or not B) and (not C or B)" in my notebook, but didn't succeed. problem

taraspyndykivskiy commented 4 years ago

Anyways, I've committed intermediate result.

GasperPaul commented 4 years ago

I meant the first blue box from your notes when I was talking about simplification. It can be read as a more or less logical condition. But the version you end up with in the commit is fine. You may as well not write it in one line and present two separate conditions, up to you.

Good job! You can close this, if you're finished.

taraspyndykivskiy commented 4 years ago

I think that the last update of match_with_gaps() isn't worth changing, cause it seems to me, that condition in line with return can't be simplified to a more laconic one. In my opinion, it's easier to realise the principle of conditional expression (with last update) and find out what it means in general for our specific function.

taraspyndykivskiy commented 4 years ago

I am really grateful to you for spending so much time pointing at such an obvious things and giving recommendations for optimizing my "code". I genuinely appreciate that. Thanks!