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\$
  • 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 Jul 25 '16 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 Jul 25 '16 at 0:59
  • \$\begingroup\$ Is your function supposed to work for 'John Roberts, Jr.'? \$\endgroup\$ – zondo Jul 25 '16 at 1:04
  • \$\begingroup\$ @zondo No, it just needs to work for strings in the format I've written in the docstring. \$\endgroup\$ – Hatshepsut Jul 25 '16 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 Jul 28 '16 at 12:41
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\$
  • \$\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 Jul 25 '16 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\$
  • \$\begingroup\$ >>> sorted(name_parts('JOHNSON, John, Jr.').items()) [('first', 'John'), ('last', 'JOHNSON'), ('middle', ''), ('suffix', 'Jr.')] \$\endgroup\$ – Hatshepsut Sep 2 '16 at 11:13
  • \$\begingroup\$ '(?P<first>[^ ,]*)\s*' comma exclusion is needed \$\endgroup\$ – Hatshepsut Sep 2 '16 at 11:33

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.