4
\$\begingroup\$

I am in my first year of programming with Python 3 and am wondering if anyone had a better way to program this problem. When the user provides up to 7 random letters as a single string, the program should read the ‘wordlist.txt’ file, list the words from that file that can be constructed using some or all of those letters.

For example, if the user provides three letters, ‘AER’ as a string, the program will suggest the following words from the given file: 'AE', 'AR', 'ARE', 'EA', 'EAR', 'ER', 'ERA', 'RE'

This is what I have for now:

wordlist=list(line.strip('\n') for line in open('wordlist.txt','r'))

def wordFinder():
    words=[]
    letters=str.upper(input('enter up to 7 letters:'))
    for word in wordlist:
        candidate=True
        letterlist=list(letters)
        for letter in word:
            if letter not in letterlist:
                candidate=False
                break
            else:
                letterlist.remove(letter)
        if candidate==True:
            words.append(word)                
    return words

>>>options()

enter up to 7 letters:aer
>>>['AE', 'AR', 'ARE', 'EA', 'EAR', 'ER', 'ERA', 'RE']

Even though it works I am looking to see if there is an easier/more efficient way for doing it. I came close with list comprehension, but I couldn't figure out how to avoid input letters matching more than once with the potential words.

def options2():
    words=[]
    letters=set(str.upper(input('Enter up to 7 letters:')))
    words=[w for w in wordlist
        if set(w).issubset(letters)
        and len(w)<=len(letters)]
    return words

>>>options2()

enter up to 7 letters:aer
>>>['AA','AE','AR','ARE','EA','EAR','EE','ER','ERA','ERE','ERR','RE','REE']

If any changes could be made for the second function to work that would be awesome.

\$\endgroup\$
3
\$\begingroup\$

The way you handle the file is not ideal. It's not apparent in such a small program, but you are not .close()-ing the file after opening it. Of course, Python will close the file once the program is done running, but in a bigger application, using your method would get clunky since you would have to remember to call .close() each time you open a file. Python's with keyword can take care of that for you, though.

with open('wordlist.txt','r') as file:
    wordlist=list(line.strip('\n') for line in file
    # more file handling code...

# file will be automagically closed after `with` block exits

I think your wordFinder function does too much (also per PEP8 convention it should be named word_finder). I would expect a function named like that to just find and return words, but it also handles user input, which I think should be a separate function.


Speaking of handling input, your program does not seem to handle non-letter characters at all. What happens when a user enters abc_123 as input? I'm guessing it will just use ABC_123 to look in your file, and return results for ABC after it finds nothing for _123. My suggestion would be to get input in a while loop, and exit the loop only after validating it:

input_is_only_letters = False
while not input_is_only_letters:
    letters = str.upper(input('enter up to 7 letters:'))
    if letters.isalpha():
        input_is_only_letters = True
    else:
        print('Please enter only letters A-Z (not case sensitive)')

I would also wrap this logic into its own function that returns the input.


A few small things:

  • Since you are using Python 3, you can (and IMO should) use type hints, which I did in the below code.

  • Get in the habit of making a separate function for each "action" or separate type of thing. This makes debugging and testing code much easier.

  • When you assign values, use a space on each side of the equal sign, e.g., my_value = 42.

  • Use snake_case naming for functions, variables, etc.

  • You don't need to check for if my_var == True: or if my_var == False:, you can simply write if my_var: and if not my_var:

  • Try to get in the habit of writing docstring for your functions, you will thank yourself later when you or others have to revisit your code.

  • Get in the habit of executing your self-contained calls in if __name__ == '__main__', so that if you were to import this into another module it wouldn't execute the file itself when it's loaded. See this article on thepythonguru.com for more information.

  • I haven't looked at your list comprehension code, maybe someone else will think of a way to do it that way.


My refactored code.

Note that I don't have a way to test it, to the best of my knowledge it should work fine but please let me know if I made an error.

from typing import List

def get_7_letters_input() -> str:
    """Captures up to 7 letters from stdin"""
    input_is_only_letters = False
    while not input_is_only_letters:
        letters = str.upper(input('enter up to 7 letters:'))
        if letters.isalpha():
            if len(letters) <= 7:
                input_is_only_letters = True
            else:
                print('Please enter no more than 7 letters')
        else:
            print('Please enter only letters A-Z (not case sensitive)')
    return letters

def get_words_from_file(filename: str) -> List:
    """Opens a file and splits it into a list for each new line"""
    with open(filename, 'r') as file:
        return list(line.strip('\n') for line in file

def find_words_matching_letters(letters: str) -> List:
    """Find words that match the letters provided"""
    words = []
    word_list = get_words_from_file('wordlist.txt')

    for word in word_list:
        candidate = True
        letterlist = list(letters)
        for letter in word:
            if letter not in letterlist:
                candidate = False
                break
            else:
                letterlist.remove(letter)
        if candidate:
            words.append(word)                
    return words

if __name__ == '__main__':
    find_words_matching_letters(get_7_letters_input())
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ And you can also make get_words_from_file a generator. \$\endgroup\$ – N74 Mar 29 '18 at 6:46
  • \$\begingroup\$ @N74 Feel welcome to edit my post to add that if you feel like it \$\endgroup\$ – Phrancis Mar 29 '18 at 6:47
  • \$\begingroup\$ I don't have edit privileges, and anyway it is just a suggestion to speedup things, as you don't need to store the whole wordlist in memory. \$\endgroup\$ – N74 Mar 29 '18 at 6:49
2
\$\begingroup\$

The code in the post checks if a word can be made out of the letters in the input like this:

candidate=True
letterlist=list(letters)
for letter in word:
    if letter not in letterlist:
        candidate=False
        break
    else:
        letterlist.remove(letter)

The operations letter not in letterlist and letterlist.remove(letter) take time proportional to the length of letterlist, and this has to be done for every letter in word, making the runtime for the check proportional to the product of the numbers of letters in the two words.

It's possible to perform this check in linear time by counting the number of occurrences of each letter in the word, for example using collections.Counter. First, you'd count the occurrences of each letter in the input:

letter_counts = Counter(letters)

and then for each word in the wordlist, check if there are enough of each letter in the word:

all(n <= letter_counts[c] for c, n in Counter(word).items())
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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