Django post function : split method is a good way?

Question

I'm working on my Django project and I'm trying to develop this one with 'Class Based View' (CBV) method in order to keep the code maintainable.

I have a class named FreepubHome with some methods and a very important post method. I would like to split this method into three methods and I don't know how I can do that.

This post method let to:

• fill and submit the form
• create a token
• send an e-mail

So I would like to get a post function which let to fill and submit my form, call the token method and the sending method.

It's very important for me to understand the process, the methodology and how I can do that in order to simplify others methods that are very important.

This is my class :

from django.template.loader import get_template
from django.core.mail import EmailMultiAlternatives
from django.views.generic import CreateView
import hashlib
from .models import Publication, Document, Download
class FreepubHomeView(CreateView):
 """ Render the home page """
 template_name = 'freepub/index.html'
 form_class = CustomerForm
 def get_context_data(self, **kwargs):
 kwargs['document_list'] = Document.objects.all().order_by('publication__category__name')
 return super(FreepubHomeView, self).get_context_data(**kwargs)
 @staticmethod
 def create_token(self, arg1, arg2, datetime):
 # Create token based on some arguments
 plain = arg1 + arg2 + str(datetime.now())
 token = hashlib.sha1(plain.encode('utf-8')).hexdigest()
 return token
 @staticmethod
 def increment(model):
 model.nb_download = model.nb_download + 1
 return model.save()
 def post(self, request, *args, **kwargs):
 form = self.form_class()
 document_choice = request.POST.getlist('DocumentChoice')
 if request.method == 'POST':
 form = self.form_class(request.POST)
 for checkbox in document_choice:
 document_edqm_id = Document.objects.get(id=checkbox).edqm_id
 publication_title = Document.objects.get(id=checkbox).publication.title
 email = request.POST['email']
 token = self.create_token(email, document_edqm_id, datetime)
 Download.objects.create(email=email, pub_id=checkbox, token=token)
 document_link = Document.objects.get(id=checkbox).upload #gives media/file
 document_link2 = Download.objects.get(token = token).pub_id #gives document id
 print(document_link)
 print(document_link2)
 context = {'document_link': document_link,
 'publication_title': publication_title}
 if form.is_valid():
 message = get_template('freepub/message.txt').render(context)
 html_message = get_template('freepub/message.html').render(context)
 subject = 'EDQM HelpDesk and Publications registration'
 mail = EmailMultiAlternatives(subject, message, '[email protected]', [email])
 mail.attach_alternative(html_message, "text/html")
 #mail.attach_file(document_link.path) # Add attachement
 mail.send(fail_silently=False)
 print('Email envoyé à ' + email)
 messages.success(request, str(
 publication_title) + '\n' + 'You will receive an e-mail with your access to ' + document_edqm_id)
 # Update number of downloads for document and publication
 document = Document.objects.get(id=checkbox)
 document = self.increment(document)
 publication_id = Document.objects.get(id=checkbox).publication.id
 publication = Publication.objects.get(id=publication_id)
 publication = self.increment(publication)
 else:
 print('form invalid')
 return HttpResponseRedirect(self.get_success_url())
 def get_success_url(self):
 return reverse('freepub-home')

These are the classes in models.py: (Not for review)

class Document(models.Model):
 FORMAT_CHOICES = (
 ('pdf', 'PDF'),
 ('epub', 'ePUB'),
 )
 LANGUAGE_CHOICES = (
 ('FR', 'FR'),
 ('EN', 'EN'),
 )
 edqm_id = models.CharField(max_length=12, verbose_name=_('publication ID'), unique=True, default='')
 language = models.CharField(max_length=2, verbose_name=_('language'), choices=LANGUAGE_CHOICES, null=False)
 format = models.CharField(max_length=10, verbose_name=_('format'), choices=FORMAT_CHOICES, null=False)
 title = models.CharField(max_length=512, verbose_name=_('document title'), null=False)
 publication = models.ForeignKey(Publication, verbose_name=_('publication title'), null=False, related_name='documents')
 upload = models.FileField(upload_to='media/', validators=[validate_file_extension])
 creation_date = models.DateTimeField(auto_now_add=True, verbose_name=_('creation date'), null=False)
 modification_date = models.DateTimeField(auto_now=True, verbose_name=_('modification date'), null=False)
 nb_download = models.IntegerField(verbose_name=_('number of download'), default=0)
 class Meta:
 verbose_name = _('document')
 verbose_name_plural = _('document')
 def __str__(self):
 return f"{self.edqm_id} : {self.title}"
class Download(models.Model):
 email = models.CharField(max_length=150, verbose_name=_('e-mail'), null=False)
 pub = models.ForeignKey(Document, verbose_name=_('document'), null=False)
 token = models.CharField(max_length=40, verbose_name=_('download token'), unique=True, null=False)
 download_date = models.DateTimeField(auto_now_add=True, verbose_name=_('download date'), null=False)
 expiration_date = models.DateTimeField(auto_now=True, verbose_name=_('expiration date'), null=False)
 nb_download = models.IntegerField(verbose_name=_('usage'), default=0)
 class Meta:
 verbose_name = _('download')
 verbose_name_plural = _('download')
 def __str__(self):
 return f"{self.email} : {self.token}"

