7
\$\begingroup\$

Here is my solution for CodeWars - Highest Scoring Game

Given a string of words, you need to find the highest scoring word.

Each letter of a word scores points according to its position in the alphabet: a = 1, b = 2, c = 3 etc.

You need to return the highest scoring word as a string.

If two words score the same, return the word that appears earliest in the original string.

All letters will be lowercase and all inputs will be valid

Example:

Input: 'man i need a taxi up to ubud'

Output: 'taxi'

This is my solution:

def word_value(input_word):
      values = {
        'a': 1,
        'b': 2,
        'c': 3,
        'd': 4,
        'e': 5,
        'f': 6,
        'g': 7,
        'h': 8,
        'i': 9,
        'j': 10,
        'k': 11,
        'l': 12,
        'm': 13,
        'n': 14,
        'o': 15,
        'p': 16,
        'q': 17,
        'r': 18,
        's': 19,
        't': 20,
        'u': 21,
        'v': 22,
        'w': 23,
        'x': 24,
        'y': 25,
        'z': 26
      }
      value = 0
      for letter in input_word:
          value += values[letter]
      return value

def high(x):
    word_list = x.split(" ")

    word_values = []
    for word in word_list:
        word_values.append(word_value(word))

    max_value = max(word_values)
    index_of_max = word_values.index(max_value)

    return word_list[index_of_max]

The solution passed the test suite but I think I can improve the part where I store all the letters matched with their value. Is there any suggestions for this and in general the entire code?

Thank you.

\$\endgroup\$
  • \$\begingroup\$ TXR Lisp: (defun highest-score-word (wordlist) (find-max wordlist : (opip (mapcar (lop - #`)) (apply +)))) \$\endgroup\$ – Kaz Aug 30 at 7:47
10
\$\begingroup\$

Indeed, the values dictionary looks unnecessary. Using a built-in function ord you could compute the letter score with ord(letter) - ord('a') + 1. One may argue that it is even faster than a dictionary lookup, but in this case the timing difference is rather marginal.


With Python you should avoid rolling explicit loops. For example, collecting the word values is more idiomatically expressed as a comprehension:

    word_values = [word_value(word) for word in word_list]

Similarly, instead of

    for letter in input_word:
        value += values[letter]

consider functools.reduce:

    value = functools.reduce(lambda x, y: x + ord(y) - ord('a') + 1, input_word, 0)
\$\endgroup\$
  • 10
    \$\begingroup\$ Wouldn't using sum with a generator expression be more clear than reduce? \$\endgroup\$ – JAD Aug 30 at 8:44
  • 4
    \$\begingroup\$ I fail to see how reduce is more clear than the for loop, what makes it better? \$\endgroup\$ – jmarkmurphy Aug 30 at 14:26
9
\$\begingroup\$

Welcome to code review, good job as your first program I suppose.

Style

Docstrings: Python documentation strings (or docstrings) provide a convenient way of associating documentation with Python modules, functions, classes, and methods. An object's docstring is defined by including a string constant as the first statement in the object's definition.

def word_value(input_word):
def high(x):

You should include a docstring with your functions indicating what they do specially the names are pretty ambiguous ...

def get_word_value(word):
    """Return word letter score."""

def get_highest_scoring_word(word_list):
    """Return highest scoring word."""

Blank lines: I suggest you check PEP0008 https://www.python.org/dev/peps/pep-0008/ the official Python style guide and regarding blank lines, use blank lines sparingly to separate logical sections inside a function (too many blank lines in your high function)

word_values = []
for word in word_list:
    word_values.append(word_value(word))

max_value = max(word_values)
index_of_max = word_values.index(max_value)

return word_list[index_of_max]

Code

First function

can be shortened in the following way:

import string
values = dict(zip(string.ascii_lowercase, range(1, 27)))

def get_value(word):
    """Return word letter score."""
    return sum(values[letter] for letter in word)

Second function:

word_list = x.split(" ")

split() has a default value of " " so no need to specify

word_list = x.split()

does the same functionality

Descriptive names:

word_list is extremely confusing, because word_list is not a list so I suggest changing the name to word_sequence

Comprehension syntax:

word_values = []
for word in word_list:
    word_values.append(word_value(word))

this can be enclosed in a comprehension syntax (it's more efficient and shorter)

word_values = [word_value(word) for word in word_sequence]

Code might look like:

import string
values = dict(zip(string.ascii_lowercase, range(1, 27)))


def get_value(word):
    """Return word letter score."""
    return sum(values[letter] for letter in word)


def get_highest(word_sequence):
    """
    Return word with the highest score assuming word_sequence a 
    string of words separated by space.
    """
    return max(word_sequence.split(), key=get_value)


if __name__ == '__main__':
    words = 'abc def ggg'
    print(get_highest(words))
\$\endgroup\$
  • \$\begingroup\$ Thank you for your response. That is some beautiful code you wrote. \$\endgroup\$ – King Cold Aug 29 at 20:55
  • \$\begingroup\$ You're welcome man, I'm not an expert though \$\endgroup\$ – user203258 Aug 29 at 20:57
  • \$\begingroup\$ You are however re-creating the values-dict for every word you check. (and you don't need this dict at all if you just use ord(_)) \$\endgroup\$ – FooBar Aug 30 at 9:54
  • \$\begingroup\$ @ FooBar does not mean this is the wrong answer and unless the word list has a 7 figure length, efficiency is negligible and this is much more readable than the creation of complicated ord() formulas or making the dictionary a global variable which is not the best practice ever. \$\endgroup\$ – user203258 Aug 30 at 10:36
  • 2
    \$\begingroup\$ If you would like to stick with the dict-version, using {letter: i for i, letter in enumerate(string.ascii_lowercase, 1)} could be a more readable approach. To compensate what @FooBar said, you could make it into a global variable. \$\endgroup\$ – AlexV Aug 30 at 11:01
0
\$\begingroup\$

If performance isn't your main concern then consider the following approach of looking up the index of each character in an alphabet string. This will be slower than the other methods suggested, but provides greater readability.

def word_value(word):
      alphabet = 'abcdefghijklmnopqrstuvwxyz'
      return sum(alphabet.index(char) + 1 for char in word)

def highest_valued_word(phrase):
    return max(
      (word for word in phrase.split(' ')),
      # Use the `word_value` function to select the maximum.
      key=word_value
    )
\$\endgroup\$
  • \$\begingroup\$ The +1 is strictly necessary. Consider the score of 'round' verses 'around'. The latter would should be clearly be higher than the former, but without the +1, you treat the letter 'a' as worthless, and they compare as equal! \$\endgroup\$ – AJNeufeld Sep 17 at 22:50
  • \$\begingroup\$ @AJNeufeld Thanks, hadn't considered that case! \$\endgroup\$ – rob.earwaker Sep 18 at 5:01
0
\$\begingroup\$

If you really want to go for simplicity, you might want to try a one-liner like this:

def highest_scoring_word(words):
    return max(words.split(), key=lambda word: sum(map(ord, word)) - 96*len(word))
\$\endgroup\$
  • \$\begingroup\$ Your "one-liner" is far too long. Instead of a for-loop over the word characters, try a mapping: max(words.split(), key=lambda word: sum(map(ord, word)) - 96*len(word)) \$\endgroup\$ – AJNeufeld Sep 17 at 22:40

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.