4
\$\begingroup\$

I have a tool that displays data in some format. For example, it can display a list of dogs in a CSV or JSON formats. In the code, I had a lot of places where I had to check the format and display the data in that format. The only difference between the code blocks is the calls to the data. Some of them call get_name() and get_id() while others call get_type() and so on.

I tried to create an abstract class that has the display method and the derived classes have the get_block (for JSON), get_line (for CSV) and so on. The code looks like:

class DisplayingFormatEnum(Enum):
    OPTION_A = 'user-option-a'
    OPTION_B = 'user-option-b'


class DisplayingFormat(ABC):
    def get_row(data):
        pass

    def get_block(data):
        pass

    def get_line(data):
        pass

    def display_format_table(self):
        table = PrettyTable(self.title)
        for d in self.data:
            table.add_row(self.get_row(d))
        print(table)

    def print_json(self):
        result = []
        for d in self.data:
            block = self.get_block(d)
            result.append(block)
        print(result)

    def display_format_dsv(self):
        for d in self.data:
            print(f"{self.get_line(d)}")

    def display_format_list(self):
        if self.result_format == 'table':
            self.display_format_table()
        elif self.result_format == 'json':
            self.display_format_json()
        else:
            self.display_format_dsv()


class DisplayingFormatPeople(DisplayingFormat):
    def __init__(self, people, result_format, enum_type):
        if not isinstance(enum_type, DisplayingFormatEnum):
            raise DisplayingFormatError("Entity must be an instance of DisplayingFormatEnum Enum")

        if enum_type.value == DisplayingFormatEnum.OPTION_A.value:
            self.title = TITLE_OPTION_A['people']
        else:
            raise DisplayingFormatError(f"DisplayingFormatPeople can only use {DisplayingFormatEnum.OPTION_A.value} as a valid enum_type. Invalid enum_type: {enum_type.value}")

        self.result_format = result_format
        self.data = people
        self.enum_type = enum_type

    def get_row(self, person):
        return [person.get_name(), person.get_id()]

    def get_block(self, person):
        block = {}
        block[self.title[0]] = person.get_name()
        block[self.title[1]] = person.get_id()
        return block

    def get_line(self, person):
        return ';'.join((person.get_name(), person.id()))


class DisplayingFormatCars(DisplayingFormat):
    def __init__(self, cars, result_format, enum_type):
        if not isinstance(enum_type, DisplayingFormatEnum):
            raise DisplayingFormatError("Entity must be an instance of DisplayingFormatEnum Enum")

        if enum_type.value == DisplayingFormatEnum.OPTION_A.value:
            self.title = TITLE_OPTION_A['cars']
        else:
            raise DisplayingFormatError(f"DisplayingFormatProjects can only use {DisplayingFormatEnum.OPTION_A.value} as a valid enum_type. Invalid enum_type: {enum_type.value}")

        self.result_format = result_format
        self.data = cars
        self.enum_type = enum_type

    def get_row(self, car):
        return [car.get_type()]

    def get_block(self, car):
        block = {}
        block[self.title[0]] = car.get_type()
        return block

    def get_line(self, car):
        return car.get_type()


class DisplayingFormatDogs(DisplayingFormat):
    def __init__(self, dogs, result_format, enum_type):
        if not isinstance(enum_type, DisplayingFormatEnum):
            raise DisplayingFormatError("Entity must be an instance of DisplayingFormatEnum Enum")

        if enum_type.value == DisplayingFormatEnum.OPTION_A.value:
            self.title = TITLE_OPTION_A['dogs']
        elif enum_type.value == DisplayingFormatEnum.OPTION_B.value:
            self.title = TITLE_OPTION_B['dogs']
        else:
            error_line  = "DisplayingFormatDogs can only use {DisplayingFormatEnum.OPTION_A.value} and {DisplayingFormatEnum.OPTION_B.value} as valid entities."
            error_line += "Invalid enum_type: {enum_type.value}"
            raise DisplayingFormatError(f"{error_line}")

        self.result_format = result_format
        self.data = dogs
        self.enum_type = enum_type

    def get_row(self, dog):
        return [dog.get_nickname(), dog.get_type()]

    def get_block(self, dog):
        block = {}
        block[self.title[0]] = dog.get_nickname()
        block[self.title[2]] = dog.get_type()
        return block

    def get_line(self, dog):
        return ';'.join((dog.get_nickname(), dog.get_type()))


class DisplayingFormatError(Exception):
    pass

I believe that this code is a good use of inheritance but I think it does not fit well with the "composition over inheritance" rule. I'm afraid of unexpexted future changes that will require me to change this code. How should I change my code to follow this rule?

\$\endgroup\$
1
  • \$\begingroup\$ Please edit your question so that the title describes the purpose of the code, rather than its mechanism. We really need to understand the motivational context to give good reviews. Thanks! \$\endgroup\$ Dec 4, 2019 at 15:32

1 Answer 1

3
\$\begingroup\$

I'm not convinced using an abstract base class is really useful here. All of your methods follow exactly the same principles. The only differences are the names and fields and the allowed enumeration values.

So as a first step I would put all the code into the baseclass, and only add the check for the enumeration into the constructor and implement the get_data method in each subclass:

