HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythondjangoMinor

How would you rate this python code? (Django AJAX)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisajaxdjangoyouratewouldpythonhowcode

Problem

This code comes straight out from the example code in a Django book. I find it quite bad, mainly because the use of flags (if ajax then again if ajax) and unnecessarily bigger variable scope (set title='', tags='' and then use these variables in different flows). The function body also seems way too long.

I guess I'll subtract the book title (for the record I find the book in general good).

How would you rate this code, say out of 10 (10=very good, 3 and below being unacceptable)? (I'd rate it a 3)

The reason I ask is, I had rejected similar code in the past during code review and now I'm wondering whether I've been too strict or didn't understand python culture (I'm new to python).

```
@login_required
def bookmark_save_page(request):
ajax = 'ajax' in request.GET
if request.method == 'POST':
form = BookmarkSaveForm(request.POST)
if form.is_valid():
bookmark = _bookmark_save(request, form)
if ajax:
variables = RequestContext(request, {
'bookmarks': [bookmark],
'show_edit': True,
'show_tags': True
})
return render_to_response(
'bookmark_list.html', variables
)
else:
return HttpResponseRedirect(
'/user/%s/' % request.user.username
)
else:
if ajax:
return HttpResponse(u'failure')
elif 'url' in request.GET:
url = request.GET['url']
title = ''
tags = ''
try:
link = Link.objects.get(url=url)
bookmark = Bookmark.objects.get(
link=link,
user=request.user
)
title = bookmark.title
tags = ' '.join(
tag.name for tag in bookmark.tag_set.all()
)
except (Link.DoesNotExist, Bookmark.DoesNotExist):
pass
form = BookmarkSaveForm({
'url': url,
'title': title,
'tags': tags
})
else:
form = BookmarkSaveForm()
variables = RequestContext(request, {
'form': form
})
if ajax:
return render_to_response(

Solution

In my opinion it's little bit hard to read. You have to split it up into POST/GET methods.
Then you have to clean up code in POST/GET methods.
Something like this, for example.

@login_required
def bookmark_save(request):
    form = BookmarkSaveForm()
    if request.method == 'POST':
        form = bookmark_POST(request):
        if isinstance(form, HttpResponse):
            return form
    elif 'url' in request.GET:
        form = bookmark_GET(request)

    variables = RequestContext(request, {'form': form})

    if 'ajax' in request.GET:
        template_name = 'bookmark_save_form.html'
    else:
        temaplte_name = 'bookmark_save.html'
    return render_to_response(temaplte_name, variables)


And functions for POST/GET actions

def bookmark_POST(request):
    form = BookmarkSaveForm(request.POST)
    if form.is_valid():
        bookmark = _bookmark_save(request, form)
        if 'ajax' in request.GET:
            variables = RequestContext(request, {
                'bookmarks': [bookmark],
                'show_edit': True,
                'show_tags': True
            })
            return render_to_response(
                'bookmark_list.html', variables
            )
        else:
            return HttpResponseRedirect(
                '/user/%s/' % request.user.username
            )
    else:
        if 'ajax' in request.GET:
            return HttpResponse(u'failure')
    return form

def bookmark_GET(request):
    url = request.GET['url']
    try:
        link = Link.objects.get(url=url)
        bookmark = Bookmark.objects.get(
            link=link,
            user=request.user
        )
        title = bookmark.title
        tags = ' '.join(
            bookmark.tag_set.all().\
            values_list('name', flat=True)
        )
    except (Link.DoesNotExist, Bookmark.DoesNotExist):
        title = ''
        tags = ''
    form = BookmarkSaveForm({
        'url': url,
        'title': title,
        'tags': tags
    })
    return form

Code Snippets

@login_required
def bookmark_save(request):
    form = BookmarkSaveForm()
    if request.method == 'POST':
        form = bookmark_POST(request):
        if isinstance(form, HttpResponse):
            return form
    elif 'url' in request.GET:
        form = bookmark_GET(request)

    variables = RequestContext(request, {'form': form})

    if 'ajax' in request.GET:
        template_name = 'bookmark_save_form.html'
    else:
        temaplte_name = 'bookmark_save.html'
    return render_to_response(temaplte_name, variables)
def bookmark_POST(request):
    form = BookmarkSaveForm(request.POST)
    if form.is_valid():
        bookmark = _bookmark_save(request, form)
        if 'ajax' in request.GET:
            variables = RequestContext(request, {
                'bookmarks': [bookmark],
                'show_edit': True,
                'show_tags': True
            })
            return render_to_response(
                'bookmark_list.html', variables
            )
        else:
            return HttpResponseRedirect(
                '/user/%s/' % request.user.username
            )
    else:
        if 'ajax' in request.GET:
            return HttpResponse(u'failure')
    return form

def bookmark_GET(request):
    url = request.GET['url']
    try:
        link = Link.objects.get(url=url)
        bookmark = Bookmark.objects.get(
            link=link,
            user=request.user
        )
        title = bookmark.title
        tags = ' '.join(
            bookmark.tag_set.all().\
            values_list('name', flat=True)
        )
    except (Link.DoesNotExist, Bookmark.DoesNotExist):
        title = ''
        tags = ''
    form = BookmarkSaveForm({
        'url': url,
        'title': title,
        'tags': tags
    })
    return form

Context

StackExchange Code Review Q#2721, answer score: 6

Revisions (0)

No revisions yet.