2
\$\begingroup\$

This is my forms.py for a small survey application I am developing. I previously posted some of my models.py.

I am completely self-taught and am just looking for advice on how to improve. Any advice, even things you might think should be obvious, would be much appreciated.

from django import forms
from django.forms import ModelForm, Textarea, extras, RadioSelect, IntegerField
from django.forms.extras import SelectDateWidget
from django.forms.extras import *
from survey.models import Person
from django.core import validators
import random
from django.forms.util import ErrorList
from itertools import chain
from django.utils.encoding import force_unicode



# Used to make RadioSelect non manditory and remove default option in the ModelForm 
# (http://stackoverflow.com/questions/30218930/how-to-make-a-radioselect-non-manditory-and-remove-default-option-in-a-modelform) 
class RadioSelectNotNull(RadioSelect):
    def get_renderer(self, name, value, attrs=None, choices=()):
        """Returns an instance of the renderer."""
        if value is None: value = ''
        str_value = force_unicode(value) # Normalize to string.
        final_attrs = self.build_attrs(attrs)
        choices = list(chain(self.choices, choices))
        if choices[0][0] == '':
            choices.pop(0)
        return self.renderer(name, str_value, final_attrs, choices)


class SurveyFormIT1(forms.ModelForm):      
    class Meta:
        model = Person    
        fields = ['instruction_task_one_value']
        widgets = {'instruction_task_one_value' : forms.HiddenInput}       

class SurveyFormIT2(forms.ModelForm):      
    class Meta:
        model = Person    
        fields = ['instruction_task_two_value']
        widgets = {'instruction_task_two_value' : forms.HiddenInput}      

class Start(forms.ModelForm):      
    class Meta:
        model = Person    
        fields = ['start']
        widgets = {'start' : forms.HiddenInput}

class SurveyFormA(forms.ModelForm):

    birthdate = forms.DateField(widget=extras.SelectDateWidget(), required=False)

    class Meta:
        model = Person
        fields = ['sender', 'birthdate', 'sex', 'marital_status', 'state']

class SurveyFormB(forms.ModelForm): 
    class Meta:
        model = Person
        fields = ['internet_usage', 'smart_phone_ownership', 'smart_phone_usage']        
        widgets = {'internet_usage' : RadioSelectNotNull,
                   'smart_phone_ownership' : RadioSelectNotNull,
                   'smart_phone_usage' : RadioSelectNotNull,}

class SurveyFormC(forms.ModelForm):   
    class Meta:
        model = Person
        fields = ['education', 'wages', 'presentage_savings', 'occupation', 'living']         
        widgets = {'education' : RadioSelect(),
                   'wages' : RadioSelect(),
                   'presentage_savings' : RadioSelect(),
                   'occupation' : RadioSelect(),
                   'living' : RadioSelect(),}

class SurveyFormD(forms.ModelForm): # Political Viewpoints
    class Meta:
        model = Person
        fields = ['political_leaning', 'democrat_republican', 'voting_rights', 'election']             
        widgets = {'political_leaning' : RadioSelect(),
                   'democrat_republican' : RadioSelect(),
                   'voting_rights' : RadioSelect(),
                   'election' : forms.CheckboxSelectMultiple,}

class SurveyFormE(forms.ModelForm): #News Access Questions
    class Meta:
        model = Person
        fields = ['news_access', 'station_access', 'newspaper_access']        
        widgets = {'news_access' : forms.CheckboxSelectMultiple,
                   'station_access' : forms.CheckboxSelectMultiple,
                   'newspaper_access' : forms.CheckboxSelectMultiple,}

class SurveyFormSpike1(forms.ModelForm):           
    class Meta:
        model = Person    
        fields = ['spike_one_value']
        widgets = {'spike_one_value' : forms.HiddenInput}  


class SurveyFormF1(forms.ModelForm):      
    class Meta:
        model = Person       
        fields = ['slider_one_value']
        widgets = {'slider_one_value' : forms.HiddenInput}   

class SurveyFormF2(forms.ModelForm):        
    class Meta:
        model = Person        
        fields = ['slider_two_value']
        widgets = {'slider_two_value' : forms.HiddenInput}   

class SurveyFormF3(forms.ModelForm):       
    class Meta:
        model = Person        
        fields = ['slider_three_value']
        widgets = {'slider_three_value' : forms.HiddenInput}  

class SurveyFormDV1(forms.ModelForm):       
    class Meta:
        model = Person        
        fields = ['DV_one_value', 'DV_two_value', 'DV_three_value']

        widgets = {'DV_one_value' : forms.HiddenInput,
                   'DV_two_value' : forms.HiddenInput,
                   'DV_three_value' : forms.HiddenInput}                 

class SurveyFormF4(forms.ModelForm):       
    class Meta:
        model = Person        
        fields = ['slider_four_value']
        widgets = {'slider_four_value' : forms.HiddenInput}    

class SurveyFormF5(forms.ModelForm):        
    class Meta:
        model = Person        
        fields = ['slider_five_value']
        widgets = {'slider_five_value' : forms.HiddenInput}   