• 301_Moved_Permanently: Hi! Welcome to Code Review! I’m not sure you’re asking the right kind of question for this site. Are you only asking "how do I split my method in three?" in which case this may be off-topic here, or are you asking for general improvements and think that splitting the method in three is the way to go?
• Essex: Thank you @MathiasEttinger with your answer. Your question is very good because it's a bit both of them. If it was "How do Isplit my method in three", I should ask this question on stackoverflow I think. So it's rather the second part from your question. I would like to now if it's interessant to split my method in three and if you could help me to improve my post function.

Answer 1

(I don't know Django and so some aspects of my review may be wrong)

1. I first removed most of your functions, as things like increment aren't really that helpful. It also leaves your code with everything in one place, so that when you try to improve it you can see everything.

2. I then used guard clauses to reduce the amount of indentation post needs.

   Take the following change for example, with it you know that you only perform actions on post requests. Where with your code it would take longer to know that.

   if request.method != 'POST':
    return HttpResponseRedirect(self.SUCCESSFUL_URL)
   

3. Assuming Document.objects.get(id=checkbox) doesn't have any side effects, then I'd just make it a variable.

4. I would reduce the amount of variables you have. Most of your lines of code were just variables that are used once.

   # Original
   context = {'publication_title': Document.objects.get(id=checkbox).publication.title}
   # With (3)
   context = {'publication_title': document.publication.title}
   

   With (3) all you have to do is add document to your variable, and it removes a line of code. And so it improves readability at the expense of having to write document a couple more times.

5. I'd hope Django objects support += and so you can change increment to use it.

   model.nb_download += 1
   

6. I'd make a function email that takes a couple of arguments but performs all EmailMultiAlternatives and get_template handling.

from django.template.loader import get_template
from django.core.mail import EmailMultiAlternatives
from django.views.generic import CreateView
import hashlib
from .models import Publication, Document, Download
def gen_token(*values):
 plain = ''.join([str(i) for i in values] + [str(datetime.now())])
 return hashlib.sha1(plain.encode('utf-8')).hexdigest()
class FreepubHomeView(CreateView):
 """ Render the home page """
 template_name = 'freepub/index.html'
 form_class = CustomerForm
 SUCCESSFUL_URL = reverse('freepub-home')
 def get_context_data(self, **kwargs):
 kwargs['document_list'] = Document.objects.all().order_by('publication__category__name')
 return super(FreepubHomeView, self).get_context_data(**kwargs)
 def email(self, email, upload, title, edqm_id):
 context = {
 'document_link': upload,
 'publication_title': title
 }
 subject = 'EDQM HelpDesk and Publications registration'
 message = get_template('freepub/message.txt').render(context)
 mail = EmailMultiAlternatives(subject, message, '[email protected]', [email])
 html_message = get_template('freepub/message.html').render(context)
 mail.attach_alternative(html_message, "text/html")
 #mail.attach_file(document.upload.path) # Add attachement
 mail.send(fail_silently=False)
 print('Email envoyé à ' + email)
 messages.success(request, str(title) + '\n' + 'You will receive an e-mail with your access to ' + edqm_id)
 def post(self, request, *args, **kwargs):
 if request.method != 'POST':
 return HttpResponseRedirect(self.SUCCESSFUL_URL)
 form = self.form_class(request.POST)
 email = request.POST['email']
 for checkbox in request.POST.getlist('DocumentChoice'):
 document = Document.objects.get(id=checkbox)
 token = gen_token(email, document.edqm_id)
 Download.objects.create(email=email, pub_id=checkbox, token=token)
 if not form.is_valid():
 print('form invalid')
 continue
 self.email(email, document.upload, document.publication.title, document.eqdm_id)
 document.nb_download += 1
 document.save()
 publication = Publication.objects.get(id=document.publication.id)
 publication.nb_download += 1
 publication.save()
 return HttpResponseRedirect(self.SUCCESSFUL_URL)

• 301_Moved_Permanently: "I'd hope Django objects support += and so you can change increment to use it." In fact, for such simple updates, you can also do it all in the database without retrieving the objects in the Python world: stackoverflow.com/a/1599090/5069029
• Essex: Very good job @Peilonrayz ! I agree with you for lots of modifications ! It's better mainly with document variable which is called just one time by the queryset. I had too many variables called one time and you solved that. I like the email function and I don't know why I didn't overcome to do that ...It works fine, it's clear. I don't modify SUCCESSFUL_URL because I believe with Django CreateView it's better to define success_url by a function. Point 1 : I'm ok ! Point 2 : It's ok, but I keep my success_url function Point 3 to 6 : I'm ok Thank you so much !