How can I improve this "register" view in Django?

Question

I've got a Django-based site that allows users to register (but requires an admin to approve the account before they can view certain parts of the site). I'm basing it off of django.contrib.auth. I require users to register with an email address from a certain domain name, so I've overridden the UserCreationForm's save() and clean_email() methods.

My registration page uses the following view. I'm interested in hearing about how I might improve this view—code improvements or process improvements (or anything else, really).

def register(request):
    if request.method == 'POST':
        form = UserCreationForm(request.POST)

        if form.is_valid():
            message = None

            form.save()

            username = form.cleaned_data['username']
            password = form.cleaned_data['password1']

            user = authenticate(username=username, password=password)

            first_name = form.cleaned_data['first_name']
            last_name = form.cleaned_data['last_name']
            email = user.email

            # If valid new user account
            if (user is not None) and (user.is_active):
                login(request, user)
                message = "<strong>Congratulations!</strong> You have been registered."

                # Send emails
                try:
                    # Admin email
                    pk = None
                    try: pk = User.objects.filter(username=username)[0].pk
                    except: pass

                    admin_email_template = loader.get_template('accounts/email_notify_admin_of_registration.txt')
                    admin_email_context = Context({
                        'first_name': first_name,
                        'last_name': last_name,
                        'username': username,
                        'email': email,
                        'pk': pk,
                    })
                    admin_email_body = admin_email_template.render(admin_email_context)
                    mail_admins("New User Registration", admin_email_body)

                    # User email
                    user_email_template = loader.get_template('accounts/email_registration_success.txt')
                    user_email_context = Context({
                        'first_name': form.cleaned_data['first_name'],
                        'username': username,
                        'password': password,
                    })
                    user_email_body = user_email_template.render(user_email_context)
                    user.email_user("Successfully Registered at example.com", user_email_body)
                except:
                    message = "There was an error sending you the confirmation email. You should still be able to login normally."
            else:
                message = "There was an error automatically logging you in. Try <a href=\"/login/\">logging in</a> manually."

            # Display success page
            return render_to_response('accounts/register_success.html', {
                    'username': username,
                    'message': message,
                },
                context_instance=RequestContext(request)
            )
    else: # If not POST
        form = UserCreationForm()

    return render_to_response('accounts/register.html', {
            'form': form,
        },
        context_instance=RequestContext(request)
    )

Answer 1

First response: It looks a heck of a lot better than 95% of the "improve my code" questions.

Is there anything in particular you are dissatisfied with?

Answer 2

You don't even need this code, but I think the style:

pk = None
try: pk = User.objects.filter(username=username)[0].pk
except: pass

is more naturally written like:

try:
    user = User.objects.get(username=username)
except User.DoesNotExist:
    user = None

and then in your admin notify template use {{ user.id }} instead of {{ pk }}.

But, like I said, you don't need that code at all because you already have a user object from your call to authenticate().

Generally in Python, it's considered poor practice to have the exception handler in a try/except block be empty. In other words, always capture a specific exception such as User.DoesNotExist for this case.

It's also poor practice to have many lines inside the try part of the try/except block. It is better form to code this way:

try:
    ... a line of code that can generate exceptions to be handled ...
except SomeException:
    ... handle this particular exception ...
else:
    ... the rest of the code to execute if there were no exceptions ...

A final, minor, recommendation is to not send the email directly in your view because this won't scale if your site starts to see heavy registration requests. It is better add in the django-mailer app to offload the work into a queue handled by another process.

Answer 3

I personally try to put the short branch of an if-else statement first. Especially if it returns. This to avoid getting large branches where its difficult to see what ends where. Many others do like you have done and put a comment at the else statement. But python doesnt always have an end of block statement - like if a form isn't valid for you.

So for example:

def register(request):
    if request.method != 'POST':
       form = UserCreationForm()
       return render_to_response('accounts/register.html', 
                                 { 'form': form, },
                                 context_instance=RequestContext(request)
                                 )

    form = UserCreationForm(request.POST)
    if not form.is_valid():
       return render_to_response('accounts/register.html', 
                                 { 'form': form, },
                                 context_instance=RequestContext(request)
                                 )
    ...