9
\$\begingroup\$

I wrote this code to filter a list of names based on certain conditions, and wanted to see if I can reduce this further and eliminate redundancy in code.

names1 = ["Jane", "Jake", "Bradley", "Bill", "Betty", "Kara", "Kris", "Jil"]
names2 = ["George", "Kate", "Karen", "Kurt", "Greg", "Gary"]
selection_criteria = ["full_list", "four_characters", "three_characters", "start_with_k", "start_with_z"]

def sublist (name_list, condition):
    return_list = []
    if condition == "full_list":
        return name_list
    if condition == "four_characters":
        for name in name_list:
            if len(name) == 4:
                return_list.append(name)
    if condition == "three_characters":
        for name in name_list:
            if len(name) == 3:
                return_list.append(name)
    if condition == "start_with_k":
        for name in name_list:
            if name[0] == 'K':
                return_list.append(name)
    if condition == "start_with_z":
        for name in name_list:
            if name[0] == 'Z':
                return_list.append(name)
    return return_list

for criteria in selection_criteria:
    print(sublist(names1, criteria))

for criteria in selection_criteria:
    print(sublist(names2, criteria))
\$\endgroup\$
  • \$\begingroup\$ Do you know about dicts and regexes? Look them up \$\endgroup\$ – Tamoghna Chowdhury Nov 28 '16 at 4:19
  • 4
    \$\begingroup\$ What is the real task that you are trying to accomplish? Why not define five separate functions instead of one function with five independent modes of operation? \$\endgroup\$ – 200_success Nov 28 '16 at 6:55
  • \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Nov 28 '16 at 15:11
2
\$\begingroup\$

Overall your code looks fairly clean and well written. Naming conventions are consistent, and your names are clear and spelled correctly. Good job.

  • While your naming conventions are consistent, and most of your variable names are good, a few could use some work. for example, the list return_list in the function sublist() could be improved. Don't name a variable for what data type it is, or what its suppose to do. Name the variable for what it is representing. Because a variable is just that. Instead of return_list you should name it something such as filtered_names.

    Also while on the topic of naming, I should mention that criterion is the singular form for criteria. So if you wanted you could be more specfic, you could considered using it.

  • Use elif statements instead of multiple if statements. When Python is reading your source code, the virtual machine which runs Python must check every if statement to test its validity, which takes time. If you make use of elif statements instead, as soon as one elif statement is true, Python can jump straight to the return statement in your function. It also adds the benefit of making your code more explicit.

  • Now is a good time to suggest a better name for your function. When creating names for functions give them meaningful names. I should just be able to read your function name and have a good idea of what your function is suppose to accomplish. Instead of sublist, which really doesn't convey much meaning to me, use filter_names_by_criteria() or even just filter_by_criteria(). Also, Instead of using name_list, condition for your parameters, I would use names, criterion

  • In programming, there is a commonly used principal called Don't Repeat Yourself(DRY). The principal is often useful to apply to most every program. In your case look here:

    ...
    for criteria in selection_criteria:
        print(sublist(names1, criteria))
    
    for criteria in selection_criteria:
        print(sublist(names2, criteria))
    

    Here you running two, practically identical for loops. Instead, extract this logic to a function:

    def print_filtered_names(names, criteria):
        for criterion in criteria:
            print(sublist(names, criterion))
    
    print_filtered_names(names1, selection_criteria)
    ...
    

If you've followed along, and implemented my improvements your code should look something like:

names1 = ["Jane", "Jake", "Bradley", "Bill", "Betty", "Kara", "Kris", "Jil"]
names2 = ["George", "Kate", "Karen", "Kurt", "Greg", "Gary"]
selection_criteria = ["full_list", "four_characters", "three_characters", 
"start_with_k", "start_with_z"]

def filter_names_by_criteria(names, criterion):
    if criterion == "full_list":
        return names
    # don't create a list without knowing if you'll need one 
    filtered_names = []
    if criterion == "four_characters":
        for name in names:
            if len(name) == 4:
                filtered_names.append(name)
    elif criterion == "three_characters":
        for name in names:
            if len(name) == 3:
                filtered_names.append(name)
    elif criterion == "start_with_k":
        for name in names:
            if name[0] == 'K':
                filtered_names.append(name)
    elif criterion == "start_with_z":
        for name in names:
            if name[0] == 'Z':
                filtered_names.append(name)
    return filtered_names 

