current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
1

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?

share|improve this question

3 Answers 3

up vote 2 down vote

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.

share|improve this answer
    
+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. – Winston Ewert Feb 4 '12 at 17:42
up vote 4 down vote

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
share|improve this answer
    
I think your answer would be better to include the numerical indexes as well as the string ones. – Winston Ewert Feb 4 '12 at 17:40
    
FWIW, PEP8 asks you to use if response is not None: – Wieland Feb 5 '12 at 15:42
    
@WielandH., this way you will get an AttributeError on the next line, if response will happen to be 0/[]/False/() – Misha Akovantsev Feb 10 '12 at 4:22
up vote 2 down vote

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.

share|improve this answer
1  
+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. – Paul Martel Feb 10 '12 at 3:47
    
@PaulMartel, excellent point! – Winston Ewert Feb 10 '12 at 3:51
    
@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()? – Misha Akovantsev Feb 10 '12 at 4:27
    
@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. – Winston Ewert Feb 10 '12 at 4:45
    
@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. – Winston Ewert Feb 10 '12 at 4:53

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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