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

• Dan: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .

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':

• SuperBiasedMan: Your ternary for extension isn't even necessary though. The original code doesn't use 'jpg' in an else case. You could just have extension = self.image_format.

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

• Benj: Please see revised answer above.