2
\$\begingroup\$

I am trying to learn OOP by refactoring my existing code (functions only). This example works but I am not sure whether it is pythonic and good practice.

I have a shared library which consist of several file with functions:

color_spaces.py

import cv2

def rgb_to_hsv(image):
    print ('hsv')
    hsv_image = cv2.cvtColor(image, cv2.COLOR_RGB2HSV)
    hue, sat, val = hsv_image[:, :, 0], hsv_image[:, :, 1], hsv_image[:, :, 2]
    return hsv_image, hue, sat, val

def rgb_to_lab(image):
    lab_image = cv2.cvtColor(image, cv2.COLOR_RGB2LAB)
    l_chan, a_chan, b_chan = lab_image[:, :, 0], lab_image[:, :, 1], lab_image[:, :, 2]
    return lab_image, l_chan, a_chan, b_chan

thresholding.py

import cv2


def treshold_otsu(gray_image):
    ret, thresholded_image = cv2.threshold(gray_image, 0, 255, cv2.THRESH_BINARY_INV + cv2.THRESH_OTSU)
    return thresholded_image

def global_threshold(gray_image):
    ret, thresholded_image = cv2.threshold(gray_image, 127, 255, cv2.THRESH_BINARY)
    return thresholded_image

My class which uses functions of the shared library:

pipline01.py

import cv2
from color_spaces import rgb_to_hsv, rgb_to_lab
from thresholding import treshold_otsu, global_threshold

class Segmentation1(object):
    def __init__(self, filename, path, procedure):
        self.filename = filename
        self.path = path
        self.procedure = procedure

    def start_pipline(self):
        # Read image
        image = cv2.imread(self.path + self.filename)

        # Do segmentation procedure depending on procedure
        if self.procedure == 'test01':
            source = rgb_to_hsv(image)[1]
            binary = treshold_otsu(source)
        elif self.procedure == 'test02':
            source = rgb_to_lab(image)[1]
            binary = global_threshold(source)

        # Save image
        cv2.imwrite(self.path + self.filename.split('.')[0] + '_' + self.procedure + '.png', binary)

Then I call the class in a batch mode by looping over a folder with images:

batch_pipline01.py

import os
import argparse
from pipline01 import Segmentation1

parser = argparse.ArgumentParser(description = 'Test.')
parser.add_argument('-d', '--img_dir', required = True,
                    help = 'directory containing images to segment.')
parser.add_argument('-p', '--procedure', required = True,
                    help = 'procedure to run.')

args = parser.parse_args()
print(args)
img_dir = args.img_dir
procedure = args.procedure
images = os.listdir(img_dir)
#images = [img[2:] for img in args.images]

# Start segmentation for each image
for img in images:
    os.chdir(img_dir)
    print('SEGMENTING ' + img)
    segmenter = Segmentation1(img, img_dir, procedure)
    obj = segmenter.start_pipline()
    del obj

Can I improve my code and design?

\$\endgroup\$
3
\$\begingroup\$

This scenario is not best case for doing an OOP conversion: In this example you're not getting real assistance from the Segmentation1 class over what you'd get by using static functions. The code looks like it does what you need done; it's simply not a great application for OOP as written. At some point you should check out this talk by Python core dev Jack Diederich for some perspective on when OOP helps in Python and when it doesn't. One of the main takeways there is that classes which expose just one method ususally don't need to be classes.

That said:

If you want to use this as learning case, you can refactor your arrangement so that you have two different classes for the two different strategies. This is overkill in the code as is, but if you anticipated a much wider range of segmentation tasks with different inner workings it might make sense. It would be a good choice if the segmentation operation had multiple steps and needed to maintain state within a single run -- for example, if it had to collect data from multiple data sources and coordinate it differently for LAB colors and HSV colors.

