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.

• Deepend: Thanks for your answer. Its very helpfull. The only part I dont understand is images[step - 5] = image = random.choice(path_one_images) and images[3 + step - 9] = image = random.choice(path_one_images) Could you explain whats going on here please?
• janos: It's to derive the index in the images array based on the step number. For example: 5 => 0, 6 => 1, 7 => 2, and then 9 => 3, 10 => 4, and so on.
• Deepend: Hi Janos, I am having an issue with the above code. I thought I had it working but it is now giving me an error Exception Value: global name 'images' is not defined When i define images then the array wont update. Any ideas? Thanks

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.