0
\$\begingroup\$

The goal of the following short Python 3 program is to improve my understanding of the decorator design pattern. Therefore, I would be mostly interested about feedback on this aspect, i.e. whether this is a correct usage of mentioned pattern. Also, I would like some comments regarding Python coding best practices, i.e. if there are ways to make my code more "Pythonesque". Of course, any other hints and remarks (e.g., related to the chosen use case, image editing), are welcome too.

#!/usr/bin/python3

import PIL.Image, PIL.ImageFilter, PIL.ImageOps


class IImage:
  def get_img_obj(self):
    raise NotImplementedError("cannot call method of IImage.getImgArr on the interface")

class BaseImage(IImage):
  def __init__(self, img_obj):
    self.img_obj = img_obj

  def get_img_obj(self):
    return self.img_obj


class ImageDecorator(IImage):
  def get_img_obj(self):
    raise NotImplementedError("cannot call method of ImageDecorator.getImgArr on the base class")
  def __init__(self, img):
    self.img = img


class BlurFilter(ImageDecorator):
  def __init__(self, img, radius):
    ImageDecorator.__init__(self, img)
    self.radius = radius
  def get_img_obj(self):
    imageIternal = self.img.get_img_obj()
    return imageIternal.filter(PIL.ImageFilter.GaussianBlur(self.radius))


class ImageFlipper(ImageDecorator):
  def __init__(self, img):
    ImageDecorator.__init__(self, img)
  def get_img_obj(self):
    imageIternal = self.img.get_img_obj()
    return PIL.ImageOps.flip(imageIternal)



im_raw = PIL.Image.open("lena.gif")
im_raw = im_raw.convert('L')
im_obj = BaseImage(im_raw)
im_obj_flipped = ImageFlipper(im_obj)
im_obj_flipped_blurred = BlurFilter(im_obj_flipped, 5)


im_obj.get_img_obj().show("original")
im_obj_flipped.get_img_obj().show("flipped")
im_obj_flipped_blurred.get_img_obj().show("flipped and blurred")
\$\endgroup\$
3
\$\begingroup\$

First of all, since Python is dynamically typed, there is not really a need for interfaces. That’s why there is no such language concept. So your IImage type does not really serve any need.

Similarly, the base type ImageDecorator is not really that useful. In statically typed language you are using these design patterns, and ultimately such a base type, because you need to interact with some common base type. Since we have duck typing in Python, this is not really necessary.

Following the Zen of Python, “Simple is better than complex”; and looking at those classes you wrote, it looks somewhat complex to me considering what it does. Compare it with the following solution:

#!/usr/bin/python3
import PIL.Image, PIL.ImageFilter, PIL.ImageOps

def blur(image, radius):
    return image.filter(PIL.ImageFilter.GaussianBlur(radius))

def flip(image):
    return PIL.ImageOps.flip(image)

im_obj = PIL.Image.open('lena.gif')
im_obj = im_raw.convert('L')
im_obj_flipped = flip(im_obj)
im_obj_flipped_blurred = blur(im_obj_flipped, 5)

im_obj.show('original')
im_obj_flipped.show('flipped')
im_obj_flipped_blurred.show('flipped and blurred')

I think this is a lot more to the point, and it shows exactly what’s going on without unnecessarily wrapping the image in an object that does not really serve any purpose.

Of course, I don’t know if your BaseImage is actually needed for something; but if it’s just what you’re showing here, then it’s just a wrapper that doesn’t do anything on its own. You should avoid creating types that don’t have any behavior, especially if they only contain a single object. It’s much easier to just use that actual object then instead.

That being said, if the BaseImage actually is more complicated, then you can still keep your filters simple:

def blur(image, radius):
    rawImage = image.get_img_obj()
    modifiedImage = rawImage.filter(PIL.ImageFilter.GaussianBlur(radius))
    return BaseImage(modifiedImage)

def flip(image):
    rawImage = image.get_img_obj()
    modifiedImage = PIL.ImageOps.flip(rawImage)
    return BaseImage(modifiedImage)

Now you retain your image type, even after the transformation.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your insights (I do have more C++/Java background, so I tend to think along their concepts :) ). That said, I like especially your second code sample, because it still retains all the intermediary states, without the need for classes. \$\endgroup\$ – Attilio Jul 24 '16 at 14:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.