13
\$\begingroup\$

I'm very new to coding and am trying to work through projects for practical review of what I know, but I would like to be able to simplify/improve what I've made.

This is a project I saw online that I tried to tackle today. The task was to create something that would provide a list of the best possible words to play given a set of letters on your rack.

scores = {"a": 1, "c": 3, "b": 3, "e": 1, "d": 2, "g": 2,
         "f": 4, "i": 1, "h": 4, "k": 5, "j": 8, "m": 3,
         "l": 1, "o": 1, "n": 1, "q": 10, "p": 3, "s": 1,
         "r": 1, "u": 1, "t": 1, "w": 4, "v": 4, "y": 4,
         "x": 8, "z": 10}

def calc_score(word):
    """Calculate the score of a given word."""
    word_score = 0
    for x in word:
        word_score += scores[x]
    return word_score

def look_up():
    """Create the variable containing the master list of words."""
    read_dict = open("sowpods.txt", "r")
    master = read_dict.read()
    read_dict.close()
    master = master.split("\n")
    return master

word_master = look_up()

def rack_check(f_rack):
    """Check the letters in the rack against the SOWPOD dictionary and
    append valid words to a list."""
    valid_list = []
    for item in word_master:
        letter_bank = list(f_rack)
        for letter in item:
            if letter in letter_bank:
                valid = True
                letter_bank.remove(letter)
            else:
                valid = False
                break
        if valid == True:
            valid_list.append(item)
    return valid_list

def dict_maker():
    """Create a dictionary of the words with their scores."""
    valid_dict = {}
    f_list = rack_check(list((raw_input("Letters: ")).lower()))
    for i in f_list:
        valid_dict[i] = calc_score(i)
    return valid_dict

def list_print():
    """Print a list of the words in descending order of value."""
    dict = dict_maker()
    sort_dict = sorted(dict.items(), key=lambda k: k[1], reverse=True)
    for x in sort_dict:
        print x[0], "-", x[1]

list_print()

Calling the "list_print()" function runs through all the other necessary functions, but I feel like it could be cleaner. Any feedback would be greatly appreciated.

\$\endgroup\$
8
\$\begingroup\$

You have some generally good ideas, especially for a beginner. However, I echo @JoshDawson's critique that your functions are not well designed to make the code readable.

Code organization

  • Function names should be verb-like. If you have calc_score(), then for consistency, you should have check_rack(), make_dict(), and print_list().

  • look_up() needs to be renamed: it doesn't actually look anything up.

  • Names like something_check() or check_something() tend to be ambiguous. What is being checked? What happens if the check fails — does it raise an exception? Your rack_check() is even more unconventional: it acts as a filter on the global variable word_master!

    Furthermore, the rack_check() function should be broken down. In Python, filtering is easy: you can use the filter() builtin function, or better yet, use a list comprehension with an if clause. Therefore, what you really want is a function can_form_word(rack_letters, word) that returns True or False, which you can then apply to the master word list.

  • As you noted, it's weird that the entire challenge is solved within a function called list_print(). That function certainly does a lot more than printing a list, as its name would suggest! A pretty good convention is just to call the main function of your program main(). Furthermore, it is customary to call that function using if __name__ == '__main__': main() at the bottom of the code.

  • It is a bad idea to bury a raw_input("Letters: ") prompt inside your dict_maker() function. Avoid doing I/O and computations in the same function, since such mixing hinders readability and code reuse. In this case, the main() function should prompt for the input.

Implementation details

  • Take advantage of list comprehensions and generator expressions to make your code concise. In general, whenever you see a pattern like

    accumulator = init_value
    for val in some_list:
        accumulator += val
    return accumulator
    

    … then that is a candidate to be replaced by a one-liner.

  • Always call open() in the context of a with block, for tidier and more reliable code.

Suggested solution

I would define three very short helper functions. Then, the entire challenge can be solved in a few lines of code within main(). The master vocabulary list is a local variable within main(), instead of being a global variable that is initialized somewhere in the middle of the code and mysteriously incorporated into rack_check().

Instead of a dict, I've opted to use a list of (score, word) tuples, which is slightly easier to sort.

LETTER_POINTS = {...}

def calc_score(word):
    """Sum the point values of the letters of the word."""
    return sum(LETTER_POINTS[letter] for letter in word)

def read_word_list(filename):
    """Read a list of words, one word per line of the file."""
    with open(filename) as f:
        return [line.rstrip('\n') for line in f]

def can_form_word(rack_letters, word):
    """
    Determine whether a word can be formed only using the letters on the rack.
    """
    return all(word.count(c) <= rack_letters.count(c) for c in word)

def main():
    """
    For some letter tiles specified by the user, print the words in the
    SOWPOD dictionary that can be formed, in descending order of their
    Scrabble scores.
    """
    vocabulary = read_word_list('sowpods.txt')
    rack_letters = raw_input("Letters: ").lower()
    words = [word for word in vocabulary if can_form_word(rack_letters, word)]
    word_scores = [(calc_score(word), word) for word in words]
    for points, word in sorted(word_scores, reverse=True):
        print('{0}-{1}'.format(word, points))

