3
\$\begingroup\$

This function's goal is to reduce the length of a sentence to exactly max_length characters by cutting each word in the sentence to a length of minimum 4 characters, if cutting each word to 4 characters isn't enough the the sentence is returned anyway.
All sentences are free of special characters and words are separated by spaces.
Here is the function:

def cut_everything(sentence, max_length):
    """
    reduces each word in sentence to a length of 4

    :type sentence: string
    :param sentence: the sentence to cut
    :type max_length: int
    :param max_length: the length to which the sentence will be reduced
    """
    words = sentence.split()
    for index, word in enumerate(words):
        word_length = len(word)
        if word_length > 4:
            to_cut = len(sentence) - max_length
            to_keep = word_length - to_cut
            if to_keep < 4:
                to_keep = 4
            words[index] = word[:to_keep]
            sentence = ' '.join(words)
            if len(sentence) <= max_length:
                break

    return sentence

My main concern for this review is performance, but any readability comment is appreciated

\$\endgroup\$
2
\$\begingroup\$

Review

  • Magic numbers

    4 is a magic number, it is best to assign these to a variable name, that way it is more clear what this number means

    numbers don't have names, variables do

    MAX_WORD_LENGHT = 4
    
  • Use built-ins when possible

    to_keep = word_length - to_cut
    if to_keep < 4:
    to_keep = 4
    

    Can be replaced with the max builtin

     to_keep = max(word_length - to_cut, 4)
    
  • Add tests

    That way it becomes easy to check after a change if the function still works

Alternative

I went a slightly different route,

Instead of joining after each word, I calculate the chars we need to cut beforehand

So we can keep a variable that will hold the amount of chars we still need to cut to reach our target

And only at the return join the words

import doctest

MAX_WORD_LENGHT = 4

def cut_everything(sentence, max_length):
    """
    reduces each word in sentence to a length of 4

    :type sentence: string
    :param sentence: the sentence to cut
    :type max_length: int
    :param max_length: the length to which the sentence will be reduced

    >>> cut_everything('foo bar foooobar', 16)
    'foo bar foooobar'

    >>> cut_everything('foo bar foooobar', 8)
    'foo bar fooo'

    >>> cut_everything('foo bar foooobar baaarfoo', 20)
    'foo bar fooo baaarfo'

    >>> cut_everything('fooooooo baaaaaaar foooobar baaarfoo', 2)
    'fooo baaa fooo baaa'
    """
    words = sentence.split()
    chars_to_cut = len(sentence) - max_length
    for index, word in enumerate(words):
        if chars_to_cut < 0:
            break
        word_length = len(word)
        if word_length > MAX_WORD_LENGHT:
            to_keep = max(word_length - chars_to_cut, MAX_WORD_LENGHT)
            words[index] = word[:to_keep]
            chars_to_cut -= word_length - to_keep
    return ' '.join(words)

if __name__ == '__main__':
    doctest.testmod()
\$\endgroup\$
  • \$\begingroup\$ do you think it is useful to put use cases in the docstring or is it just for the answer to be clearer ? Also would using a parameter with a default value be more efficient than a global variable for the "magic number situation" ? \$\endgroup\$ – Comte_Zero Dec 17 '18 at 10:59
  • \$\begingroup\$ I have used the docstrings to add a test-suite with the doctest module \$\endgroup\$ – Ludisposed Dec 17 '18 at 11:01
  • \$\begingroup\$ Both could work, but depending on the situation. If the max_word_length can be changed you could add it as a parameter. But if the max_word_length is static ie cannot be changed add it as a constant. \$\endgroup\$ – Ludisposed Dec 17 '18 at 11:04

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.