Your existing code really has only two paths, splitting on the procedure. So that's a good clue that you want the sibling classes to differ primarily according to the differences -- the rule is to "encapsulate what varies" -- and, conversely, to share the parts that are the same. The file handling and so on are the same. So first you can isolate the unchanging bits into mini-methods of their own, and then bookend them around the parts that will change:

class SegmentationType(object):
    DISPLAY_NAME = "invalid"

    def __init__(self, filename, path):
        self.filename = filename
        self.path = path
        self.input_data = None
        self.output_data = None


    def read_image(self):
        self.input_data =  cv2.imread(self.path + self.filename)[1]

    def write_image(self):
        cv2.imwrite(self.path + self.filename.split('.')[0] + '_' + self.DISPLAY_NAME + '.png', self.output_data)

    def process(self):
        # override in derived classes to perform an actual segmentation
        pass

    def start_pipeline(self):
        self.read_image()
        self.process()
        self.write_image()

Then you just need to create subclasses for your strategies:

class HSV_Segmenter(SegmentationType):
    DISPLAY_NAME = 'HSV'

    def process(self):
        source = rgb_to_hsv(self.input_data)
        self.output_data = treshold_otsu(source)


class LabSegmenter(SegmentationType):
    DISPLAY_NAME = 'LAB'

    def process(self):
        source = rgb_to_lab(self.input_data)
        self.output_data = global_threshold(source)

This should be equivalent to your earlier code -- it does have one minor advantage, which is that it will fail at compile time if you don't pick the proper subclass where the original would fail at runtime if you accidentally pass a bad value for procedure. Then your program simply chooses the right class and calls it:

segmenter_class = {
    'hsv': HSV_Segmentation,
    'lab': LAB_Segmenter
}.get(procedure)

if not segmenter_class:
    raise ArgumentError("Invalid segmentation method '{}'".format(procedure))

for img in images:
    os.chdir(img_dir)
    processor =  = segmenter_class(img, img_dir, procedure)
    processor.start_pipeline()

Again, I personally would stick with static functions in this situation. In Python you could achieve all the good parts of the class arrangement by just having that dictionary lookup return the right static functions instead of classes. Lots of languages don't have that neat function and so this kind of structure is very common there.

\$\endgroup\$
  • \$\begingroup\$ That is such an awesome answer and example, I am very thankful for your efforts. I slowly see light in the darkness may I ask two more questions? You are right, the two classes look like overkill but I kept the example by purpose very simple for my understanding. The real world code would have many subclasses, one for each strategy and of course not only two steps inside the process method. So I think your example will work very well with my code. The reason I want to refactor was that I started to write a lots of duplicated code because most strategies overlap and differ only in some steps. \$\endgroup\$ – snowflake Aug 15 '18 at 7:43
  • 1
    \$\begingroup\$ There is a small error inside the LabSegmenter, could you please change def start_pipline(self): to def process(self):. Thanks! \$\endgroup\$ – snowflake Aug 15 '18 at 7:51
  • 1
    \$\begingroup\$ for Q1: yep -- To oversimplify a bit, static functions are better for any situation that can be handled with a static function alone. Not only do you save the work of creating and managing useless class instances, you're better positioned to do things like composing functions together if there are no classes in the way \$\endgroup\$ – theodox Aug 15 '18 at 17:48
  • 1
    \$\begingroup\$ for Q2: the model here is that we're using a class to make sure lots of similar operations share the boring parts (eg, file handling) and all look the same outside. If they all have the same interface it is easy to delegate work to them. \$\endgroup\$ – theodox Aug 15 '18 at 17:51
  • 1
    \$\begingroup\$ first one - if it's the same most of the time, only override it when the behavior needs to change. Otherwise you're just duplicating code to no effect. I think the responsibilities are reasonable ; one is a subset of the other (which is how this example supports the strategy pattern) but each has a clearly defined single job. If you don't allow composition it's hard to write concise code, or your callers have to write a lot of copy-paste boilerplate. \$\endgroup\$ – theodox Nov 1 '18 at 5:34

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.