7

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=[]):
    """
    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))
Improve this question
5
  • 1
    A few thoughts: a) don't offer the bounty just for that reason alone. You may, that's fine, but don't feel compelled to. b) have you seen codereview.stackexchange.com ... that's pretty well exactly what they do there, more or less. Commented Jun 5, 2011 at 21:28
  • 2
    also, why don't you let someone else sponsor the bounty? I'm willing to make that 200 a 300 on your behalf. Just @me here with a comment if the question is still live in two days and I'll be more than happy to help. Commented Jun 5, 2011 at 21:30
  • Out of interest, what version of django are you using for your project? Commented Jun 5, 2011 at 23:03
  • Thank you all for the support, it's an incredibly helpful and generous community. @jcolebrand -- no worries, I don't mind putting up the 200, makes the question more interesting. If you want to bump it up to 300 that'd be great too. django.VERSION = (1, 3, 0, 'final', 0) Commented Jun 6, 2011 at 1:23
  • +1 for putting up more than half your reps as bounty. There ought to be a badge for that level of generosity! Commented Jun 9, 2011 at 17:08

5 Answers 5

Reset to default
3

A few things I'd do differently:

  1. In your models.py you use the choices argument for your graduation field, 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)

  2. You use the optional default on your avatar field. It would make more sense to use blank=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.

  3. In your getting_started_info view, the default paramater for positions is 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]

  4. 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.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

I updated the models.py section to show the choices variable.
@David542 I'd still make it uppercase to indicate it's a constant, and include it in the class definition. Helps to logically organize them together in my opinion, that's generally what I do (and have seen done by other people). It can be nice when you are working with a model and the options used for the ChoiceField are defined as constants obviously in the class, for the purposes of introspection if nothing else.
3

(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}
Improve this answer

Comments

1
+200

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.

Improve this answer

Comments

1

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()
Improve this answer

1 Comment

It's still good practice to leave the else there regardless. This is referred to as 'defensive programming'. It means even if the if doesn't return for some reason (especially for more complex if-statement bodies), you still won't run foo and bar (which might have dangerous side-effects).
0

In check_email, the last two lines are the exact same. You can delete the first one.

Improve this answer

1 Comment

you could, you know, combine all your answers into a single one.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.