if __name__ == '__main__':
    main()
\$\endgroup\$
8
\$\begingroup\$

The program is pretty good and clean so far, the docstrings are clear and it's easy to follow what's going on.

The scores dictionary is a constant, so it should be all uppercase.


Use a context manager to open the text file. This way you don't have to worry about the file not being closed, as the context manager does it for you.

def look_up():
    """Create the variable containing the master list of words."""
    with open('sowpods.txt', 'r') as f:
        return f.read().split('\n')

def calc_score(word):
    """Calculate the score of a given word."""
    word_score = 0
    for x in word:
        word_score += scores[x]
    return word_score

This can be rewritten using a list comprehension.

def calculate_score(word):
    """Calculate the score of a given word."""    
    return sum([SCORES[letter] for letter in word])

As you can see, we don't need to initialize an empty variable. Python is a self-documenting language.x isn't really a descriptive name, so I took the liberty to change it to letter. This one is a nitpick really, but I also change the function name to calculate_score.


Staying on comprehensions, you can use a dictionary comprehension in dict_maker. Here, we create a list of letters using list comprehension and then return a dictionary containing the word and its score.

def dict_maker():
    """Create a dictionary of the words with their scores."""
    letters = [letter for letter in raw_input('Letters: ').lower()]
    return {word: calculate_score(word) for word in rack_check(letters)}

I'm a little short on time to look over the logic, so I will finish the rest later, but here's some quick observations.

  • In list_print you assigned a dictionary using dict as its name. This is fairly uncommon practice, as it shadows a builtin. More on builtins.
  • word_master = look_up() should be contained in rank_check function. Furthermore, you can even replace for item in word_master: with for item in look_up()
\$\endgroup\$
  • 1
    \$\begingroup\$ calc_score() would be better written as return sum(SCORES[letter] for letter in word) — using a generator expression rather than a list comprehension. \$\endgroup\$ – 200_success Aug 9 '17 at 5:39
6
\$\begingroup\$

Instead of

scores = {"a": 1, "c": 3, "b": 3, "e": 1, "d": 2, "g": 2,
         "f": 4, "i": 1, "h": 4, "k": 5, "j": 8, "m": 3,
         "l": 1, "o": 1, "n": 1, "q": 10, "p": 3, "s": 1,
         "r": 1, "u": 1, "t": 1, "w": 4, "v": 4, "y": 4,
         "x": 8, "z": 10}

you may use more readable

letters = 'a c b e d g f i h k j m l o n q  p s r u t w v y x z '.split()
numbers = '1 3 3 1 2 2 4 1 4 5 8 3 1 1 1 10 3 1 1 1 1 4 4 4 8 10'.split()

scores = dict(zip(letters, map(int, numbers)))

The split() function convert your strings into lists and the last command creates the dictionary scores from the pairs of the corresponding letter/number.

(The map(int, numbers) applies the int() function to every member of the number list, as this list seems like ['1', '3', '3', ...].)

\$\endgroup\$
4
\$\begingroup\$

Code Cohesion

Several of your functions are doing too much work. For example dict_maker gets input from the user (not specified in docstring). It would be much better to pass that user input as a parameter.

Another function that does too much is list_print. Since it kind of acts like a main function, this is somewhat excusable; however, it's doing its own job (printing the tuples) while also having to call dict_maker.

You could break out some of the code in rack_check into a seperate function perhaps called can_be_spelled. I'd argue this helps with code readability since the function name documents what the inner for loop is doing while also allowing for code reuse.

def rack_check(rack, words):
    return [word for word in words if can_be_spelled(word, rack)]

def can_be_spelled(word, rack):
    letters = list(rack)
    for letter in word:
        if not letter in letters:
            return False
        letters.remove(letter)
    return True

Note how this also simplifies the logic too since a function return acts more naturally than a break.


Use a main function so you can import the code in this file:

if __name__ == "__main__":
    scores = get_scrabble_letter_scores()
    word_master = look_up("sowpods.txt")
    rack = raw_input("Letters: ")).lower()
    dict = dict_maker(scores, word_master, rack)
    list_print(dict)

Some quick things to consider:

  • You're using a fair amount of hard coded constants and globals. If you refactor these to function parameters, you can programatically change and reuse them (i.e. the score each letter gets, which file the words are stored in, whether you get the rack from a user or an AI). These changes come at little to no cost while also vastly improving the code's cohesion, so why not?
  • if valid reads better than if valid == True
  • look_up, dict_maker, and list_print are pretty vauge function names
  • Getting user input in a function restricts it to a user context
  • Printing output restricts a function to a printing context

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

Your function definition

def calc_score(word):
    """Calculate the score of a given word."""
    word_score = 0
    for x in word:
        word_score += scores[x]
    return word_score

may be as simple as

def calc_score(word):
    """Calculate the score of a given word."""
    return sum([scores[x] for x in word])

