4
\$\begingroup\$

I create python dictionary based on the parsed data:

user_details['city'] = None
if api_result.get('response')[1] and api_result.get('response')[1][0]:
    api_result_response = api_result.get('response')[1][0] # city details
    if api_result_response:
        user_details['city'] = int(api_result_response.get('cid'))
        city_name = api_result_response.get('name')
if user_details['city'] and city_name:
    # do something

So, I do a lot of verifications if passed data exists, if user_details['city'] assigned etc. Is there any way to simplify that?

\$\endgroup\$

closed as off-topic by Stephen Rauch, Sᴀᴍ Onᴇᴌᴀ, Dannnno, t3chb0t, Toby Speight Jun 5 '18 at 10:00

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Stephen Rauch, Sᴀᴍ Onᴇᴌᴀ, Dannnno, t3chb0t, Toby Speight
If this question can be reworded to fit the rules in the help center, please edit the question.

4
\$\begingroup\$

Another key feature to dictionaries is the dict.get(key, default) function which allows you to get a default value if the key doesn't exist. This allows you to make the code less cluttered with if statements and also try/except.

The solution is often a balance between if and try statements and default values. If you are checking correctness of values, keep the try/catch statements as small as possible.

    response = api_result.get('response', None)
    if response:
        city_name = response.get('name', '')
        try:
            user_details['city'] = int( reponse.get('cid', 0) )
        except ValueError, e:
            pass # report that the 'cid' value was bogus
\$\endgroup\$
  • \$\begingroup\$ I think your answer would be better to include the numerical indexes as well as the string ones. \$\endgroup\$ – Winston Ewert Feb 4 '12 at 17:40
  • \$\begingroup\$ FWIW, PEP8 asks you to use if response is not None: \$\endgroup\$ – Wieland Feb 5 '12 at 15:42
  • \$\begingroup\$ @WielandH., this way you will get an AttributeError on the next line, if response will happen to be 0/[]/False/() \$\endgroup\$ – Misha Akovantsev Feb 10 '12 at 4:22
2
\$\begingroup\$

You should just use try/except here:

try:
    response = api_result['response'][1][0]
    user_details['city'] = int(response['cid'])
    city_name = response['name']
except KeyError, IndexError:
    pass
else:
    # do something

The else on a try/except block is only executed if the try block ended normally, i.e. no exception was raised.

You may also want to catch 'ValueError' in case int() is passed an invalid value.

\$\endgroup\$
  • \$\begingroup\$ +1, but I'd note that you should be careful with this. If you call a function inside that block, it may raise a IndexError due to a bug, which will be hidden by this code. \$\endgroup\$ – Winston Ewert Feb 4 '12 at 17:42
2
\$\begingroup\$

I'd write a class that let me do this:

data = DataExtractor(api_result)
try:
    city = data.fetch('response', 1, 0, 'cid', int)
    city_name = data.fetch('response', 1, 0, 'city_name', str)
except DataExtractionError:
    print "Couldn't get the data!"

Then I'd just have to worry about checking the validity of incoming data in one place, DataExtractor. It will take care of verifying anything I throw at it, and will always throw the DataExtractionError if something doesn't line up. As Paul Martel points out, you should give the arguments of fetch to DataExtractionError so you can have a clear error message.

Another option to consider is using a json schema validator. Most of them can be used with objects that aren't actually JSON. Here is a link to PyPi which lists a few of them.

http://pypi.python.org/pypi?%3Aaction=search&term=json+schema&submit=search

The basic idea is that you tell it the structure of the data, and it can determine whether or not it actually fits. This can simplify your code because you once you've validated the data you can assume that the data's structure is correct and you don't have to do as much testing.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1, and DataExtractionError should contain the arguments passed to data.fetch and the exception condition (missing value or wrong type) so you can print what when wrong in the except block. \$\endgroup\$ – Paul Martel Feb 10 '12 at 3:47
  • \$\begingroup\$ @PaulMartel, excellent point! \$\endgroup\$ – Winston Ewert Feb 10 '12 at 3:51
  • \$\begingroup\$ @WinstonEwert, omg, what are all those arguments? How about going 1 step further and hiding them under the argumentless DataExtractor.get_city_id() and DataExtractor.get_city_name()? \$\endgroup\$ – Misha Akovantsev Feb 10 '12 at 4:27
  • \$\begingroup\$ @MishaAkovantsev, the argument specifies where in the data structure the piece of information you are interested lies. The op accesses api_result['response'][1][0]['cid'] and I'm suggesting that he should pass all those keys to a function which can handle the logic of safely extracting in one place. \$\endgroup\$ – Winston Ewert Feb 10 '12 at 4:45
  • \$\begingroup\$ @MishaAkovantsev, the arguments tell it where the data lives. DataExtractor is not specific to a single extraction task, but can be reused for any data structure you find yourself with. So having a get_city_id() function doesn't make any sense. \$\endgroup\$ – Winston Ewert Feb 10 '12 at 4:53

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