class SurveyFormF6(forms.ModelForm):       
    class Meta:
        model = Person        
        fields = ['slider_six_value']
        widgets = {'slider_six_value' : forms.HiddenInput}  

class SurveyFormDV2(forms.ModelForm):       
    class Meta:
        model = Person        
        fields = ['DV_four_value', 'DV_five_value', 'DV_six_value']
        widgets = {'DV_four_value' : forms.HiddenInput,
                   'DV_five_value' : forms.HiddenInput,
                   'DV_six_value' : forms.HiddenInput} 


class SurveyFormSpike2(forms.ModelForm):      
    class Meta:
        model = Person    
        fields = ['spike_two_value']
        widgets = {'spike_two_value' : forms.HiddenInput}  



class SurveyFormF7(forms.ModelForm):        
    class Meta:
        model = Person        
        fields = ['slider_seven_value']
        widgets = {'slider_seven_value' : forms.HiddenInput}   

class SurveyFormF8(forms.ModelForm):       
    class Meta:
        model = Person       
        fields = ['slider_eight_value']
        widgets = {'slider_eight_value' : forms.HiddenInput}   

class SurveyFormF9(forms.ModelForm):        
    class Meta:
        model = Person        
        fields = ['slider_nine_value']
        widgets = {'slider_nine_value' : forms.HiddenInput}   

class SurveyFormDV3(forms.ModelForm):       
    class Meta:
        model = Person        
        fields = ['DV_seven_value', 'DV_eight_value', 'DV_nine_value']
        widgets = {'DV_seven_value' : forms.HiddenInput,
                   'DV_eight_value' : forms.HiddenInput,
                   'DV_nine_value' : forms.HiddenInput} 

class SurveyFormDV4(forms.ModelForm):          
    class Meta:
        model = Person       
        fields = ['DV_positive']
        widgets = {'DV_positive' : forms.HiddenInput}


class SurveyFormDV5(forms.ModelForm):     
    class Meta:
        model = Person        
        fields = ['DV_negative']
        widgets = {'DV_negative' : forms.HiddenInput}



class SurveyFormG(forms.ModelForm): #Reflective Questions
    class Meta:
        model = Person

        fields = ['medium', 'gender', 'political', 'racial', 'climate_change', 'health_care', 'religious', 'party_benefit', 'party_benefit_message']        
        widgets = {'medium' : RadioSelect(),
                   'gender' : RadioSelect(),
                   'political' : RadioSelect(),
                   'racial' : RadioSelect(),
                   'climate_change' : RadioSelect(),
                   'health_care' : RadioSelect(),
                   'religious' : RadioSelect(),
                   'party_benefit' : RadioSelect(),
                   'party_benefit_message' : forms.Textarea} 

class SurveyFormH(forms.ModelForm): #Reflective Questions
    class Meta:
        model = Person

        fields = ['topics', 'topics_message', 'website_aspects', 'deliberate', 'deliberate_message']        
        widgets = {'topics' : forms.CheckboxSelectMultiple,
                   'topics_message' : forms.Textarea,
                    'website_aspects' : forms.CheckboxSelectMultiple,
                    'deliberate' : RadioSelect(),
                    'deliberate_message' : forms.Textarea}   

class SurveyFormI(forms.ModelForm):
    class Meta:
        model = Person

        fields = ['medium', 'branding', 'agenda', 'share_buttons', 'agreement_share_buttons', 'elements_features', 'elements_features_message', 'limit_elements_features', 'limit_elements_features_message']        
        widgets = {'medium' : RadioSelect(),
                   'branding' : RadioSelect(),
                   'agenda' : RadioSelect(),
                   'share_buttons' : RadioSelect(),
                   'agreement_share_buttons' : RadioSelect(),
                   'elements_features' : RadioSelect(),
                   'elements_features_message' : forms.Textarea,
                   'limit_elements_features' : RadioSelect(),
                   'limit_elements_features_message' : forms.Textarea,}           

class SurveyFormJ(forms.ModelForm):
    class Meta:
        model = Person

        fields = ['bias_action', 'bias_action_message', 'bias_stop_reading']        
        widgets = {'bias_action' : RadioSelect(),
                   'bias_action_message' : forms.Textarea,
                   'bias_stop_reading' : RadioSelect(),}   
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

shuttle87 already hit a few of the major recommendations, so I'll cover the more smaller details that stood out to me.

You have a lot of forms for a single model

Right now I count 28 separate forms for the Person model. This is usually a really good (or bad) sign that the model holds too much information, and that can (and likely will) have negative side effects in the long run.

You are using 25 hidden inputs in your form, and while this isn't always a bad thing, it is when you only have 28 separate forms. I counted that 19 out of 28 of your forms contain only hidden form fields, which suggests one of two things:

  1. You are only using forms to validate and save information in your database.
  2. You are using widgets on your website that require hidden input fields instead of a native field.

