4
\$\begingroup\$

The first piece of code should cover the standard expected data. But as you can easily see, the data not always conveys to this standard. The code works great but I wonder if this is thought be a miss-use of the try/except clause. Better to go with if/else?

try:
    short_name = short_name.upper()
    m = (k for k,v in c.countries_dict.iteritems() if v[0] == short_name).next() 
    m = [m]
except StopIteration:
    if re_type == 'MAP_INFO' or re_type == 'SHAPE' or re_type == 'ARC_INFO':
        reg_dict = REGIONS
        short_name = short_name.lower()
    elif re_type == 'NAV_SHAPE':
        reg_dict = NAV_REGIONS
        short_name = short_name.upper()
    m = []
    for k,v in reg_dict.iteritems():
        if k == short_name:
            for tup in v:
                m.append(tup[1])
\$\endgroup\$
3
  • 2
    \$\begingroup\$ Can you provide something that can be executed/tested properly and/or some information about the intended purpose. \$\endgroup\$
    – SylvainD
    Jan 24, 2014 at 16:44
  • \$\begingroup\$ Is the code underneath yielding bad data? Or is it using StopIteration to signal 'I'm done'? Or to indicate that some data source has gone offline? \$\endgroup\$
    – theodox
    Jan 24, 2014 at 23:46
  • \$\begingroup\$ @theodox I think it's to indicate the value was not found. \$\endgroup\$ Jan 25, 2014 at 4:19

1 Answer 1

6
\$\begingroup\$

I would discourage this particular usage. Not exactly because it's a risk or a hack—in other circumstances it might even be the right choice for good performance—but because I find it quite obfuscatory. (This happens to echo the usual guidance to avoid using exceptions for flow control, although it's harder to apply that guidance around Python's iterators.)

Instead I would suggest trying to make it clear what you are doing. I believe in this case you are trying to find the "first" key that maps to a list of values containing short_name in index 0. (I say "first" because it's in dictionary order, which isn't a particularly meaningful; it's probably more useful to say an arbitrary key.) If such a key does not exist, you wish to fall through to your except case. A reader is going to find that motive hard to discern. Here is an alternative way to write the first couple lines using itertools:

from itertools import islice

short_name = short_name.upper()
# find a key of an item containing short_name, if available
m = list(islice((k for k,v in c.countries_dict.iteritems() if v[0] == short_name), 1))
if not m:
    # due to bad data, not all short_name values are mapped correctly; try another way
    # code from original except StopIteration

For bonus readability you might split that up into a named generator function that finds all matches, and then filter to the first match with islice or another helper. Clearly performance isn't your goal here, as you're scanning a dict by value. (If you have a lot of these to look up, you may want to create the reverse dict instead of scanning multiple times.)

I'm far from certain that this code is what you intended. Since in the fallback case you build up a list of potentially multiple values, it seems surprising that in the primary case you explicitly exclude alternate keys. If you wanted to include all keys, you can skip islice and use a simple list comprehension: (Note that by slicing the list comprehension with [:1], it would yield the same value for m as the previous approach, and is more readable, although it potentially does more work.)

short_name = short_name.upper()
m = [k for k,v in c.countries_dict.iteritems() if v[0] == short_name]
if not m:
    # due to bad data, not all short_name values are mapped correctly; try another way
    # code from original except StopIteration
\$\endgroup\$
3
  • \$\begingroup\$ Rather than islice with 1, I'd use the default argument to next, and write m = next((k for k,v in c.countries_dict.iteritems() if v[0] == short_name), None). \$\endgroup\$
    – DSM
    Jan 25, 2014 at 7:16
  • \$\begingroup\$ @DSM Thanks for bringing this up. I agree next(iterable, default) itself is a better way to write the operation, but it seems to communicate a subtly different intent than the "give me at most one" of islice(iterable, 1). Since my review focused on communicating intent, I went with the islice. \$\endgroup\$ Jan 25, 2014 at 14:02
  • \$\begingroup\$ Thanks @MichaelUrman, very helpful answer. It is true that the generator should yield exactly one value from the dict. That is the default. But there are other data sources that won't have corresponding key/value pair in c.countries_dict. I agree that this is somewhat obfuscatory. I appreciate your thoughts on the matter. \$\endgroup\$
    – LarsVegas
    Jan 26, 2014 at 9:44

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