[scores[x] for x in word] is so called list comprehension - it creates the list of scores[x] for every x in word - and then the function sum() will add its items together.


Similarly, your function definition

def look_up():
    """Create the variable containing the master list of words."""
    read_dict = open("sowpods.txt", "r")
    master = read_dict.read()
    read_dict.close()
    master = master.split("\n")
    return master

may be as simple as

def look_up():
    """Create the variable containing the master list of words."""
    with open("sowpods.txt", "r") as sowpods:
        master = list(sowpods)
    return [m.rstrip('\n') for m in master]

Using the context manager (with) is preferred for files, as it properly closes a file for you, even after the occurrence of an exception, and moreover gives you individual lines in the sowpods variable.

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

A combination of other answers plus my own changes (with changes highlighted/explained in comments):

SCORES = {"a": 1, "c": 3, "b": 3, "e": 1, "d": 2, "g": 2,
         "f": 4, "i": 1, "h": 4, "k": 5, "j": 8, "m": 3,
         "l": 1, "o": 1, "n": 1, "q": 10, "p": 3, "s": 1,
         "r": 1, "u": 1, "t": 1, "w": 4, "v": 4, "y": 4,
         "x": 8, "z": 10} # Capitalized (constant)

def calc_score(word):
    """Calculate the score of a given word."""
    sum({SCORES[letter] for letter in word}) # Using set comprehension and sum function

def look_up():
    """Create the variable containing the master list of words."""
    with open("sowpods.txt", "r") as read_dict: # Ensures file is closed no matter what
        return read_dict.read().split("\n") # Returns immediately instead of creating an unnecessary variable

all_words = look_up()

def get_words(rack):
    """Check the letters in the rack against the SOWPOD dictionary and append valid words to a list."""
    return {word for word in all_words if set(rack).issuperset(word)} # Uses set, set comprehension

def get_word_scores(letters):
    """Create a dictionary of the words with their scores."""
    return {word: calc_score(word) for word in get_words()} # Uses map comprehension

def list_print():
    """Print a list of the words in descending order of value."""
    word_scores = get_word_scores(set(raw_input("Letters: ").lower())) # Changed var. name
    sorted_word_scores = sorted(dict.items(), key=lambda k: k[1], reverse=True) # Changed var. name
    for word_score in sorted_word_scores: # Changed var. name
        print word_score[0], "-", word_score[1]
if __name__ == "__main__":
    list_print()
\$\endgroup\$
0
\$\begingroup\$

Editing the code based on the feedback from everyone below, this is what I've come up with:

LETTER_SCORE = {"a": 1, "c": 3, "b": 3, "e": 1, "d": 2, "g": 2,
         "f": 4, "i": 1, "h": 4, "k": 5, "j": 8, "m": 3,
         "l": 1, "o": 1, "n": 1, "q": 10, "p": 3, "s": 1,
         "r": 1, "u": 1, "t": 1, "w": 4, "v": 4, "y": 4,
         "x": 8, "z": 10}

def calc_score(word):
    """Calculate the score of a given word."""
    return sum(LETTER_SCORE[letter] for letter in word)

def get_dictionary(filename):
    """Read a dictionary of valid scrabble words."""
    with open(filename, "r") as sowpods:
        return sowpods.read().split("\n")

def can_spell(word, rack):
    """Determine if a word can be spelled with the given rack."""
    letter_bank = list(rack)
    for letter in word:
        if letter not in letter_bank:
            return False
        letter_bank.remove(letter)
    return True

def make_valid_list(rack):
    """Make a list of all words that can be spelled with a given rack of letters."""
    return [word for word in get_dictionary("sowpods.txt") if can_spell(word, rack)]

def score_valid_words(rack):
    """Create a dictionary of the words with their scores."""
    valid_words = []
    for word in make_valid_list(rack):
        valid_words.append([word, calc_score(word)])
    return valid_words

def main():
    """For the letters in a user-specified rack, return the scores for
    all possible words checked against the SOWPODS dictionary for
    validity, and print the word and score for each in descending order."""
    vocabulary = get_dictionary("sowpods.txt")
    rack_letters = raw_input("Letters: ").lower()
    word_list = make_valid_list(rack_letters)
    word_scores = score_valid_words(rack_letters)
    for pair in sorted(word_scores, key=lambda k: k[1], reverse=True):
        if pair[1] > 0:
               print pair[0], "-", pair[1]

if __name__ == "__main__":
    main()

A quick breakdown of the changes:

  • Opted for more comprehensions in the functions.
  • Tried to make the functions and references a bit more descriptive, while breaking down some functions that were doing too much.
  • Added a main() function as well as a if __name__ == "__main__": parameter at the end. I'm still not too confident on the usage of that one, but I think it does what it needs to do in this circumstance.
  • Removed the most of the hard coded constants in favour of parameters that could be more easily transferred.

Thanks to everyone for the replies, they were a huge help. I don't know if it's necessary to write the main() function as it is, but for the moment, I put all the info recommended in there (though not exactly as recommended...)

\$\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.