Optimising and condensing Django View Class

Question

This follows on from a previous question I asked which was of huge help to me. I am still only learning Python and Django and I would love to learn how to code better.

The below is a get_context_data method which is used to return a variable to an internal page of a Django SessionWizardView. The SessionWizardView splits a lengthy form over multiple pages.

The below is designed to show the user a different image, selected at random from path_one_images to the user. After the user views three such images they are then presented with the previous three on the forth page. This is repeated until all 9 images are seen by the user.

In the previous question I asked shortening the code proved to be a simple matter as each of my if statements were essentially doing the same thing. However In this case they are further nested, undertake different (but similar) functions and are interrupted by the multi views.

Has anyone got any suggestions for optimising, and preferably condensing the below code? Remember I am still learning so any suggestion would be helpful.

views.py

path_one_images = ['P1D1.jpg', 'P2D2.jpg', 'P3D3.jpg', 'P4D4.jpg', 'P5D5.jpg', 'P6D6.jpg', 'P7D7.jpg', 'P8D8.jpg', 'P9D9.jpg']


class SurveyWizardOne(SessionWizardView):                      
        def get_context_data(self, form, **kwargs):
            context = super(SurveyWizardOne, self).get_context_data(form, **kwargs)  
            if self.steps.current in ['5','6','7','8','9','10','11','12','13','14','15','16']:
                print '\nThe available list of Path_One images is', path_one_images

                global first_image, second_image, third_image, fourth_image, fifth_image, sixth_image, seventh_image, eight_image, ninth_image


                if self.steps.current == '5':
                    first_image = random.choice(path_one_images)          
                    path_one_images.remove(first_image)
                    display_image = first_image
                    context['display_image'] = display_image

                if self.steps.current == '6':
                    second_image = random.choice(path_one_images)    
                    path_one_images.remove(second_image)
                    display_image = second_image
                    context['display_image'] = display_image

                if self.steps.current == '7':
                    third_image = random.choice(path_one_images)              
                    path_one_images.remove(third_image)
                    display_image = third_image
                    context['display_image'] = display_image   


                if self.steps.current == '8':
                    context['first_image'] = first_image
                    context['second_image'] = second_image
                    context['third_image'] = third_image             


                if self.steps.current == '9':
                    fourth_image = random.choice(path_one_images)              
                    path_one_images.remove(fourth_image)
                    display_image = fourth_image
                    context['display_image'] = display_image  

                if self.steps.current == '10':
                    fifth_image = random.choice(path_one_images)    
                    path_one_images.remove(fifth_image)
                    display_image = fifth_image
                    context['display_image'] = display_image

                if self.steps.current == '11':
                    sixth_image = random.choice(path_one_images)              
                    path_one_images.remove(sixth_image)
                    display_image = sixth_image
                    context['display_image'] = display_image


                if self.steps.current == '12':
                    context['fourth_image'] = fourth_image
                    context['fifth_image'] = fifth_image
                    context['sixth_image'] = sixth_image 


                if self.steps.current == '13':
                    seventh_image = random.choice(path_one_images)          
                    path_one_images.remove(seventh_image)
                    display_image = seventh_image
                    context['display_image'] = display_image

                if self.steps.current == '14':
                    eight_image = random.choice(path_one_images)    
                    path_one_images.remove(eight_image)
                    display_image = eight_image
                    context['display_image'] = display_image

                if self.steps.current == '15':
                    ninth_image = random.choice(path_one_images)              
                    path_one_images.remove(ninth_image)
                    display_image = ninth_image
                    context['display_image'] = display_image

                if self.steps.current == '16':
                    context['seventh_image'] = seventh_image
                    context['eight_image'] = eight_image
                    context['ninth_image'] = ninth_image  

                steps = ['5','6','7','9','10','11','13','14','15']              

                context.update({'steps': steps})

            return context 

Answer 1

Use if-elif-else

Avoid these kind of if statements:

if x == y:
    # ...
if x == z:  # note: z != y
    # ...

Since x cannot be equal to two different values at the same time, you should use elif for the second and subsequent if statements on the same variable:

if x == y:
    # ...
elif x == z:
    # ...
elif x == w:
    # ...

The last elif is often just an else, if there can be no other possibility, for example here:

if x in (1, 2, 3):
    if x == 1:
        # ...
    elif x == 2:
        # ...
    else:  # must be inevitably 3
        # ...

Condensing

Instead of storing the image names in individual variables like first_image, second_image, ..., you should use an array, let's call it images. Then you can shorten a little bit:

step = int(self.steps.current)

if step in (5, 6, 7):
    images[step - 5] = image = random.choice(path_one_images)          
    path_one_images.remove(image)
    context['display_image'] = image

elif step == 8:
    context['first_image'] = images[0]
    context['second_image'] = images[1]
    context['third_image'] = images[2]             

elif step in (9, 10, 11):
    images[3 + step - 9] = image = random.choice(path_one_images)          
    path_one_images.remove(image)
    context['display_image'] = image

elif step == 12:
    context['fourth_image'] = images[3]
    context['fifth_image'] = images[4]
    context['sixth_image'] = images[5] 

... and so on, you can follow the same logic for steps 13 - 16.

And you can probably improve your HTML template too, to work with a list of images instead of hardcoded first_image, second_image, ..., and so on. Once you do that, you will be able to further simplify the Python part too, without the hardcoded context['first_image'], context['second_image'], ..., and so on.

Answer 2

Code as is

Constants should use upper case notation. path_one_images should be PATH_ONE_IMAGES. If it is not constants it should be assigned from constant.

One more hard-coded constanst can be found there:

if self.steps.current in ['5','6','7','8','9','10','11','12','13','14','15','16']:

display_image used, but it have no sense.

                display_image = second_image
                context['display_image'] = display_image

WOW, ...

global first_image, second_image, third_image, fourth_image, fifth_image,\
    sixth_image, seventh_image, eight_image, ninth_image

... where did they come from?

Proposals

IMHO: following blocks should not be part of view, it's can be some util that implements needed behaviour.

            if self.steps.current == '11':
                sixth_image = random.choice(path_one_images)              
                path_one_images.remove(sixth_image)
                display_image = sixth_image
                context['display_image'] = display_image

At least this code is called more than once, so it is reasonable to make a helper function for this purpose and make more reusable code. But I'm sure that you need few strategy classes and multiform manager to hold options and build flow using strategy instances.