Object oriented architecture with an image processing code

Question

This code is python 3X code (in django). It takes an instance of user entry, and process images: it creates a resized version of original image, 2 thumbs, saves all, and update instance fields to point to correct files so it's correctly saved. Then it returns the updated instance.

I'm not sure this code object oriented architecture is correct. It seems lots of line of redundant code. Could I have made it smarter? I tried to break code on several methods that doe only one thing. Then I process it with the method call 'execute' which is used elsewhere in the script when this code is used.

Also what do you think of encapsulation of this code?

class ImageProcessing:
    def __init__(self, instance, image):
        self.instance = instance
        self.image = image

        # Settings
        self.sizes                    = OrderedDict()
        self.sizes['image_main']      = (800, 800)
        self.sizes['thumbsize_big']   = (200, 200)
        self.sizes['thumbsize_small'] = (100, 100)
        self.png_compress = 6
        self.jpg_compress = 80
        self.has_iterated = 0
        # Variables
        self.slug = slugify(instance.myField, allow_unicode=True)
        self.saving_path = instance.savePath
        self.base_dir = settings.BASE_DIR + self.saving_path
        self.image_format = self.image.image.format.lower()
        has_iterated = 0

    def resize_image(self, image, size):
        myimage = Image.open(image)
        myimage = myimage.resize(size, Image.ANTIALIAS)
        return myimage

    def save_image(self, image, save_dir, png_compress, jpg_compress):
        if self.image_format == 'png':
            image.save(save_dir + '.png', compress_level=png_compress, format='PNG')
        if self.image_format == 'jpg' or 'jpeg':
            image.save(save_dir + '.jpg', quality=jpg_compress, format='JPEG')

    def execute(self):
        for key, value in self.sizes.items():
            save_dir = self.base_dir + self.slug + '_' + key

            if self.image_format == 'png':
                # Save images on disk
                self.save_image(self.resize_image(self.image, value),save_dir, self.png_compress, self.jpg_compress)
                # Save images in fields
                if self.has_iterated == 0: self.instance.image_main = self.saving_path + self.slug + '_' + key + '.png'
                if self.has_iterated == 1: self.instance.image_thumbsize_big = self.saving_path + self.slug + '_' + key + '.png'
                if self.has_iterated == 2: self.instance.image_thumbsize_small = self.saving_path + self.slug + '_' + key + '.png'
                self.has_iterated += 1
            if self.image_format == 'jpg' or 'jpeg':
                self.save_image(self.resize_image(self.image, value),save_dir, self.png_compress, self.jpg_compress)
                if self.has_iterated == 0: self.instance.image_main = self.saving_path + self.slug + '_' + key + '.jpg'
                if self.has_iterated == 1: self.instance.image_thumbsize_big = self.saving_path + self.slug + '_' + key + '.jpg'
                if self.has_iterated == 2: self.instance.image_thumbsize_small = self.saving_path + self.slug + '_' + key + '.jpg'
                self.has_iterated += 1

        return self.instance

Answer 1

Reducing repetition

self.sizes

self.sizes being repeated 4 times is not necessary:

    self.sizes                    = OrderedDict()
    self.sizes['image_main']      = (800, 800)
    self.sizes['thumbsize_big']   = (200, 200)
    self.sizes['thumbsize_small'] = (100, 100)

You may give an initial argument to OrderedDict to avoid it:

    self.sizes = OrderedDict( (
        ('image_main', (800, 800)),
        ('thumbsize_big', (200, 200)),
        ('thumbsize_small', (100, 100)) 
    ))

myimage

def resize_image(self, image, size):
    myimage = Image.open(image)
    myimage = myimage.resize(size, Image.ANTIALIAS)
    return myimage

In fact the myimage variable may be avoided completely, just chaining the method calls is much simpler:

def resize_image(self, image, size):
    return Image.open(image).resize(size, Image.ANTIALIAS)

Whole block repetition

        if self.image_format == 'png':
            # Save images on disk
            self.save_image(self.resize_image(self.image, value),save_dir, self.png_compress, self.jpg_compress)
            # Save images in fields
            if self.has_iterated == 0: self.instance.image_main = self.saving_path + self.slug + '_' + key + '.png'
            if self.has_iterated == 1: self.instance.image_thumbsize_big = self.saving_path + self.slug + '_' + key + '.png'
            if self.has_iterated == 2: self.instance.image_thumbsize_small = self.saving_path + self.slug + '_' + key + '.png'
            self.has_iterated += 1
        if self.image_format == 'jpg' or 'jpeg':
            self.save_image(self.resize_image(self.image, value),save_dir, self.png_compress, self.jpg_compress)
            if self.has_iterated == 0: self.instance.image_main = self.saving_path + self.slug + '_' + key + '.jpg'
            if self.has_iterated == 1: self.instance.image_thumbsize_big = self.saving_path + self.slug + '_' + key + '.jpg'
            if self.has_iterated == 2: self.instance.image_thumbsize_small = self.saving_path + self.slug + '_' + key + '.jpg'
            self.has_iterated += 1

The blocks are very similar just the file-extension changes.

extension = 'png' if self.image_format == 'png' else 'jpg'

And then you can delete the conditional branches that follow and leave only one path.

Bug on the use of or

    if self.image_format == 'jpg' or 'jpeg':

Does not work as you intended. or returns the first truthy value, so you wrote the same as:

    if self.image_format == 'jpg':

Answer 2

You have a block name # Settings in your __init__, but really these look like constants. None of them are affected by the parameters passed to __init__ or any other factors. If you want a class to have constants, you should instead put them as class attributes:

class ImageProcessing:

    SIZES = OrderedDict( (
        ('image_main', (800, 800)),
        ('thumbsize_big', (200, 200)),
        ('thumbsize_small', (100, 100)) 
    ))
    PNG_COMPRESS = 6
    JPG_COMPRESS = 80

These can still be accessed with self.SIZES, or alternatively ImageProcessing.SIZES. The added benefit is you don't need to recreate these values each time, they only need to be created once when the class is created.

You also set both self.has_iterated = 0 and has_iterated = 0 the latter is redundant, you can remove it.

In both save_image and execute you don't do anything for a file that's neither jpg or png. This ought to raise a ValueError rather than do nothing. Even if you want to silently ignore invalid files when using this, the functions should raise errors so that when this is used in feature people have to consciously choose to ignore those errors. Also, passing png_compress and jpg_compress is redundant when you have those as attributes on the class:

def save_image(self, image, save_dir):
    if self.image_format == 'png':
        image.save(save_dir + '.png', compress_level=self.PNG_COMPRESS, format='PNG')
    elif self.image_format in ('jpg', 'jpeg'):
        image.save(save_dir + '.jpg', quality=self.JPG_COMPRESS, format='JPEG')
    else:
        raise ValueError("Invalid filetype '{}'".format(self.image_format))

Don't put if condition: execute line all on one line, it's confusing and hard to read. More whitespace makes code more readable and easier to follow.

# Save images on disk
self.save_image(self.resize_image(self.image, value), save_dir,
                self.png_compress, self.jpg_compress)
# Save images in fields
if self.has_iterated == 0:
    self.instance.image_main = self.saving_path + self.slug + '_' + key + '.png'
if self.has_iterated == 1:
    self.instance.image_thumbsize_big = self.saving_path + self.slug + '_' + key + '.png'
if self.has_iterated == 2:
    self.instance.image_thumbsize_small = self.saving_path + self.slug + '_' + key + '.png'
self.has_iterated += 1