class DisplayingFormat:
    def __init__(self, data, result_format, enum_type):
        if not isinstance(enum_type, DisplayingFormatEnum):
            raise DisplayingFormatError("Entity must be an instance of DisplayingFormatEnum Enum")
        self.result_format = result_format
        self.data = data
        self.enum_type = enum_type
        self.title = None

    def get_data(self, obj):
        raise NotImplementedError

    def to_table(self):
        table = PrettyTable(self.title)
        for d in self.data:
            table.add_row(self.get_data(d))
        return table

    def to_json(self):
        return [dict(zip(self.title, self.get_data(d))) for d in self.data]

    def to_dsv(self):
        return [';'.join(map(str, self.get_data(d))) for d in self.data]

    def display_format_list(self):
        if self.result_format == 'table':
            print(self.to_table())
        elif self.result_format == 'json':
            print(self.to_json())
        else:
            print(self.to_dsv())


class DisplayingFormatPeople(DisplayingFormat):
    def __init__(self, people, result_format, enum_type):
        super().__init__(people, result_format, enum_type)

        if enum_type.value == DisplayingFormatEnum.OPTION_A.value:
            self.title = TITLE_OPTION_A['people']
        else:
            raise DisplayingFormatError(f"DisplayingFormatPeople can only use {DisplayingFormatEnum.OPTION_A.value} as a valid enum_type. Invalid enum_type: {enum_type.value}")

    def get_data(self, person):
        return person.get_name(), person.get_id()


class DisplayingFormatCars(DisplayingFormat):
    def __init__(self, cars, result_format, enum_type):
        super().__init__(cars, result_format, enum_type)

        if enum_type.value == DisplayingFormatEnum.OPTION_A.value:
            self.title = TITLE_OPTION_A['cars']
        else:
            raise DisplayingFormatError(f"DisplayingFormatProjects can only use {DisplayingFormatEnum.OPTION_A.value} as a valid enum_type. Invalid enum_type: {enum_type.value}")

    def get_data(self, car):
        return car.get_type()

class DisplayingFormatDogs(DisplayingFormat):
    def __init__(self, dogs, result_format, enum_type):
        super().__init__(cars, result_format, enum_type)

        if enum_type.value == DisplayingFormatEnum.OPTION_A.value:
            self.title = TITLE_OPTION_A['dogs']
        elif enum_type.value == DisplayingFormatEnum.OPTION_B.value:
            self.title = TITLE_OPTION_B['dogs']
        else:
            error_line  = f"DisplayingFormatDogs can only use {DisplayingFormatEnum.OPTION_A.value} and {DisplayingFormatEnum.OPTION_B.value} as valid entities."
            error_line += f"Invalid enum_type: {enum_type.value}"
            raise DisplayingFormatError(error_line)

    def get_data(self, dog):
        return dog.get_nickname(), dog.get_type()

Note that I also fixed some un-/needed f-strings and used list comprehensions where applicable. I also modified and renamed your format methods so they return the formatted object and are only printed in display_format_list, which might need a better name as well.

This could be simplified further. The easiest way is if you can modify the objects in data to have an implemented __getitem__ method, but you can also hack it together using getattr. What you really only need for each subclass are the field names and the association between the option and the title:

class DisplayingFormat:
    fields = []
    titles = {}
    def __init__(self, data, result_format, enum_type):
        if not isinstance(enum_type, DisplayingFormatEnum):
            raise DisplayingFormatError("Entity must be an instance of DisplayingFormatEnum Enum")
        if enum_type not in self.titles:
            raise DisplayingFormatError(f"enumy_type must be in {[opt.value for opt in self.titles]}. Invalid enum_type: {enum_type.value}")
        self.result_format = result_format
        self.data = data
        self.title = self.titles[enum_type]

    def get_data(self, obj):
        # if you implement __getitem__
        return [obj[f] for f in self.fields]
        # or this, if you cannot modify your objects interface
        return [getattr(obj, f"get_{f}")() for f in self.fields]

    ...

class DisplayingFormatPeople(DisplayingFormat):
    fields = ["name", "id"]
    titles = {DisplayingFormatEnum.OPTION_A: TITLE_OPTION_A['people']}

class DisplayingFormatCars(DisplayingFormat):
    fields = ["type"]
    titles = {DisplayingFormatEnum.OPTION_A: TITLE_OPTION_A['dogs']}

class DisplayingFormatDogs(DisplayingFormat):
    fields = ["nickname", "type"],
    titles = {DisplayingFormatEnum.OPTION_A: TITLE_OPTION_A['dogs']
              DisplayingFormatEnum.OPTION_B: TITLE_OPTION_B['dogs']}

If you really want to go the route of composition, you would probably want to make the formatter an argument, so that you have only one class with this interface:

class Display:
    def __init__(self, data, title, formatter):
        ...

where taking care of selecting the proper columns to go to data as well as selecting the proper title is all external to the class. The formatter then takes the role of the to_json, ... methods. And at that point it doesn't even need to be a class anymore, a function would suffice.

\$\endgroup\$
1
  • \$\begingroup\$ I don't get how to_json and to_dsv work. What if get_data returns only one string and not tuple? it prints something like p;e;r;s;o;n. \$\endgroup\$
    – vesii
    Dec 10, 2019 at 23:55

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.