def print_filtered_names(names, criteria):
  for criterion in criteria:
    print("criterion: {}".format(criterion))
    print(filter_names_by_criteria(names, criterion))

print_filtered_names(names1, selection_criteria)
print_filtered_names(names2, selection_criteria)
\$\endgroup\$
  • \$\begingroup\$ "It also adds the benefit of making your code mr explicit." : should this be "It also adds the benefit of making your code more explicit." ? Or is mr some CS term I do not know about? (I'm French ^^) \$\endgroup\$ – Olivier Dulac Nov 28 '16 at 14:11
  • \$\begingroup\$ @OlivierDulac Nope. Just another typo. I meant more :) \$\endgroup\$ – Christian Dean Nov 28 '16 at 14:29
3
\$\begingroup\$

I would make this two different functions, one for filtering all names of length n and one for all names starting with some letter. I made them into generator comprehensions, so they are more memory efficient on their own (even though here I cast them to lists, as soon as I want to use them).

Then there is a function which is similar to your function, except that it is configurable. By default it just yields the original list. If additional n's or c's are passed, it applies these criteria.

Finally there is a print function, which has your concrete realization of n = 4, 3 and c = "K", "Z".

def n_chars(names, n):
    return (name for name in names if len(name) == n)

def starts_with(names, c):
    return (name for name in names if name.lower().startswith(c))

def print_sublists(names):
    print(names)
    print(list(n_chars(names, 4))
    print(list(n_chars(names, 3))
    print(list(starts_with(names, "k"))
    print(list(starts_with(names, "z"))

if __name__ == '__main__':
    names1 = ["Jane", "Jake", "Bradley", "Bill", "Betty", "Kara", "Kris", "Jil"]
    names2 = ["George", "Kate", "Karen", "Kurt", "Greg", "Gary"] 
    for names in names1, names2:
        print_sublists(names)
\$\endgroup\$
  • \$\begingroup\$ Not sure if I suppose to do that, but I will review your comment, while I found this one better than other answers, there are few things. parameter names like ns/cs could be better. Your n_chars, starts_with could be replaced with the simple built-in filter function. It's not clear why they are generators while in fact, you cast it to a list when you use it. Default values of ns/cs are strings, I know they are iterable but better make it empty tuple, because n_chars will not work with string. And maybe you want to add if name == 'main' block :) \$\endgroup\$ – Alex Nov 28 '16 at 11:01
  • \$\begingroup\$ @Alex agree on ns, cs, could not think of better names, agree on the built-ins, they are generators to be more widely applicable, but that is resolved by using the built-ins, which don't return lists in Python 3 either, The default values are strings because they need to be iterable (so there is no if ns is not None check) and mutable default arguments are potentially dangerous, even though here they would not be, Agree on __name__ == '__main__'. \$\endgroup\$ – Graipher Nov 28 '16 at 11:08
2
\$\begingroup\$

Some notes:

  • def sublist (. No space between identifier and parenthesis.
  • Better naming: sublist is not declarative enough; names_list: just use the plural form: names.
  • Use elif to write a switch construct.
  • Use list-comprehensions whenever possible instead of the imperative pattern empty+for+if+append.
  • name[0] == 'K': That will fail if name is empty. Use str.startswith.
  • Are you sure you want to silently accept an unknown condition name? If not you can raise a ValueError exception on that branch.

I'd write:

def filter_names(names, condition):
    if condition == "full_list":
        return names
    elif condition == "four_characters":
        return [name for name in names if len(name) == 4]
    elif condition == "three_characters":
        return [name for name in names if len(name) == 3]
    elif condition == "start_with_k":
        return [name for name in names if name.startswith("K")]
    elif condition == "start_with_z":
        return [name for name in names if name.startswith("Z")]
    else:
        return []
\$\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.