5
\$\begingroup\$

I want to create a function that filters a list with the following criteria:

  1. Strings containing numbers should be removed
  2. Strings containing only one word and first letter of the word is not uppercase should also be removed.
  3. Strings containing more than one word and first letter of each words except first word is not uppercase should also be removed.

The amount of code I am using for this task seems excessive. Can I produce the same results with something more compact? If yes, how?

def list_filter(str_list):
    filtered = []
    is_end = False
    for string in str_list:
        words = string.split(' ')
        for i, word in enumerate(words):
            if i > 0:
                if word[0].isupper():
                    continue
                else:
                    is_end = True
                    break
            if word.isdigit():
                is_end = True
                break
            if len(words) == 1 and not word[0].isupper():
                is_end = True
                break
        if is_end:
            break
        else:
            filtered.append(string)
    return filtered
\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review! As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – 301_Moved_Permanently Jun 29 '16 at 12:42
  • 2
    \$\begingroup\$ Whilst refactoring, it became apparent that your code only checks if the first letter is uppercase instead of the whole word. You should edit your description of the algorithm and the code so that they match each other in actual execution, otherwise you have a bug. \$\endgroup\$ – phyrfox Jun 29 '16 at 14:00
  • 2
    \$\begingroup\$ Also, you check if an the entire string contains nothing but digits, rather than whether there is any digit anywhere in the string as your description says. \$\endgroup\$ – TheBlackCat Jun 29 '16 at 15:07
11
\$\begingroup\$

I took the liberty of rewriting most of this from scratch for a few reasons.

First, this very difficult to actually test. All your embedded if statements... oh my! Second it's also difficult to add new checks into if you want other filter conditions.

Code like your original code starts out well intentioned but becomes a giant spaghetti mess fairly quickly - you only have three conditions and it's already a complex and hard to read and quickly understand function.

I moved each conditional check into a separate, small function:

# Convention: returns true if should be removed, based on criteria
def contains_numbers(s):
    return any(char.isdigit() for char in s)

Any is a great use for things like this. It will return True if any condition in the list matches.

def single_word_is_not_all_caps(s):
    if len(s.split(' ')) == 1:
        return s.upper() != s
    return False

def contains_multiple_words_not_uppercase(s):
    words = s.split(' ')
    if len(words) > 1:
        return words[0][0].upper() != words[0][0]
    return False

Fairly straightforward here. Both these test specific conditions that can be optimized a bit better in single functions.

This can streamline your main logic significantly (using any again):

 filters = [contains_numbers,
       contains_multiple_words_not_uppercase,
       single_word_is_not_all_caps]

def filter_strings(strs):

    filtered = []
    for s in strs:
        if not any(foo(s) for foo in filters):
            filtered.append(s)

    return filtered

Notice how easy it is now to add new check conditions, you just need to make/test a small function and add it to your filter list. Then iterate over that list, checking if any of them return true for the current string.

You could make this a list comprehension and even simpler if you want:

def filter_strings_list_comp(strs):
    return [s for s in strs if not any(f(s) for f in filters)]

Adding new logical checks is suddenly trivial. Changing existing ones, similarly. With one main "if tree to end all if trees" you will over time end up with a complete mess.

This approach also has the added feature/benefit of making testing much more trivial (I used basic assert statements here):

assert(True == contains_multiple_words_not_uppercase('foo bar'))
assert(False == contains_multiple_words_not_uppercase('Foo bar'))
assert(False == contains_multiple_words_not_uppercase('Foo'))

assert(True == contains_numbers('123'))
assert(True == contains_numbers('1asdf23'))
assert(False == contains_numbers('Foo bar'))
assert(False == contains_numbers(''))

assert(False == single_word_is_not_all_caps('FOO'))
assert(False == single_word_is_not_all_caps('foo bar'))
assert(False == single_word_is_not_all_caps('Foo bar'))
assert(True == single_word_is_not_all_caps('Foo'))


assert(['FOO'] == filter_strings(['Foo', 'FOO']))
assert(['BAR', 'Foo bar'] == filter_strings(['Foo', 'foo', 'BAR', 'Foo bar', '123']))

assert(['FOO'] == filter_strings_list_comp(['Foo', 'FOO']))
assert(['BAR', 'Foo bar'] == filter_strings_list_comp(['Foo', 'foo', 'BAR', 'Foo bar', '123']))

This is a lot easier to isolate problems, particularly if this ends up with many filters.

I would recommend adding more tests here, there are conditions I'm not sure how you want to test or what they should return.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ In filtered_strings you may use the built-in filter instead of manual iteration; other than this, Good Answer :) \$\endgroup\$ – Caridorc Jun 29 '16 at 17:15
  • \$\begingroup\$ @Caridorc might as well just use the list comprehension and cut out more lines :-) \$\endgroup\$ – enderland Jun 29 '16 at 17:15
  • \$\begingroup\$ Yes, list comprehension is even more readable than filter (filter should really be named select like in Ruby, its current name makes you think that it filters out the values satisfiyng the condition while it keeps them: the opposite) \$\endgroup\$ – Caridorc Jun 29 '16 at 17:21

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.