CODEWITHSUNDEEP
pythontypecheckingduck-typing
Sonal Pinto
Sonal Pinto

Reputation: 23

Is this type of attribute value/function checking, a code smell in Python?

note: This is crossposted on CodeReview, as per recommendation

Premise: I have a class hierarchy (Python), where tidy is one of the methods. It removes nodes that are of type ASTIgnore, and rebinds the children of that node to its parent.

The target node cannot delete itself, and doesn't see its parent (for rebinding). Thus, the deletion of the target (of type ASTIgnore) would happen at its parent, where the parent checks the type of its children.

Question: How would this need to be implemented to reduce the code smell?

Which among these approaches is least bad, or are there other ones (see bottom)?

# A)
if child.nodetype == "ASTIgnore":

# B)
if child.isIgnored():

# C)
if child.isIgnoreType:

# D)
if isinstance(child, ASTIgnore):

Where, the classes and tidy looks like the below. I'll remove the redundancy, based on the cleanest implementation.

class ASTNode(object):
    def __init__(self):
        self.nodetype = self.__class__.__name__
        self.isIgnoreType = False

    def isIgnored(self):
        return False

    def tidy(self):
        # Removes "Ignore" type/attribute nodes while maintaining hierarchy
        if self.children:
            for child in self.children:
                child.tidy()

            for i, child in reversed(list(enumerate(self.children))):
                #--------- Is this Bad? ----------
                if child.nodetype == "ASTIgnore":
                #------ --------------------------
                    if not child.children:
                        # leaf node deletion
                        self.children.pop(i)
                    else:
                        # target node deletion + hierarchy correction
                        grandkids = child.children
                        self.children[i:i+1] = grandkids


class ASTIgnore(ASTNode):
    def __init__(self):
        ASTNode.__init__()
        self.isIgnoreType = True

    def isIgnored(self):
        return True

On matter of Duck Typing with a Tell-Not-Ask policy:

I am new to Python, and would like to be a pythonic coder (and a better coder in general). Hence,

How do I Duck Type the above? Would checking the attribute value(igIgnoreType)/function(isIgnored) be considered Duck Typing if the attribute/function is never touched beyond object construction?

I do have another implementation, where tidy is overloaded in Ignore type nodes. No more type checking, but the parent still has to remove target child, and rebind grandkids. Here, the Ignore types return their children, which would be [] for leaf nodes. But, there is still a check on if the return was None. I am sure this is certainly Duck Typing, but is checking for None and code-replication, bad code?

class ASTNode(object):
    def tidy(self):
        for i, child in reversed(list(enumerate(self.children))):
            grandkids = child.tidy()
            if grandkids is not None:
                self.children[i:i+1] = grandkids

        return None

class ASTIgnore(ASTNode):
    def tidy(self):
        for i, child in reversed(list(enumerate(self.children))):
            grandkids = child.tidy()
            if grandkids is not None:
                self.children[i:i+1] = grandkids

        return self.children

_edit0

Based on Eric's vote, a isIgnored() function check implementation would look like

def tidy(self):
    """
    Clean up useless nodes (ASTIgnore), and rebalance the tree
    Cleanup is done bottom-top
      in reverse order, so that the deletion/insertion doesn't become a pain
    """
    if self.children:
        # Only work on parents (non-leaf nodes)
        for i, child in reversed(list(enumerate(self.children))):
            # recurse, so as to ensure the grandkids are clean
            child.tidy()

            if child.isIgnored():
                grandkids = child.children
                self.children[i: i + 1] = grandkids

Upvotes: 2

Views: 237

Answers (1)

Blckknght
Blckknght

Reputation: 104712

I think using the return value from the tidy method is a good way to pass information between your nodes. You're going to call tidy on each of your children anyway, so getting a return value that tells you what to do with that child makes the whole code simpler.

You can avoid repeating yourself by using super to call the base class's implementation from the derived class, and just changing the return value:

class ASTIgnore(ASTNode):
    def tidy(self):
        super().tidy() # called for the side-effects, return value is overridden
        return self.children

If you're using Python 2, where super is a little bit less magical than it is in Python 3, you'll need to use super(ASTIgnore, self) instead of super().

Upvotes: 1

Related Questions