Improving Python/django view code
I am very new to Python/Django and programming in general. With the limited tools in my programming bag, I have written three views functions for after a user registers: it allows the user to add information and upload a thumbnail before activating his account.
I have posted the code that I have written so far so that someone with far more experience than I have can show me how to improve the code. No doubt this is crude code with all the marks of a novice, but I learn best from writing code -- seeing ways to improve it and learning new tools -- and rewriting it.
I know that the answer to this question will take a considerable amount of time. Therefore, I will be awarding a 200 point bounty to this question. SO will only allow me to add a bounty two days after a question has been posted, so I will be adding a bounty to this question on Tuesday (as soon as it's available to add). Please note that since I won't be selecting an answer until after I have posted a bounty, answers that are provided before the bounty has been added will still be 'as if' there is a bounty on the question
The following is my self-commented code. In particular, I have a lot of boilerplate code for the first 10-14 lines of each function to redirect a user based upon whether he is logged in, if he has already filled out this info, if he has session info, etc.
# in model.py
choices = ([(x,str(x)) for x in range(1970,2015)])
choices.reverse()
class UserProfile(models.Model):
"""
Fields are user, network, location, graduation, headline, and position.
user is a ForeignKey, unique = True (OneToOne). network is a ForeignKey.
loation, graduation, headline, and position are optional.
"""
user = models.ForeignKey(User, unique=True)
network = models.ForeignKey(Network)
location = models.CharField(max_length=100, blank=True)
graduation = models.CharField(max_length=100, blank=True, choices=choices)
headline = models.CharField(max_length=100, blank=True)
positions = models.ManyToManyField(Position, blank=True)
avatar = models.ImageField(upload_to='images/%Y/%m/%d', blank=True, default='default_profile_picture.jpg')
# if the user has already filled out the 'getting started info', set boolean=True
getting_started_boolean = models.BooleanField(default=False)
General context: after a user has registered, I am giving them two session variables:
request.session['location'] = get_location_function(request)
request.session['username'] = new_user # this is an email address
After a user has registered, they are re-directed to the getting_started pages.
First page:
# in views.py
def getting_started_info(request, positions=[]):
"""
开发者_如何学JAVA This is the first of two pages for the user to
add additional info after they have registrered.
There is no auto log-in after the user registers,
so the individiaul is an 'inactive user' until he
clicks the activation link in his email.
"""
location = request.session.get('location')
if request.user.is_authenticated():
username = request.user.username # first see if the user is logged in
user = User.objects.get(email=username) # if so, get the user object
if user.get_profile().getting_started_boolean:
return redirect('/home/') # redirect to User home if user has already filled out page
else:
pass
else:
username = request.session.get('username', False) # if not logged in, see if session info exists from registration
if not username:
return redirect('/account/login') # if no session info, redirect to login page
else:
user = User.objects.get(email=username)
if request.method == 'POST':
if 'Next Step' in request.POST.values(): # do custom processing on this form
profile = UserProfile.objects.get(user=user)
profile.location = request.POST.get('location')
populate_positions = []
for position in positions:
populate_positions.append(Position.objects.get(label=position))
profile.positions = request.POST.get('position')
profile.headline = request.POST.get('headline')
profile.graduation = request.POST.get('graduation')
profile.save()
return redirect('/account/gettingstarted/add_pic/')
else:
form = GettingStartedForm(initial={'location': location})
return render_to_response('registration/getting_started_info1.html', {'form':form, 'positions': positions,}, context_instance=RequestContext(request))
Second page:
def getting_started_pic(request):
"""
Second page of user entering info before first login.
This is where a user uploads a photo.
After this page has been finished, set getting_started_boolean = True,
so user will be redirected if hits this page again.
"""
if request.user.is_authenticated():
username = request.user.username
user = User.objects.get(email=username)
if user.get_profile().getting_started_boolean:
return redirect('/home/')
else:
pass
else:
username = request.session.get('username', False)
if not username:
return redirect('/account/login')
else:
user = User.objects.get(email=username)
try:
profile = UserProfile.objects.get(user=User.objects.get(email=username)) # get the profile to display the user's picture
except UserProfile.DoesNotExist: # if no profile exists, redirect to login
return redirect('/account/login') # this is a repetition of "return redirect('/account/login/')" above
if request.method == 'POST':
if 'upload' in request.POST.keys():
form = ProfilePictureForm(request.POST, request.FILES, instance = profile)
if form.is_valid():
if UserProfile.objects.get(user=user).avatar != 'default_profile_picture.jpg': # if the user has an old avatar image
UserProfile.objects.get(user=user).avatar.delete() # delete the image file unless it is the default image
object = form.save(commit=False)
try:
t = handle_uploaded_image(request.FILES['avatar']) # do processing on the image to make a thumbnail
object.avatar.save(t[0],t[1])
except KeyError:
object.save()
return render_to_response('registration/getting_started_pic.html', {'form': form, 'profile': profile,}, context_instance=RequestContext(request))
if 'finish' in request.POST.keys():
UserProfile.objects.filter(user=user).update(getting_started_boolean='True') # now add boolean = True so the user won't hit this page again
return redirect('/account/gettingstarted/check_email/')
else:
form = ProfilePictureForm()
return render_to_response('registration/getting_started_pic.html', {'form': form, 'profile': profile,}, context_instance=RequestContext(request))
Third page:
def check_email(request):
"""
End of getting started. Will redirect to user home
if activation link has been clicked. Otherwise, will
allow user to have activation link re-sent.
"""
if request.user.is_authenticated(): # if the user has already clicked his activation link, redirect to User home
return redirect('/home/')
else: # if the user is not logged in, load this page
resend_msg=''
user = email = request.session.get('username')
if not email:
return redirect('/account/login/')
if Site._meta.installed:
site = Site.objects.get_current()
else:
site = RequestSite(request)
if request.method == 'POST':
RegistrationProfile.objects.resend_activation(email, site)
resend_msg = 'An activation email has been resent to %s' %(email)
return render_to_response('registration/getting_started_check_email.html', {'email':email, 'resend_msg':resend_msg}, context_instance=RequestContext(request))
return render_to_response('registration/getting_started_check_email.html', {'email':email, 'resend_msg':resend_msg}, context_instance=RequestContext(request))
A few things I'd do differently:
In your models.py you use the
choicesargument for yourgraduationfield, but don't define the choices on the model, and don't use all caps which is usually used to denote a constant in Python. This would be better:class UserProfile(models.Model): ... CHOICES = [('choice1', 'Choice 1'), ('choice2', 'Choice2')] graduation = models.CharField(max_length=100, blank=True, choices=CHOICES)You use the optional
defaulton youravatarfield. It would make more sense to useblank=True, it actually complicates your logic later on, in your view:if UserProfile.objects.get(user=user).avatar != 'default_profile_picture.jpg': UserProfile.objects.get(user=user).avatar.delete()Instead it's probably better to just deal with the absence of an avatar in your template.
In your
getting_started_infoview, the default paramater forpositionsis a mutable object,positions=[]. This makes me cringe in general, although in your cause it won't cause you any issues since it's never mutated. It's a good idea to avoid using a mutable object as a default parameter in python, because function definitions are only evaluated once. It's best to avoid this.>>> def foo(l=[]): ...: l.append(1) ...: return l ...: >>> foo() <<< [1] >>> foo() <<< [1, 1] >>> foo() <<< [1, 1, 1]You do
else: pass, this is redundant, remove the `else' completely.if user.get_profile().getting_started_boolean: return redirect('/home/') # else: # pass
If I get a chance later, I'll go over a few more issues I have with the way you have things.
(I did not read all your code yet, but I am giving some general advices to make the code look better)
I believe django-annoying's render_to decorator makes code easier to read and write. (The redirects are still working):
@render_to('registration/getting_started_info1.html')
def getting_started_info(request, positions=[]):
...
return {'form':form, 'positions': positions}
@render_to('registration/getting_started_pic.html')
def getting_started_pic(request):
...
return {'form': form, 'profile': profile}
@render_to('registration/getting_started_check_email.html')
def check_email(request):
...
return {'email':email, 'resend_msg':resend_msg}
I originally tried to replicate the behaviour of your signup process using django.contrib.formtools.wizard, but it was becoming far too complicated, considering there are only two steps in your process, and one of them is simply selecting an image. I would highly advise looking at a form-wizard solution if you intend to keep the multi-step signup process though. It will mean the infrastructure takes care of carrying state across requests, and all you need to do is define a series of forms.
Anyway, I've opted to simplify your whole process to one step. Using a basic model form, we are able to simply capture ALL of the UserProfile information you need on one page, with very very little code.
I've also gone with class-based-views, introduced in Django 1.3. It makes boilerplate code (such as your check at the top of each function for what process you're up to) much nicer to manage, at the cost of more upfront complexity. Once you understand them though, they are fantastic for a lot of use cases. Ok, so; on to the code.
# in models.py
graduation_choices = ([(x,str(x)) for x in range(1970,2015)])
graduation_choices.reverse()
class UserProfile(models.Model):
# usually you want null=True if blank=True. blank allows empty forms in admin, but will
# get a database error when trying to save the instance, because null is not allowed
user = models.OneToOneField(User) # OneToOneField is more explicit
network = models.ForeignKey(Network)
location = models.CharField(max_length=100, blank=True, null=True)
graduation = models.CharField(max_length=100, blank=True, null=True, choices=graduation_choices)
headline = models.CharField(max_length=100, blank=True, null=True)
positions = models.ManyToManyField(Position, blank=True)
avatar = models.ImageField(upload_to='images/%Y/%m/%d', blank=True, null=True)
def get_avatar_path(self):
if self.avatar is None:
return 'images/default_profile_picture.jpg'
return self.avatar.name
def is_complete(self):
""" Determine if getting started is complete without requiring a field. Change this method appropriately """
if self.location is None and self.graduation is None and self.headline is None:
return False
return True
I stole a piece of this answer for handling the default image location as it was very good advice. Leave the 'which picture to render' up to the template and the model. Also, define a method on the model which can answer the 'completed?' question, rather than defining another field if possible. Makes the process easier.
# forms.py
class UserProfileForm(forms.ModelForm):
class Meta:
model = UserProfile
widgets = {
'user': forms.HiddenInput() # initial data MUST be used to assign this
}
A simple ModelForm based on the UserProfile object. This will ensure that all fields of the model are exposed to a form, and everything can be saved atomically. This is how I've mainly deviated from your method. Instead of using several forms, just one will do. I think this is a nicer user experience also, especially since there aren't very many fields at all. You can also reuse this exact form for when a user wants to modify their information.
# in views.py - using class based views available from django 1.3 onward
class SignupMixin(View):
""" If included within another view, will validate the user has completed
the getting started page, and redirects to the profile page if incomplete
"""
def dispatch(self, request, *args, **kwargs):
user = request.user
if user.is_authenticated() and not user.get_profile().is_complete()
return HttpResponseRedirect('/profile/')
return super(SignupMixin, self).dispatch(request, *args, **kwargs)
class CheckEmailMixin(View):
""" If included within another view, will validate the user is active,
and will redirect to the re-send confirmation email URL if not.
"""
def dispatch(self, request, *args, **kwargs):
user = request.user
if user.is_authenticated() and not user.is_active
return HttpResponseRedirect('/confirm/')
return super(CheckEmailMixin, self).dispatch(request, *args, **kwargs)
class UserProfileFormView(FormView, ModelFormMixin):
""" Responsible for displaying and validating that the form was
saved successfully. Notice that it sets the User automatically within the form """
form_class = UserProfileForm
template_name = 'registration/profile.html' # whatever your template is...
success_url = '/home/'
def get_initial(self):
return { 'user': self.request.user }
class HomeView(TemplateView, SignupMixin, CheckEmailMixin):
""" Simply displays a template, but will redirect to /profile/ or /confirm/
if the user hasn't completed their profile or confirmed their address """
template_name = 'home/index.html'
These views will probably be the most complicated part, but I feel are much easier to understand than reams of spaghetti view function code. I've documented the functions briefly inline, so it should make it slightly easier to understand. The only thing left is to wire up your URLs to these view classes.
# urls.py
urlpatterns = patterns('',
url(r'^home/$', HomeView.as_view(), name='home'),
url(r'^profile/$', UserProfileFormView.as_view(), name='profile'),
url(r'^confirm/$', HomeView.as_view(template_name='checkemail.html'), name='checkemail'),
)
Now this is all untested code, so it may need tweaks to get working, and to integrate into your particular site. Also, it completely departs from your multi-step process. The multi-step process would be nice in the case of many many many fields.. but a separate page JUST to do the avatar seems a bit extreme to me. Hopefully, whichever way you go, this helps.
Some links regarding class based views:
API Reference
Topic Introduction
I also wanted to mention a few things about your code in general. For instance you have this:
populate_positions = []
for position in positions:
populate_positions.append(Position.objects.get(label=position))
Which could be replaced with this:
populate_positions = Position.objects.filter(label__in=positions)
The former will hit the DB for every position. The latter will do a single query when evaluated.
Also;
if request.user.is_authenticated():
username = request.user.username
user = User.objects.get(email=username)
The above is redundant. You've got access to the user object already, and then trying to fetch it again.
user = request.user
Done.
By the way, if you want to use email addresses as a username, you will have problems. The database will only accept a maximum of 30 characters (it is how the User model is writtin in contrib.auth). Read some of them comments on this thread that discuss some of the pitfalls.
In the following block:
if request.user.is_authenticated():
return redirect('/home/')
else:
foo()
bar()
the else is not really needed. Because of the return statement, foo() will not be executed if the user is authenticated. You can prevent over-indentation by removing it.
if request.user.is_authenticated():
return redirect('/home/')
foo()
bar()
In check_email, the last two lines are the exact same. You can delete the first one.
加载中,请稍侯......
精彩评论