tags:

views:

164

answers:

4

I have a class hierarchy:

class ParentClass:

    def do_something(self):
        pass # child classes have their own implementation of this

class ChildClass1(ParentClass):

    def do_something(self):
        <implementation here>

class ChildClass2(ParentClass):

    def do_something(self, argument_x):
        <implementation here>

class ChildClass3(ParentClass):

    def do_something(self, argument_y):
        <implementation here>

There are two problems here:

  • method do_something() has different interfaces in subclasses: it accepts an argument in child classes 2 and 3, but has no argument in child class 1
  • the arguments of do_something() have different names to emphasize that they have different meanings in child classes 2 and 3. This will become clearer below, from the usage example

This is how the classes are used:

There is a factory class that returns instances:

class ChildFactory:

    def get_child(self, argument):
        if argument == '1':
            return ChildClass1()
        elif argument == '2':
            return ChildClass2()
        elif argument == '3':
            return ChildClass3()

later in code:

...
# pseudocode, not python
child_type = ? # can have values '1', '2' or '3' at this moment
var1 = 1
var2 = 'xxx'
# var1 and var2 have different types, just to emphasize the difference in their
# meaning when being passed as arguments to do_something()
# this was mentioned above (the second problem)
child = ChildFactory.get_child(child_type)
if child is an instance of ChildClass1, child.do_something() is called
if child is an instance of ChildClass2, child.do_something(var1) is called
if child is an instance of ChildClass3, child.do_something(var2) is called
# end of pseudocode

Questions:

  1. Are the two problems mentioned above a sign of a bad design? If so, what's the correct way to design the hierarchy?
  2. How to write the pseudocode snippet uniformly in python? The main concern is to avoid using a huge if/else statement for each particular case, because it will double the if/else statement from ChildFactory.get_child()
+2  A: 

Abstract questions like this are very difficult to answer. It would be much easier to answer if we knew what problem you were trying to solve. I can tell you that usually it is a bad sign to see something like:

if isinstance(obj, cls1):
    ...
elif isinstance(obj, cls2):
    ...

Usually, it means you should be defining a new method instead of using if/elif. In Python, you can define methods outside the class definition if you like, if this helps. It is also usually a bad sign if two interchangeable classes have methods with the same name but take a different number of arguments, it means that the classes are not truly interchangeable. Either the different methods should take the same arguments or they should have different names. It is also unnecessary to define methods that are never called, such as do_something in ParentClass -- this is something you see in programmers that come to Python from C++/Java/C#.

Dietrich Epp
I also see the option of making do_something() accept two arguments with default values, but code checkers like pylint will then complain that arguments that are passed into the function are not used. Hence the dilemma.
Alex_coder
`*args, **kwargs` could be used for the variable argument count, but I agree with you that this smells like bad design.
Georg
There should be a way to tell pylint that you intend for the variables to be unused. Don't use a bizzare control structure just to make pylint happy.
Dietrich Epp
+1 because it usually IS a bad sign in Python to be concerned about the particular instance name...
Francesco
A: 

You could do this, to make the signatures the same:

class ParentClass:
    pass

class ChildClass1(ParentClass):

    def do_something(self, **kwargs):
        <implementation here>

class ChildClass2(ParentClass):

    def do_something(self, **kwargs):
        argument_x = kwargs[argument_x]
        <implementation here>

class ChildClass3(ParentClass):

    def do_something(self, **kwargs):
        argument_y = kwargs[argument_y]
        <implementation here>

The factory could just be a dict:

childfactory = {1:ChildClass1, 2:ChildClass2, 3:ChildClass3}

then later:

...
# pseudocode, not python
child_type = ? # can have values '1', '2' or '3' at this moment
var1 = 1
var2 = 'xxx'
# var1 and var2 have different types, just to emphasize the difference in their
# meaning when being passed as arguments to do_something()
# this was mentioned above (the second problem)
child = childfactory[child_type]()
child.do_something(argument_x=val1, argument_y=var2)
# end of pseudocode
pwdyson
+1  A: 

If things are to be used interchangeably, they should have the the same interface. For a method, this means the same number of arguments with the same meanings and the same names in the same order. If they do not behave exactly the same, simply give them different names and make it look like they are interchangable.

Mike Graham
A: 

Methods with the same name and different arguments are a code smell.

"method do_something() has different interfaces in subclasses: it accepts an argument in child classes 2 and 3, but has no argument in child class 1"

You don't say why. There are two good reasons why

  • child class 1 has a default value.

  • child class 2 ignores the value.

Almost any other reason indicates that the do_something is truly different and should have a different name.

If child class 1 has a default value, then simply code a default value explicitly in the arguments to the method function.

class ChildClass1( ParentClass ):
    def do_something( argument_x= None )
        ....

If child class 1 ignores the value, then simply ignore the value. Don't stand on your head to ignore a vlaue.

class ChildClass1( ParentClass ):
    def do_something( argument_x )
        return True

There's no magic about a polymorphic method function that doesn't happen to use all argument values.

"the arguments of do_something() have different names to emphasize that they have different meanings in child classes 2 and 3."

This is just bad design. You can't have the same method function with different argument names because they do different things.

Having the same method function name is just wrong. If they are different implementations of similar things, then the arguments will have essentially the same meanings.

If they actually do different things, then you don't have polymorphism, and you should not be giving these methods the same names.

When the methods in two classes do fundamentally different things -- requiring different arguments with distinct names to make that obvious -- those methods must not have the same names. The name ceases to have meaning when it doesn't describe what the method actually does.

Note

Your code, BTW, will work in Python because of duck typing. As long as the method names match, the argument types do not have to even come close to matching. However, this is really poor design because the essential differences among the methods are so huge.

S.Lott
Consider classes that implement cards. A card class has a method that accepts an argument (another card) and returns a boolean value. True, if it can beat that card. A joker can beat any other card, its method will always return True and doesn't require an input argument. Not sure how suitable this example is. The point is that there is no default value at all for ChildClass1.self() in my example.
Alex_coder
@Alex_coder: The card comparison for `Joker` *still* accepts an argument. It just doesn't use the argument. `def beats( self, aCord): return True`. The card comparison is the *meaning* of the method, even though it ignores the argument.
S.Lott
Thanks for a clear explanation. Your statement about the meaning is the best AHA candidate for now. Does this mean that in such cases it is wise to not treat pylint warnings as imperatives, just silence them and let the common sense prevail? I can see the advantage: the code becomes uniform if I'll ignore the little redundancy issue on purpose.
Alex_coder
@Alex_coder: "little redundancy issues"? You mean unused argument warnings? Yes, you have to silence that to have proper polymorphism.
S.Lott