Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My python code is ugly and I'm having difficulty improving it. What kind of steps could be done here to make it. This is an example function that comes from time to time and I can't seem to have a good way to pick it up. The re.match for me is a mess. The creation of variables that have negative values like 'created = False' 'option_id = None' makes me shiver at night, also how to avoid the 'if value:' that pushes all the function to the right. I'm in need of a zen teaching:

def get_id(string):
    try:
        match = re.match('^[\w_]+?_(\d+)$', string).groups()
    except:
        return None
    return match[0]

def admin_add_detail_option(request, detail_id, value):
    created = False
    option_id = None
    if value:
        detail_id = get_id(detail_id)
        try:
            detail = Detail.objects.get(id=detail_id)
        except Detail.DoesNotExist:
            pass
        else:
            option, created = DetailOption.objects.get_or_create(name=value, detail=detail)
            option_id = option.id
    return simplejson.dumps({'created':created, 'id':option_id, 'name':detail_id, 'text':value})

Thank you

After reading Bruno post this was re-factored to:

def get_id(string):
    match = re.match('^[\w_]+?_(\d+)$', string)
    if match
        return match.groups()[0]
    return None

def admin_add_detail_option(label, text):
    result = {'created':False, 'id':None, 'name':label, 'text':text}
    if not text:
        return simplejson.dumps(result)
    detail_id = get_id(label)
    try:
        option, created = DetailOption.objects.get_or_create(name=text, detail__id=detail_id)
    except Detail.DoesNotExist:
        pass
    else:
        result.update({'created':created, 'id':option_id})
    return simplejson.dumps(result)

I hope I understood all of your point has they have been very helpful.

Thank you.

share|improve this question

1 Answer

up vote 5 down vote accepted

wrt/ get_id, the bare except clause is a definitive no no unless the exception is re-raised (and even then...). The cleanest way to write this function is, as often in Python, the more straightforward one:

def get_id(string):
    match = re.match('^[\w_]+?_(\d+)$', string)
    if match:
        return match.groups()[0]
    return None

It's simpler, easier to understand, and as an added bonus it won't silence other possible errors (like a TypeError if you pass None as argument).

Also having to post-process an argument named "detail_id" to really (and eventually) get the effective detail_id is a code smell to me - it should be the caller's responsibility to provide the correct detail_id.

It's not clear to me whether admin_add_detail_option is a view function or an helper for a view function.

If it's a view function (in which case I assume it's decorated somewhere to return a proper response object) then I wonder why you don't rewrite your url pattern so you only get the effective detail_id. Also in that case it should only accept POST requests (a GET request is supposed to be idempotent), and the "value" should come from the post body, not the URL.

Else - if it's an helper function called by the view - then you should get rid of the unused request parameter and make the caller responsible for providing a correct detail_id - as well as a non-empty value which is the simplest way to get rid of the if value: test FWIW. Also if it's a helper function it should return a plain Python dict so that the caller can inspect / modify the result if and when necessary.

In both cases (view or helper) and assuming DetailOption as a non-null foreign key on Detail, you don't need to load the Detail object from the database:

try:
    option, created = DetailOption.objects.get_or_create(
        name=value, 
        detail_id=detail_id
        )
    option_id = option.id
except Detail.DoesNotExist:
    # assume created and option_id are already set
    pass

Naming could also be improved a bit: you have a parameter named value that is used for the name field of DetailOption but returned under the text field in the JSON object, which itself has a name field that holds the detail_id. Polysemy leads to confusion and confusion leads to stupid logical errors and wasted time.

wrt/ avoiding the if value: test, if you cannot avoid it (cf above), you can at least invert the test and return early:

if not value:
    return <some_default_value>
# now we can proceed

Early returns are very pythonic : first get rid of the corner cases, then do the job - if you still have a job a to do.

Using this pattern often means having a decent default return value at hand (if your function is not supposed to return None). When a function has to conditionally set names that are unconditionally used, it's good practice to first define these names with appropriate default values (which is what you did). It avoids NameErrors, simplifies the logic and makes obvious you are going to use these names somewhere (and what are the default values).

This - with the early return - is a very common coding pattern in Python.

share|improve this answer
match.groups()[0]match.group(0) – Gareth Rees Apr 14 at 20:22

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.