优化Python/Django视图代码

7 投票
5 回答
679 浏览
提问于 2025-04-16 19:00

我刚开始接触Python/Django和编程。由于我掌握的工具有限,我写了三个视图函数,用于用户注册后:这让用户可以在激活账户之前添加信息和上传缩略图。

我把目前写的代码贴出来,希望能有经验更丰富的人帮我看看怎么改进。毫无疑问,这些代码很粗糙,明显是新手写的,但我觉得通过写代码来学习是最有效的——看到改进的地方,学习新工具,然后再重写。

我知道回答这个问题需要花费不少时间。因此,我会为这个问题提供200分的悬赏。 StackOverflow只允许我在问题发布两天后添加悬赏,所以我会在周二(一旦可以添加时)为这个问题添加悬赏。请注意,由于我在添加悬赏之前不会选择答案,因此在悬赏添加之前提供的答案仍然会被视为“好像”这个问题有悬赏。

以下是我自我注释的代码。特别是,我在每个函数的前10到14行有很多重复代码,用来根据用户是否登录、是否已经填写过这些信息、是否有会话信息等来重定向用户。

# 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) 

一般背景: 用户注册后,我给他们两个会话变量:

    request.session['location'] = get_location_function(request)
    request.session['username'] = new_user   # this is an email address

用户注册后,他们会被重定向到入门页面。

第一页:

# 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))

第二页:

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))

第三页:

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))

5 个回答

3

(我还没有看完你的代码,但我给你一些建议,让代码看起来更好)

我认为django-annoying的render_to装饰器可以让代码更容易阅读和编写。(重定向依然有效):

@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}
3

我会有一些不同的做法:

  1. 在你的 models.py 文件中,你为 graduation 字段使用了 choices 参数,但没有在模型中定义这些选项,而且没有使用全大写字母,通常在 Python 中全大写字母用来表示常量。这样做会更好:

    class UserProfile(models.Model):
        ...
        CHOICES = [('choice1', 'Choice 1'), ('choice2', 'Choice2')]
        graduation = models.CharField(max_length=100, blank=True, choices=CHOICES)
    
  2. 你在 avatar 字段上使用了可选的 default,其实用 blank=True 会更合理,这样做会让你后面的逻辑更简单。在你的视图中:

    if UserProfile.objects.get(user=user).avatar != 'default_profile_picture.jpg':
        UserProfile.objects.get(user=user).avatar.delete()
    

    与其这样,不如在你的模板中直接处理没有头像的情况。

  3. 在你的 getting_started_info 视图中,positions 的默认参数是一个可变对象,positions=[]。这让我有点不舒服,虽然在你的情况下不会出问题,因为它从未被修改。一般来说,最好避免在 Python 中使用可变对象作为默认参数,因为函数定义只会被评估一次。最好避免这样做。

    >>> def foo(l=[]):
    ...:     l.append(1)
    ...:     return l
    ...: 
    
    >>> foo()
    <<< [1]
    
    >>> foo()
    <<< [1, 1]
    
    >>> foo()
    <<< [1, 1, 1]
    
  4. 你写了 else: pass,这其实是多余的,完全可以去掉这个 `else`。

    if user.get_profile().getting_started_boolean:
        return redirect('/home/')
    # else:   
    #    pass
    

如果有机会的话,我会再提一些我对你代码的其他看法。

1

我最开始尝试用django.contrib.formtools.wizard来模拟你的注册流程,但发现这太复杂了,毕竟你的流程只有两个步骤,其中一个只是选择一张图片。如果你打算保持多步骤的注册流程,我强烈建议你考虑使用表单向导解决方案。这样可以让系统自动处理请求之间的状态,你只需要定义一系列的表单就可以了。

不过,我决定把整个流程简化成一个步骤。通过使用一个基本的模型表单,我们可以在一页上轻松捕获所有需要的用户资料信息,而且代码量非常少。

我还选择了基于类的视图,这是在Django 1.3中引入的。这种方式让一些重复的代码(比如在每个函数顶部检查当前流程的代码)更容易管理,虽然一开始可能会显得复杂一些。但一旦你理解了它们,它们在很多情况下都非常好用。好了,接下来是代码。

# 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

我借用了这个回答中的一部分来处理默认图片位置,因为这个建议非常好。把“渲染哪张图片”交给模板和模型来处理。另外,在模型上定义一个方法来回答“是否完成?”的问题,而不是再定义一个字段,这样可以简化流程。

# forms.py

class UserProfileForm(forms.ModelForm):
    class Meta:
        model = UserProfile
        widgets = {
            'user': forms.HiddenInput() # initial data MUST be used to assign this
        }

这是一个基于UserProfile对象的简单ModelForm。这将确保模型的所有字段都能在表单中显示,并且所有内容可以原子性地保存。这是我主要偏离你方法的地方。与其使用多个表单,不如用一个就可以。我觉得这样用户体验更好,特别是字段并不多的情况下。你还可以在用户想修改信息时重用这个表单。

# 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'

这些视图可能是最复杂的部分,但我觉得比一堆复杂的视图函数代码要容易理解多了。我在函数内部做了简单的注释,这样应该能稍微帮助理解。剩下的就是把你的URL连接到这些视图类上。

# 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'),
)

现在这些代码都是未经测试的,所以可能需要调整才能正常工作,并且要与你的网站集成。此外,这完全不同于你的多步骤流程。如果字段很多,多步骤流程会很好,但单独为头像设置一个页面似乎有点过了。希望无论你选择哪种方式,这些建议都有帮助。

关于基于类的视图的一些链接:

API参考
主题介绍

我还想提一下你代码中的一些问题。例如,你有这个:

populate_positions = []
for position in positions:
    populate_positions.append(Position.objects.get(label=position))

可以用这个替换:

populate_positions = Position.objects.filter(label__in=positions)

前者会对每个位置都查询数据库,而后者在评估时只会进行一次查询。

还有;

if request.user.is_authenticated():
    username = request.user.username                      
    user = User.objects.get(email=username)

上面的代码是多余的。你已经可以访问用户对象了,然后又试图再次获取它。

user = request.user

完成。

顺便说一下,如果你想用电子邮件地址作为用户名,你会遇到问题。数据库最多只接受30个字符(这是contrib.auth中User模型的限制)。可以看看这个讨论串中的一些评论,讨论了一些潜在的问题。

撰写回答