2
\$\begingroup\$

I heard that fat models are good idea and I should avoid operations on database in my views so I do something like this below. Can you tell me if it's good idea and which version is better? Please also review the rest of the code by the way.

views.py

class CandidateCreateProfileView(AnonymousRequiredMixin, FormView):
    form_class = CandidateCreateProfileForm
    template_name = 'users/candidate_create_profile.html'
    success_url = reverse_lazy('account_login')

    def dispatch(self, request, *args, **kwargs):
        activation_key = self.kwargs['activation_key']
        self.user = get_object_or_404(User, activation_key=activation_key)

        if self.user.key_expires < timezone.now():
            return render_to_response('users/candidate_confirm_expired.html')
        if self.user.is_active:
            return HttpResponseRedirect(reverse_lazy('account_login'))

        return super(CandidateCreateProfileView, self).dispatch(
            request, *args, **kwargs)

    def get_initial(self):
        initial = super(CandidateCreateProfileView, self).get_initial()
        initial['name'] = self.user.name
        initial['mobile_phone'] = self.user.mobile_phone
        return initial

    def form_valid(self, form):
        self.user.name = form.cleaned_data['name']
        self.user.mobile_phone = form.cleaned_data['mobile_phone']
        self.user.notification_type = form.cleaned_data['notification_type']
        self.user.set_password(form.cleaned_data['password'])
        self.user.is_active = True
        self.user.save()

        return super(CandidateCreateProfileView, self).form_valid(form)

vs.

models.py

class User(models.Model):
    name = models.CharField(max_length=50)
    ...
    def create_candidate_profile(self, name, mobile_phone, notification_type, password):
        self.name = name
        self.mobile_phone= mobile_phone
        self.notification_type=notification_type
        self.set_password(password)
        self.is_active = True
        self.save()

and then in my CandidateCreateProfileView view:

def form_valid(self, form):
    self.user.create_candidate_profile(
        name = form.cleaned_data['name'],
        mobile_phone = form.cleaned_data['mobile_phone'],
        notification_type = form.cleaned_data['notification_type'],
        password = form.cleaned_data['password']
    )
    return super(CandidateCreateProfileView, self).form_valid(form)
\$\endgroup\$
1
\$\begingroup\$

Yes, using MVC frameworks, or in case of Django modified term MTV (Model-Template-View) you will often hear the expression "Fat models, thin controllers". I do think it is a good idea and the business logic should be put into the model.

In your case the method create_candidate_profile is performing a database operation. More important it changes the database and persists data to it.

Data persistence should be done in the model. So, yes, I'd say the second version is definitely the way to go.

In future you might want to add another possibility for creating candidate profile, maybe through a custom command. Then you can make use of this model method:

User.create_candidate_profile(
    name, mobile_phone, notification_type, password
)

The only line in the code I'd slightly change is the following:

activation_key = self.kwargs['activation_key']

should become:

activation_key = self.kwargs.get('activation_key', 'deafult value')

That is just for the case that self.kwargs doesn't have the key activation_key, so you can pass some default value, maybe an empty string ''.

\$\endgroup\$

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.