10
\$\begingroup\$

It seems like I'm repeating myself here. Is there a standard way to deal with code like this?

def getNameParts(name):
    """Return a dictionary of a name's parts.
    name: e.g. 'JOHNSON, John Roberts, Jr. (Jack)'
    Ignores nicknames.
    """
    csSplit = name.split(', ')
    try:
        last = csSplit[0]
    except:
        last = ''
    try:
        first = csSplit[1].split(' ')[0]
    except:
        first = ''
    try:
        middle = ' '.join(csSplit[1].split(' ')[1:])
    except:
        middle = ''
    try:
        suffix = csSplit[2].split(' ')[0]
    except:
        suffix = ''
    partsDict = {'first': first,
                 'last': last,
                 'middle': middle,
                 'suffix': suffix}
    return(partsDict)
\$\endgroup\$
5
  • 1
    \$\begingroup\$ I don't see the logic. It is guaranteed that csSplit[0] will never throw an error because a split, even on an empty string, will still have at least one item in the result. What are all these try blocks guarding against? \$\endgroup\$
    – zondo
    Commented Jul 25, 2016 at 0:49
  • \$\begingroup\$ @zondo the list indexes are out of range for [1] and [2]. I guess I can remove the try/except for last. \$\endgroup\$
    – Hatshepsut
    Commented Jul 25, 2016 at 0:59
  • \$\begingroup\$ Is your function supposed to work for 'John Roberts, Jr.'? \$\endgroup\$
    – zondo
    Commented Jul 25, 2016 at 1:04
  • \$\begingroup\$ @zondo No, it just needs to work for strings in the format I've written in the docstring. \$\endgroup\$
    – Hatshepsut
    Commented Jul 25, 2016 at 1:05
  • 2
    \$\begingroup\$ Just in case you are ever going to use this in production, be sure to read kalzumeus.com/2010/06/17/… first. \$\endgroup\$
    – Gilad Naor
    Commented Jul 28, 2016 at 12:41

2 Answers 2

9
\$\begingroup\$

You have some unnecessary try blocks. Watch this:

>>> 'this'.split(', ')
['this']
>>> ''.split(', ')
['']

Look, Ma! No empty lists! Since a split will always have at least one item in the result, you don't need to try to get the first item. It will always be there for you. Watch this:

>>> [][1:]
[]
>>> ' '.join([])
''

Look, Ma! No errors! A slice never throws errors, and a ' '.join() will return '' when the argument is empty. That means that you need only one try block for first and middle.


An except block should specify what it is expecting. A bare except is dangerous. I have found myself caught when I have done that because I can't even interrupt the script with Ctrl+C. If it isn't an IndexError that trips the except, we want to know about it. We don't want to hide it under an except.


Your naming does not comply with PEP 8 (the Python style guide). It says to use lowercase_with_underscores for function and variable names. Besides that, csSplit is a little hard to understand. Sure, one can recognize cs as being short for comma-separated if one thinks about it, but I prefer something a little bit easier to understand. Keeping the same spirit, why not comma_split? I don't really like that name, but it seems a little easier to understand that csSplit.


It doesn't make much difference with short strings, but first and middle both perform the same split operation. To speed things up, make the split only once by assigning a variable to it.


Why the intermediate variable partsDict? We know that the function returns a dictionary of parts by its name and doc string. Therefore, the variable cannot be for clarity. It is longer to use than a simple return, so it isn't for line length. Why then? Just return directly.


The code ends up looking like this:

def get_name_parts(name):
    """Return a dictionary of a name's parts.
    name: e.g. 'JOHNSON, John Roberts, Jr. (Jack)'
    Ignores nicknames.
    """
    comma_split = name.split(', ')
    last = comma_split[0]

    try:
        first_mid = comma_split[1].split(' ')
    except IndexError:
        first_mid = ['']

    first = first_mid[0]
    middle = ' '.join(first_mid[1:])

    try:
        suffix = comma_split[2].split(' ')[0]
    except IndexError:
        suffix = ''

    return {'first': first,
            'last': last,
            'middle': middle,
            'suffix': suffix}

Is it shorter? Not really. It is only two or three lines shorter, but it is clearer, less repetitive, and safer. Happy coding!

\$\endgroup\$
1
  • \$\begingroup\$ I am especially glad to hear about the error free string indexing. I also recently got stuck in a bare exception, not able to escape with Ctrl+c, so I know exactly what you mean - way better to avoid that. I also learned that you can get out of it if needed by sending SIGQUIT with `Ctrl+\`, at least in many terminals. \$\endgroup\$
    – Hatshepsut
    Commented Jul 25, 2016 at 19:52
7
\$\begingroup\$

By PEP 8, function names should be lower_case_with_underscores. In my opinion, the "get" could (and therefore should) be dropped, since the function isn't really retrieving existing data.

The docstring would be clearer and more useful as a doctest.

It's hard to keep track of which string contains what, with all that splitting. String analysis is usually best done using regular expressions. The whole function could be a one-liner, using named capture groups.

import re

def name_parts(name):
    """
    Return a dictionary of a name's parts, ignoring nicknames.

    >>> sorted(name_parts('JOHNSON, John Roberts, Jr. (Jack)').items())
    [('first', 'John'), ('last', 'JOHNSON'), ('middle', 'Roberts'), ('suffix', 'Jr.')]
    >>> sorted(name_parts('JOHNSON, John').items())
    [('first', 'John'), ('last', 'JOHNSON'), ('middle', ''), ('suffix', '')]
    >>> sorted(name_parts('JOHNSON').items())
    [('first', ''), ('last', 'JOHNSON'), ('middle', ''), ('suffix', '')]
    """
    # This match always succeeds, since all components are optional
    return re.match(
        '(?P<last>[^,]*),?\s*'
        '(?P<first>[^ ]*)\s*'
        '(?P<middle>[^,]*),?\s*'
        '(?P<suffix>[^ ]*)',
        name
    ).groupdict()
\$\endgroup\$
2
  • \$\begingroup\$ >>> sorted(name_parts('JOHNSON, John, Jr.').items()) [('first', 'John'), ('last', 'JOHNSON'), ('middle', ''), ('suffix', 'Jr.')] \$\endgroup\$
    – Hatshepsut
    Commented Sep 2, 2016 at 11:13
  • \$\begingroup\$ '(?P<first>[^ ,]*)\s*' comma exclusion is needed \$\endgroup\$
    – Hatshepsut
    Commented Sep 2, 2016 at 11:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.