1
\$\begingroup\$

Situation: I have implemented a factory design pattern in Python as below. There are different screens which have different configurations. One can register the screen in the factory and get the screen in case correct configuration is passed to respective factory class method.

Question: I am wondering if this is a clean solution as each screen class (Screen1, Screen2, ..) has a main-method which calls in a specific order the respective private class methods. Is there a better/cleaner way to handle the main methods in each class? Is it cleaner to implement a combination of factory and builder design pattern?

Highly appreciate any comments and improvements!!

Code:

import numpy as np
import pandas as pd

from abc import ABCMeta, abstractmethod 

df = pd.DataFrame({"ident": ["A1", "A2", "B3", "B4"], "other_col": np.random.randint(1, 6, 4)})


class IScreens(metaclass=ABCMeta):

    @abstractmethod
    def main(self):
        pass 
    

class Screen1(IScreens):
    
    def __init__(self, data, config):
        self.data = data
        self.batch = config["cfg1"]    
    
    def __get_letter(self):
        self.data["campaign"] = self.data[self.batch].str[:1]

        return self.data
    
    def __get_number(self):
        self.data["num"] = self.data[self.batch].str[1:]

        return self.data
    
    def __some_other_stuff(self):
        self.data["other_stuff"] = self.data["num"].astype(int) * 100 / 2

        return self.data
    
    def main(self):
        self.data = self.__get_letter()
        self.data = self.__get_number()
        self.data = self.__some_other_stuff()
        # some more processing steps follow 

        return self.data
    
    
class Screen2(IScreens):
    
    def __init__(self, data, config):
        self.data = data
        self.batch = config["cfg1"]    
    
    def __some_cool_stuff(self):
        self.data["cool_other_stuff"] = self.data[self.batch] + "_COOOOL"

        return self.data
    
    def main(self):
        self.data = self.__some_cool_stuff()
        # some more processing steps follow 
        
        return self.data
        

class ScreenFactory:

    def __init__(self):
        self._screens = {}

    def register_screen(self, screen_name, screen_class):
        self._screens[screen_name] = screen_class
    
    def get_screen(self, data, config):
        
        if "screen_indicator" not in config:
            raise AssertionError("Your config file does not include 'screen_indicator' key")
        
        screen = self._screens.get(config["screen_indicator"])
        if not screen:
            raise AssertionError("screen not implemented")
        return screen(data=data, config=config)
    

factory = ScreenFactory()

factory.register_screen(screen_name="s1", screen_class=Screen1)
config = {"screen_indicator": "s1", "cfg1": "ident"}
factory.get_screen(data=df, config=config).main()

