HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythondjangoMinor

Optimising and condensing Django View Class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
djangoviewoptimisingcondensingandclass

Problem

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

Solution

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.

Code Snippets

if x == y:
    # ...
if x == z:  # note: z != y
    # ...
if x == y:
    # ...
elif x == z:
    # ...
elif x == w:
    # ...
if x in (1, 2, 3):
    if x == 1:
        # ...
    elif x == 2:
        # ...
    else:  # must be inevitably 3
        # ...
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]

Context

StackExchange Code Review Q#64451, answer score: 2

Revisions (0)

No revisions yet.