4
\$\begingroup\$

I solved this problem: the Edabit Hangman Game challenge.

Create a function that, given a phrase and a number of letters guessed, returns a string with hyphens - for every letter of the phrase not guessed, and each letter guessed in place.

Notes

  • The letters are always given in lowercase, but they should be returned in the same case as in the original phrase.
  • All characters other than letters should always be returned.

I saw people solved this in a one line but is it my solution good? I want to take everything and solving into smaller pieces and how to improve the solution any please explain why is it better to use that solution. Does the built in functions used as a cheating?

def hangman(phrase, lst):
   alphanumerical_characters = ['1','2','3','4','5','6','7','8','9',".","!","?",","] #characters which are alphanumerical 
   #if requires from A-Z and numbers 1-9 and other symbols
   result = ''
   new_list = [] #creating new_list because i want to take the uppercase letters and lowercase letters
   new_list = lst.copy() #we create copy of that to get the lowercase elements
   for element in phrase:
      if element.isupper():
         element = element.lower()
         if element in lst:
            element = element.upper()
            new_list.append(element) #this for loop requires if element is uppercase , make lowercase and then check if its in lst
   for element in phrase:  
      if element.isspace():
         result += " "
      elif element not in new_list:
         if element in alphanumerical_characters:
            result += element
         else:
            result += '-'
      else:
         result += element #in second for loop we check if there's space between characters , element not in new_list, element in list
   return result 
\$\endgroup\$
3
  • \$\begingroup\$ You have two for element in phrase loops. That jumps out to me as bad practice. \$\endgroup\$ Commented Sep 1, 2022 at 0:33
  • \$\begingroup\$ Also, why are you calling .!?, alphanumeric? \$\endgroup\$ Commented Sep 1, 2022 at 0:34
  • \$\begingroup\$ Yea i know that is dry repeating same concept more times , but i wanted to story every character from A-Z , numbers 1-9 and some symbols i think they are .!?, \$\endgroup\$
    – mcccuklev
    Commented Sep 1, 2022 at 10:55

2 Answers 2

5
\$\begingroup\$

Any non-trivial function ought to have a docstring describing its purpose and any constraints on its arguments. That's also a good place for the unit tests, if we use the doctest module.


alphanumerical_characters is a misleading name for that variable - it seems to be intended as a list of non-alphabetic characters. It's incomplete (e.g. missing ', which is present in many words, including the first one of this sentence). I think it would be better to check whether a character is alphabetic, and hide those, revealing only the ones that don't match any alphabetic letter. We could use element.isalpha() for that:

if element.isalpha():
    result += '-'
else:
    result += element

There's no point assigning [] to new_list, since we immediately replace that with a copy of lst.


element is a long name for iterating over characters. Since its scope is small, I'd go for a very short name, such as c.


I don't really follow the logic in the if element.isupper() test. It looks like we could eliminate all that by just considering uppercase versions of lst when testing. E.g.

guesses = set(lst) | set(lst.upper())

Now guesses contains both the original lowercase letters and their uppercase partners.


Some performance notes:

  • Testing in works better if the right-hand argument is a set rather than a list.
  • Because strings are immutable, building them character by character with += is inefficient. Consider creating a list of characters, then converting to a string with ''.join(chars).

Applying these suggestions, we get

def hangman(phrase, lst):
    '''
    Replace any letters in phrase not present in lst (case-insensitively) with '-'.

    >>> hangman('abc', '')
    '---'
    >>> hangman('abc', 'ax')
    'a--'
    >>> hangman('ABC', 'cb')
    '-BC'
    >>> hangman("? !", '')
    '? !'
    >>> hangman("Don't worry!", 'dor')
    "Do-'- -orr-!"
    '''
    guesses = set(lst) | set(lst.upper())
    result = []
    for c in phrase:
        if c.isalpha() and not c in guesses:
            c = '-'
        result.append(c)
    return ''.join(result)


if __name__ == '__main__':
    import doctest
    doctest.testmod()

We can further refine that by using a generator to build result:

guesses = set(lst) | set(lst.upper())
return ''.join(c if c in guesses or not c.isalpha() else '-' for c in phrase)
\$\endgroup\$
2
  • 1
    \$\begingroup\$ The challenge site actually passes lst as a list (eg, hangman("tree", ["r", "t", "e"])), not a string, so lst.upper() will unfortunately raise an exception. My fault - I didn't quote the entire challenge text in the body of the question during my edit. \$\endgroup\$
    – AJNeufeld
    Commented Aug 31, 2022 at 16:21
  • 1
    \$\begingroup\$ Thanks for the extra info. Not hard to modify my suggestion, to either map upper() over the list, or to ''.join() it first. \$\endgroup\$ Commented Sep 1, 2022 at 6:52
6
\$\begingroup\$

The first half of your code is converting the given lst into new_list, with additions for the upper case letters.

There are a few problems with this. Consider the following test:

hangman("""A Big BLACK BUG bit a BIG BLACK BEAR
           & the BIG BLACK BEAR bleed BADLY!!!!""", ["e", "b", "a"])

First, the length of phrase is 84 characters, so for element in phrase: is going to loop for 84 iterations. 39 of those will pass the element.isupper() test, and will be converted to lower case, tested if they exist in lst, and if so, will be added to new_list. At the end of the loop, new_list will contain ['e', 'a', 'b', 'A', 'B', 'B', 'A', 'B', 'B', 'B', 'A', 'B', 'E', 'A', 'B', 'B', 'A', 'B', 'E', 'A', 'B', 'A']!

This is way more than necessary. In fact, only ['e', 'b', 'a', 'E', 'B', 'A'] is necessary. You shouldn't be looping over phrase; you can simply loop over the existing lst and add the uppercase variants to new_list:

    new_list = lst.copy()
    for element in lst:
        new_list.append(element.upper())

Second issue, the output of the above is:

'A B-- B-A-- B-- b-- a B-- B-A-- BEA- - --e B-- B-A-- BEA- b-ee- BA---!!!!'

This issue has 2 parts:

  1. The & has been converted to a - (issue already noted in answer by Toby)
  2. The newline (\n) has been converted to a space ( )!

Toby has already covered part #1. Part #2 is caused by:

      if element.isspace():
         result += " "

... which turns everything that matches .isspace() (spaces, horizontal tabs, vertical tabs, newlines, etc) to an actual space character. To avoid this conversion, simply add the element unchanged:

      if element.isspace():
         result += element

Is using built-in functions cheating?

If you are not explicitly told not to use them, then they are allowed, ... even encouraged. The point of these challenges is to improve your coding skills. A problem asking you to might ask you to compute a square-root without using the math module or the pow() function or ** operator. Using the cmath library would be considered "cheating" by some, and merely "thinking outside the box" by others. The question is: are you doing these challenges to improve your skills? If the answer is yes, are you learning anything?

Getting the top score on a coding challenge site by asking Code Review to improve the efficiency of your code would be cheating; asking Code Review to help you improve your coding skills is not.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.