-2
\$\begingroup\$

I'm working with some code left from other developers. Here we have several projects we can work with. The project object is instantiated at the runtime. Then it used in other classes. There are no methods in Projects and the class holds pure configuration values.

Is subclassing a proper way to define configuration for different Projects? Isn't it better to create one class and get values from configuration file?

class BaseProject(metaclass=ABCMeta):
    """
    abstract class for projects
    concrete version of this needed only to keep transition IDs in case of custom workflow.
    """
    @property
    @abstractmethod
    def project_id(self):
        pass

    @property
    @abstractmethod
    def transition_id(self):
        pass

    @staticmethod
    def by_name(name):
        for prj in BaseProject.__subclasses__():
            if name.lower() == prj.__name__.lower():
                return prj()
        return Project3()

    @staticmethod
    def by_card_name(name):
        for prj in BaseProject.__subclasses__():
            if name.split("-")[0].lower() == str(prj.__name__).lower():
                return prj()
        return Project3()


# concrete projects
class Project1(BaseProject):
    project_id = 12334
    transition_id = 444


class Project2(BaseProject):
    project_id = 4451
    transition_id = 88


class Project3(BaseProject):
    project_id = 12346
    transition_id = 88

It looks good in term of open/closed principle. But I'm a little confused that values are in classes themself not in configuration.

BTW we don't expect any more projects in future and existing are hardly to change.

\$\endgroup\$

closed as off-topic by Toby Speight, BCdotWEB, IEatBagels, 200_success, yuri May 29 at 18:11

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Toby Speight, BCdotWEB, IEatBagels, 200_success, yuri
If this question can be reworded to fit the rules in the help center, please edit the question.

0
\$\begingroup\$

There is a very famous quote in software engineering: The only fixed thing in software is change.

So if it doesn't seem to you face any change in the future, that not means that you won't face any change in the future.

But there is something in your code. The instances of your class have no property for themselves. All values are static. This doesn't seem good. If you have a real object in your code that represents a project, so that object should has its own values.

So I think you should remove that static notations from your code and make those properties owned by instance. If it's applicable for you, then inheritance for creating different kinds of Projects are a really good thing to do.

But If you think you should not remove those static values, inheriting is still a good idea. Because you can pass different types of Projects in your code without modifying it.

\$\endgroup\$
  • \$\begingroup\$ Thank you. I think you right I also would like to move the attributes to the object itself. But my idea was to store values in configuration. Something like class Project: def __init__(self, project_name): self.project_id, self._transition_id = Config.get_project_config(project_name) \$\endgroup\$ – Dtit May 29 at 15:51
  • \$\begingroup\$ That is another way for doing this, But I think, in that case, it's also better that get_project_cofig returns an instance of Project. But if you want to do this, I think Project class is redundant. \$\endgroup\$ – Mr Alihoseiny May 30 at 12:06
  • 1
    \$\begingroup\$ Yes, in configuration I store them as named tuples and get_project_config just looks up a right one. By the some reason I think it's more readable. Thank you for helping me \$\endgroup\$ – Dtit May 30 at 16:10

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