IBondarNET / hangman.py

0 stars 0 forks source link

Some issues #1

Open GasperPaul opened 4 years ago

GasperPaul commented 4 years ago

In general, not bad. Code duplication is a sin. And I will punish it by subtracting score.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L40-L47

This can be a map or generator expression.

Also, you use wrong wildcard pattern, it should be _ instead of _. This violates 1B.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L50-L56

Can be a map as well.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L66

You forgot u.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L67

Should be a list, as most specification-defined function expect this to be list. I will not subtract score for this.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L75-L83

This two if branches with prints are, basically, the same and can be done like this:

if warnings_left < 0:
    guesses_left -= 1
print('... You have ' + warnings_left if warnings_left >=0 else 'no' + " warnings ...")

Also, the excessive use of continue is a sign of bad design. You can probably restate the loop condition to get rid of them. Moreover, in your case they are redundant, your code will get to a new cycle of the loop without them, because you use elifs.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L84-L94

This is litteraly the same code as above. Why is this a separate branch?

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L95-L105

This is almost the same code as above. You can just parametrize the message string and merge this with the other conditions' branches.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L108

is_word_guessed already returns a boolean value. You don't need to check for == True.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L115 https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L124

This code is in both branches of the corresponding ifs. Why not just put it above/below?

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L129-L138

Consider this failed test:

assert match_with_gaps('ap_ le', 'apple') == False, '_ can\'t be p, it\'s already guessed'
assert match_with_gaps('a_ ple', 'apple') == False, '_ can\'t be p, it\'s already guessed'
assert match_with_gaps('a_ _ le', 'apple') == True, '_ can be p'

It will fail, because you should check for letters already present in the word.

As is, this violates 3A.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L142

This can be written in a less verbose manner.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L147-L148

This is literally useless. You can just omit else altogether.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L152

This is hot mess. I think I got a brain injury from reading this.

All the problems from the original function above holds.

https://github.com/ilyabondarkm93/hangman.py/blob/35a0e1f0a428fb88bedb88416abf4d208718469a/hangman.py#L166-L194

This piece of code has a number of issues:

  1. Inner while loop here is unnecessary: if anything, you can just set a flag and check it where necessary, instead of duplicating a bunch of code. Or move that duplicate code into a new function.
  2. You can use the hint mechanic only once, which violates 3C.
  3. You are not computing the score for the end game correctly here. Using hints should not prevent user from getting the score.
  4. You are not tracking warnings and guesses right if a hint was used. This violates 2.D.
GasperPaul commented 4 years ago

Much better, especially with get_guessed_word and get_available_letters.

https://github.com/ilyabondarkm93/hangman.py/blob/6d5c83694d520ff9da80353fb6150da2655b098a/hangman.py#L93

This still have the same problem, you just side-stepped it. Consider this:

assert match_with_gaps('apple', 'apple') == True, 'Words match exactly'
assert match_with_gaps('ap_ _ e', 'apple') == False, '_ can\'t be p, it\'s already guessed'

https://github.com/ilyabondarkm93/hangman.py/blob/6d5c83694d520ff9da80353fb6150da2655b098a/hangman.py#L122

Specification 3C requires that you can use hints any number of times. You should not use this once mechanic.

https://github.com/ilyabondarkm93/hangman.py/blob/6d5c83694d520ff9da80353fb6150da2655b098a/hangman.py#L129-L133 https://github.com/ilyabondarkm93/hangman.py/blob/6d5c83694d520ff9da80353fb6150da2655b098a/hangman.py#L153-L156

This is duplicate code.

https://github.com/ilyabondarkm93/hangman.py/blob/6d5c83694d520ff9da80353fb6150da2655b098a/hangman.py#L157

You don't need that second condition there, just make the loop use <=0 instead of ==.