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

Question

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(
 'bookmark_save_form.html',
 variables
 )
 else:
 return render_to_response(
 'bookmark_save.html',
 variables
 )

• Alexander: Seems okay-ish to me, but I would absolutely split it up into into functions/methods instead of the slightly silly "if ajax" check. There are also too many hardcoded values. I'd give it a "4".

Answer 1

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