CODEWITHSUNDEEP
python
AlGiorgio
AlGiorgio

Reputation: 497

Python. How to optimize set of classes?

I have following:

class DepartmentsSet(ComplexModel):
    __namespace__ = MODELS_NS

    service_difference_set = Iterable(Department)
    db_difference_set = Iterable(Department)
    intersection_difference_set = Iterable(Department)

    def __init__(self, service_diff, db_diff, intersection):
        self.service_difference_set = _convert_to_departments(service_diff)
        self.db_difference_set = _convert_to_departments(db_diff)
        self.intersection_set = _convert_to_departments(intersection)


def _convert_to_departments(operation_set):
    departments = []
    for item in operation_set:
        department = dict(item)
        departments.append(Department(
            name=department.get('name'),
            external_id=department.get('external_id')
        ))


class EmployeesSet(ComplexModel):
    __namespace__ = MODELS_NS

    service_difference_set = Iterable(Employee)
    db_difference_set = Iterable(Employee)
    intersection_set = Iterable(Employee)

    def __init__(self, service_diff, db_diff, intersection):
        self.service_difference_set = _convert_to_employees(service_diff)
        self.db_difference_set = _convert_to_employees(db_diff)
        self.intersection_set = _convert_to_employees(intersection)


def _convert_to_employees(operation_set):
    employees = []
    for item in operation_set:
        employee = dict(item)
        dep = employee.get('department')
        employees.append(Employee(
            name=employee.get('name'),
            post=employee.get('post'),
            department=(dep.name if isinstance(dep, CompanyDepartment) else u''),
            contact_info=employee.get('contact_info'),
            external_id=employee.get('external_id')))
    return employees

I have two questions:

  1. So, as you can see my classes DepartmentsSet and EmployeesSet both have similar structure. The only difference between them is that their sets consist of _convert_to_departments or _convert_to_employees. I realize that my solution is not good. What's the best practices for this case?

  2. Is there any way to unify functions _convert_to_departments and _convert_to_employees into general one?

*I can't make single class for DepartmentsSet and EmployeesSet as these classes would be types of my SOAP service.

Upvotes: 0

Views: 97

Answers (1)

LetMeSOThat4U
LetMeSOThat4U

Reputation: 6758

Addressing your direct questions is simple - move common code and variables to a superclass and inherit EmployeeSet and DepartmentSet from it like so:

class CommonModel(ComplexModel):

    def get_items(self, operation_set):
        items = [dict(x) for x in operation_set]
        return items

    # important: put COMMON methods, variables etc. here 

class DepartmentsSet(CommonModel):

    ...
    def _convert(self, operation_set):
        items = self.get_items(operation_set)
        for item in items:
            ... # convert to departments

class EmployeeSet(CommonModel):

    ...
    def _convert(self, operation_set):
        items = self.get_items(operation_set)
        for item in items:
            ... # convert to employees

However, strategically I feel that's not a good design really, if it's truly the case that "The only difference between them is that their sets consist of ", isn't DepartmentSet and EmployeeSet really the same thing? Could you not make it a single class with 2 conversion methods for instance?

That's still not quite good design though - you're mixing up collections and specific objects in a class. That's really a violation of Single Responsibility Principle. Make Employee or Department or CommonModel a single class and simply put their instances in lists. If you really, really need to convert somewhere provide a single object conversion method, and then you can do stuff like:

[x.convert() for x in list_of_model_instances]

..without even bothering to check what's the class of x (that's duck typing).

I still don't think you do really, it should be some other class or function that can take an instance of CommonModel on input and do whatever it wants with it. If you really need to do elaborate conversion, why not make a Converter class that can take a number of input instances and output whatever it is asked for? (that goes along Single Responsibility Principle)

BTW, if you do that you could make ComplexModel be an abstract base class or inherit from it, like so:

class AbstractItemCheckpoint(object):
    __metaclass__ = ABCMeta

    '''Abstract item checkpoint, derive concrete item checkpoints that verify if a item (Subsystem, Component,
       etc.) is eligible for creating a Task for it.'''

    @abstractmethod
    def convert(self):
        '''
        Ensure that model inheriting from AbstractItemCheckpoint has convert method.
        '''
        pass

This could ensure that any class like Employee or Department has to have convert method, thus making it safer to be submitted to instance of Converter.

Upvotes: 2

Related Questions