Optimisation of a views.py for survey application

I have developed a small survey application in Django 1.6.2 which requires that the participants fill out a number of questions over 27 pages using a SessionWizardView.

Overview

The most unique feature of the application is that it allows the user to rate a selection of 9 images selected at random in isolation. The user then has to verify the rating they gave each image when they are re-shown them in groups of three.

At the beginning I have two instruction image rating tasks where I make it clear to the user what they have to do and two "Spike" questions to make sure that they are paying attention. Lastly I have two pages where all 9 images are shown again and they have to select their favorite and worst.

I implemented this using a get_context_data method of a SessionWizardView. Also included is my done method.

I previously posted a question about my models.py and there seems to be very few issues. I have also posted about my forms.py and am awaiting feedback. Its almost 400 lines of code (with logging comments so you might know what I was trying to do) but I thought it best to post it all.

I am completely self thought and have never programmed before and am just looking for feedback on how to improve my application. There is a lot of very obvious stuff I have no idea of.

views.py

```
class SurveyWizardOne(SessionWizardView):
def get_context_data(self, form, **kwargs):
context = super(SurveyWizardOne, self).get_context_data(form, **kwargs)
step = int(self.steps.current)

if step == 0:
self.request.session['path_one_images'] = ['P1D1.jpg', 'P2D2.jpg', 'P3D3.jpg', 'P4D4.jpg', 'P5D5.jpg', 'P6D6.jpg', 'P7D7.jpg', 'P8D8.jpg', 'P9D9.jpg']
self.request.session['instruction_task_one_images'] = ['IT1A.jpg', 'IT1B.jpg', 'IT1C.jpg']
self.request.session['instruction_task_two_images'] = ['IT2A.jpg', 'IT2B.jpg', 'IT2C.jpg']

Python specific

Docstrings

Documenting what your code does via some docstrings in your code can help a lot.
This is especially relevant when other people are reading your code (like now!).
In any case it's a habit that's worth getting into.

Checking for valid values

There are a few parts in your code where you do something like this:

if slider_value1 != '':

Presumably you are trying to check that slider_value1 has some sort of valid state before using it.

However, consider this code:

test = None

if test != '':
    print(test)
else:
    print("Invalid")

What do you think it will do? I suggest you run this.

The more pythonic way of dealing with this is to do the following:

if slider_value:
    #do something

Refactoring

Long functions

Something that immediately stands out to me is the length of the function. Generally speaking you don't want to have any function be longer than say 50 lines of code, if you start getting really long functions you should consider refactoring into smaller functions. While long functions are not always bad they are frequently indicative of other problems.

Specifically you have this big chain of elif statements that are of the form step = .... Make those into their own functions.

Duplicated code

In this particular over-long function we can find a bunch of duplicated code.
Generally speaking Python+Django has a philosophy called Don't repeat yourself

For example when you have:

if step == 0:
    logger.debug('\n\nThis is your Instruction Task One page')
    instruction_task_first_image = random.choice(INSTRUCTION_TASK_ONE_IMAGES)     
    context['display_image'] = instruction_task_first_image                                 

elif step == 1:                    
    logger.debug('\n\nThis is your Instruction Task Two page')
    instruction_task_second_image = random.choice(INSTRUCTION_TASK_TWO_IMAGES)   
    context['display_image'] = instruction_task_second_image

You really seem to be doing just about the same thing here but using different variable names. Very similar things happen in other cases of the various handlers for step here. When you see that you are doing the same thing more than once but just changing names it's a fairly good indicator that you should create a function to do the task and pass in the relevant changes via parameters.

def do_step_1_2(task_name, image_choices, context):
    """Does step 1 and 2,  returns the appropriate context"""
    logger.debug('\n\nThis is your Instruction Task {0} page'.format(task_name))
    context['display_image'] = random.choice(image_choices)

You most likely would want to choose a better name than this. Now you can call them like so:

if step == 0:
    do_step_1_2("One", INSTRUCTION_TASK_ONE_IMAGES, context)
elif step == 1:
    do_step_1_2("Two", INSTRUCTION_TASK_TWO_IMAGES, context)

Suddenly we are starting to get less repeated code.

We can take this a bit further again by making a dictionary that calls off the different functions:

steps_dispatcher = {
    1 : do_step_1_2,
    2 : do_step_1_2,
}

step_names = {
    1 : "One",
    2 : "Two",
}

image_sets = {
    1 : INSTRUCTION_TASK_ONE_IMAGES,
    2 : INSTRUCTION_TASK_TWO_IMAGES.
}

try:
    steps_dispatcher[step](step_names[step], image_sets[step], context)
except KeyError:
    logger.debug("Invalid step!")

This might seem like a bit of overkill for just 2 cases but you have a whole bunch of cases here (survey steps I'm assuming) and I think the time spent on it would be very worthwhile. This is because it will force you to look at what code is shared and will let you consolidate that code into functions. The reduction in maintenance time in the future is usually the way in which this ROI of doing this will manifest itself. When you need to make a change you can now just make it in one place without having to first try to hunt down all the places it could have occurred (and it's really easy to miss stuff especially if you are in a hurry).

There's a fair bit more that can be improved here but I think this is a good start. Try to clean it up and feel free to post a follow up question.

StackExchange Code Review Q#94184, answer score: 3