Django forms are not a replacement for the ORM

The Django ORM (which includes the models) allows for you to update fields on models by just setting the value on the object and calling .save(). This will update the row in the database, and you will later be able to recall that information.

While Django model forms will do this automatically, they are not designed to be a replacement in the backend for interacting directly with models.

Avoid using widgets that require hidden inputs

A user cannot interact with a hidden input and set the value unless they have the power to directly set the value on the field or have a JavaScript widget that handles it for them. Because of this, using a hidden input in a form that should be holding information is not the best idea. It creates forms that are not accessible to those who do not have JavaScript, and generally by extension those using screen readers.

\$\endgroup\$
4
  • \$\begingroup\$ "Django forms are not a replacement for the ORM" absolutely agree with this. I think that really gets to the crux of the issue I was trying to point out but does so more clearly/succinctly. \$\endgroup\$
    – shuttle87
    Jun 21, 2015 at 16:02
  • \$\begingroup\$ Hey, thanks for the feedback, Its much appreciated. Yes all of the hidden inputs (bar two) are used to take the value of a jQuery slider widget and insert it into the person Model in the database. The last two take the name of the participants favorite / least favorite image. \$\endgroup\$
    – Deepend
    Jun 22, 2015 at 15:54
  • \$\begingroup\$ Honestly this is the first time I have heard of ORM. I will have to spend a while looking into it. A quick search of the documentation does not show up any obvious place to start though... - Fully on board with the advice in the last section. I only used jQuery elements because I needed to have continuous interval type data and know of no other method to elicit it. \$\endgroup\$
    – Deepend
    Jun 22, 2015 at 16:16
  • \$\begingroup\$ I would usually 100% agree with you regarding accessibility and screen readers but this is an experiment to judge tiny variations in different types of images relating to the design/layout of a website. I will ensure however that any work relating to content will be accessible in future. \$\endgroup\$
    – Deepend
    Jun 22, 2015 at 16:16
3
\$\begingroup\$

Overall Design

One thing that bothers me about this is that unless you have different types of sliders you probably should only have one slider form that you then use for each of the different sliders. This will reduce code duplication.

I see that everything here is a ModelForm that goes to Person this is a bit concerning, what if someone does a survey more than once? Do you duplicate their personal information every time they fill in a form? What if their personal information changes in between filling in forms?. I'd imagine that I would approach this by making a model for personal information and a model for the survey responses. The survey responses model would then have a foreign key to a personal information model which would be responsible for associating a person with a filled in survey.

There's other ways of approaching this, but that's a quick outline of a way that I saw that you could do this differently.

Imports

from django.forms.extras import SelectDateWidget
from django.forms.extras import *

If you want to import things you should do them explicitly. Doing it this way is insidious because the first line suggests you are only importing SelectDateWidget but then you just import everything anyway.

I'd clean up the imports so they are logically grouped:

from django.core import validators
from django.forms.extras import SelectDateWidget
from django.forms.util import ErrorList
from django import forms
from django.utils.encoding import force_unicode

from survey.models import Person

from itertools import chain
import random

Note how I removed the line with from django.forms import ModelForm, Textarea, extras, RadioSelect, IntegerField. Pick one import style and stick with it. You have things like forms.ModelForm but then RadioSelect. Why not just make this forms.RadioSelect and keep it all consistent, then at a glance we know by convention that RadioSelect was from the Django forms.

Checking for invalid values

if value is None: value = ''

If the value is None why not just leave it that way and handle it when you need to?

if choices[0][0] == '':

The more pythonic way to do this is to check existence like so:

if choices[0][0]:

Docstrings

This:

# Used to make RadioSelect non manditory and remove default option in the ModelForm 
# (http://stackoverflow.com/questions/30218930/how-to-make-a-radioselect-non-manditory-and-remove-default-option-in-a-modelform) 
class RadioSelectNotNull(RadioSelect):

can become:

class RadioSelectNotNull(RadioSelect):
    """Used to make RadioSelect non mandatory and remove default option in the ModelForm.
See (http://stackoverflow.com/questions/30218930/how-to-make-a-radioselect-non-manditory-and-remove-default-option-in-a-modelform) """
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the feedback, some great points. No it is the same type of slider, just different images being rated. But I'm not really sure how I would use the same slider form for the different sliders? How will this save the data into different columns in the DB? I definitely get your second point about having a separate Person model and Survey Response model, makes perfect sense now that you point it out. Fortunately in this instance each person 'should' compete the survey once. I will look into changing this. \$\endgroup\$
    – Deepend
    Jun 22, 2015 at 15:04
  • \$\begingroup\$ I will look into my imports now. There are a few of these which are definitely not being used. And i get the point about not using *. Ill have to look into the Checking for invalid values, I got this code from a question I asked about "How to make a RadioSelect non manditory and remove default option in a ModelForm?" stackoverflow.com/a/30261518/1214163 . Yes I should be using Docstrings. \$\endgroup\$
    – Deepend
    Jun 22, 2015 at 15:18

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.