factory.register_screen(screen_name="s2", screen_class=Screen2)
config = {"screen_indicator": "s2", "cfg1": "ident"}
factory.get_screen(data=df, config=config).main()
```
\$\endgroup\$
1
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Commented Mar 26, 2021 at 15:19

2 Answers 2

1
\$\begingroup\$

IScreens is a callable

Actually, your IScreens class (interface) contains only one abstract method (the main method), so this class is a kind of callable. Instead of creating a specific class, you can use collections.Callable:

import collections

IScreens = collections.Callable

To use that class, you can do:

class Screen1(IScreens):
    def __init__(self, data, config):
          ...

    def __call__(self, *args, **kwargs):  # instead of `main`
        return ...

s1 = Screen1(data=..., config=...)
result = s1()  # call the callable s1

IScreens can contain common data

I see that the data and config variables are common to Screen1 and Screen2. It's a good practice to factorize this in the base class:

class IScreens(collections.Callable):
    def __init__(self, data, config):
        self.data = data
        self.batch = config["cfg1"]

NOTE: Here, I don't understand why you pass a config dictionary, but only get the batch (config["cfg1"]).

Avoid extra complexity

Private methods with double underscores, like __get_letter, are more difficult to debug, because their names are disguised into _{className}__{attr_name}.

For instance:

class C:
    def __init__(self):
        self.__secret = "foo"

obj = C()
print(obj._C__secret)
# 'foo'

Very short functions like __get_letter should be avoided, unless reused several times. Here, you add complexity (and extra computation time) with them.

Getters should not mutate the instance

A getter should not change the class instance state.

Here, in your __get_letter method, you are extracting a value (self.data[self.batch].str[:1]) and modifying the internal state (self.data["campaign"]). Is it intentional?

It is better to initialize everything in your constructor:

class Screen1(IScreens):
    def __init__(self, data, config):
        self.data = data
        self.batch = config["cfg1"]
        self.data["campaign"] = self.data[self.batch].str[:1]
        self.data["num"] = self.data[self.batch].str[1:]
        self.data["other_stuff"] = self.data["num"].astype(int) * 100 / 2

Factory registration

There are two different ways to implement factories:

  1. The factory (or the main function) knows all its classes and is responsible for the class registration,
  2. Each class knows the factory and it can auto-register itself, like a plugin.

It seems that you choose the first case. You may also need to add a method to unregister. In the second case, registration is usually final: its more difficult to unregister…

Both are good, so.

To register a class in a factory, we used to use the class name (or a class instance) instead of an arbitrary name.

Example using the class name:

class ScreenFactory:
    def __init__(self):
        self._screens = {}

    def register_screen(self, screen_class):
        self._screens[screen_class.__name__] = screen_class

    def get_screen_class(self, class_name: str):
        return self._screens[class_name]

Example using the class attribute:

class IScreens:
    kind = "unknown"


class Screen1(IScreens):
    kind = "s1"


class ScreenFactory:
    def __init__(self):
        self._screens = {}

    def register_screen(self, screen_class):
        self._screens[screen_class.kind] = screen_class

    def get_screen_class(self, kind: str) -> IScreens:
        return self._screens[kind]

The second method is interesting if you want to select an implementation (a IScreens class) according to a tag/label extracted from textual data…

NOTE: you may have noticed that I added a get_screen_class method.

Factory access

This is the difficult part

In your get_screen method, you are checking if a specific configuration is valid (existence of "screen_indicator") and if there is a suitable class which satisfy a second condition (config["screen_indicator"] is a kind of IScreens).

I am OK with that, but it can be simplified. Here is a solution with error handling:

class ScreenNotImplementedError(Exception):
    fmt = "Screen not implemented for this configuration: {config}"

    def __init__(self, *, config):
        msg = self.fmt.format(config=config)
        super(ScreenNotImplementedError, self).__init__(msg)


class ScreenFactory:
    ...

    def get_screen(self, data, config):
        try:
            kind = config["screen_indicator"]  # may raise KeyError
            cls = self.get_screen_class(kind)  # may raise KeyError
        except KeyError:
            raise ScreenNotImplementedError(config=config)
        else:
            screen = cls(data=data, config=config)
            return screen
\$\endgroup\$
1
  • \$\begingroup\$ Thanks really much for your comments! As I am writing data-pipelines using pandas (which need to be highly configurable and adaptable to say applications only with small requirement changes among them) I was moving away from procedural code and try to go with OOP. can you recommend well-written OOP code using pandas (Github repo etc) or a book/blog entry for teaching? \$\endgroup\$
    – rkarft
    Commented Apr 8, 2021 at 12:27
1
\$\begingroup\$

It's fine for a class to include "helper" methods to implement more complex methods. It's just like breaking down a complex function into simpler functions. If the helper methods are not part of the class' public interface, it is a convention to use a single leading underscore in the name.

I like to use __init_subclass__() in the base class to automatically maintain a registry of subclasses and then provide a classmethod in the base class to act as a factory function. __init_subclass__() gets called after the code defining a subclass is run (so, at compile time). It gets passed the new subclass, which the code below uses to register the subclass with the subclass name. __init_subclass__() is a classmethod even without the @classmethod decorator. The classmethod from_config() is an example factory method that instantiates an appropriate subclass based arguments passed to it. Here it is based on the subclass name, but it could be based on other factors, calculations, etc.

class Screen(metaclass=ABCMeta):
    registry = {}
    
    def __init_subclass__(cls):
        if cls.__name__ not in Screen.registry:
            Screen.registry[cls.__name__] = cls
        else:
            raise RuntimeError('Screen subclass "{cls.__name__}" already defined.')

            
    def __init__(self, data, config):
        self.data = data
        self.config = config
        
        
    @classmethod
    def from_config(cls, data, config):
        if "screen_indicator" not in config:
            raise ValueError("config missing 'screen_indicator' key")
        
        screen = Screen.registry.get(config["screen_indicator"])
        if not screen:
            raise NameError(f"""No such screen: '{config["screen_indicator"]}'""")
            
        return screen(data=data, config=config)
        
        
    @abstractmethod
    def main(self):
        pass 
    
    
class Screen1(Screen):
    
    def _do_stuff(self):
        self.data[0] *= 2
    
    def main(self):
        self._do_stuff()
        return self.data
    
    
class Screen2(Screen):
        
    def _do_more_stuff(self):
        self.data[0] *= 3
    
    def main(self):
        self._do_more_stuff()
        return self.data


print(Screen.registry)

data=[42, 'what?']

config = {"screen_indicator": "Screen1", "cfg1": "ident"}
d1 = Screen.from_config(data=data, config=config).main()
print(d1)

config = {"screen_indicator": "Screen2", "cfg1": "ident"}
d2 = Screen.from_config(data=data, config=config).main()
print(d2)

config = {"screen_indicator": "Screen3", "cfg1": "ident"}
d3 = Screen.from_config(data=data, config=config).main()
print(d3)

Output:

{'Screen1': <class '__main__.Screen1'>, 'Screen2': <class '__main__.Screen2'>}
[84, 'what?']
[252, 'what?']

NameError: No such screen: 'Screen3'  ## Note: I removed the stack trace

If you need to use something other than the class name as the indicator, you can use a keyword argument in the subclass definition like so:

class Screen(metaclass=ABCMeta):
    registry = {}
    
    def __init_subclass__(cls, indicator=''):  #### keyword gets passed in here
        if indicator:
            Screen.registry[indicator] = cls
        else:
            raise ValueError('Class subclass "{cls.__name__}" missing "indicator" argument')
        
    @classmethod
    def from_config(cls, data, config):
        if "screen_indicator" not in config:
            raise ValueError("config missing 'screen_indicator' key")
        
        screen = Screen.registry.get(config["screen_indicator"])
        if not screen:
            raise NameError(config["screen_indicator"])
            
        return screen(data=data, config=config)
        
    @abstractmethod
    def main(self):
        pass 
    

class Screen1(Screen, indicator='s1'):  #### set the indicator here
    
    def __init__(self, data, config):
        self.data = data
        self.config = config
        
    def _do_stuff(self):
        self.data[0] *= 2
    
    def main(self):
        self._do_stuff()
        return self.data
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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