5
\$\begingroup\$

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
    )
\$\endgroup\$
1
  • \$\begingroup\$ 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". \$\endgroup\$
    – Alexander
    May 31 '11 at 15:24
6
\$\begingroup\$

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
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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