2016-10-01 5 views
2

ノート:私はクラス階層(パイソン)を持って、tidyは方法の一つである:これは推奨あたりこのタイプの属性値/関数のチェックは、Pythonのコード臭ですか?

前提として、コードレビューにcrosspostedです。これはタイプASTIgnoreのノードを削除し、そのノードの子をその親ノードに再バインドします。

ターゲットノードは自分自身を削除することはできません。の親(再バインド用)を参照してください。したがって、ターゲット(タイプASTIgnoreタイプ)の削除は、その親で行われます。ここで、親はその子のタイプをチェックします。

質問:コードの臭いを軽減するには、これをどのように実装する必要がありますか?

これらのアプローチの中で最も悪いのはどれですか、他のアプローチはどちらですか(下を参照)。

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

# B) 
if child.isIgnored(): 

# C) 
if child.isIgnoreType: 

# D) 
if isinstance(child, ASTIgnore): 

、クラスと tidyは、以下のようになります。最もクリーンな実装に基づいて、冗長性を削除します。知らせる-聞かない方針でダックタイピングの問題について

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 

私は、Pythonに新しいです、そしてニシキヘビコーダ(と一般的にはより良いコーダーになりたいです)。したがって、

どうすればダックタイプ上記はありますか?属性値(igIgnoreType)/関数(isIgnored)のチェックはダックタイピングと見なされますか?アトリビュート/関数がオブジェクト構築を超えて決して触られない場合は、

tidyには、がオーバーロードされています。タイプのノードは無視されます。 タイプチェックはもうありませんが、親は引き続きターゲットの子を削除し、孫を再バインドする必要があります。ここでは、Ignore型は子を返します。リーフノードの場合は[]になります。しかし、返品がNoneかどうかのチェックがまだあります。これは確かにダックタイピングですが、Noneとコード複製、不良コードをチェックしていますか?

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

Eric'sの投票に基づいて、isIgnored()機能チェックの実装は

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 
+0

IMO 'isIgnored()'はあなたのコードを最も理解しやすくします。その余分な標識を持つことは、繰り返しコードを持つ第2の密な関数よりも読者にとっては簡単です。 –

+0

投稿コードレビュー:私は答えを得る機会を得るでしょう。 –

+0

[Crossposted](http://codereview.stackexchange.com/questions/142983/is-this-type-of-attribute-value-function-checking-a-code-smell-in-python)これはCodeReviewにあります。 Ericの投票実装が追加されました。 –

答えて

1

のようになります。私はtidyメソッドからの戻り値を使用することが良い方法だと思いますノード間で情報を渡します。とにかく子供のそれぞれにtidyを呼び出すつもりです。したがって、その子と何をするべきかを示す戻り値を得ることで、コード全体が簡単になります。あなたはsuperが少しあるのPython 2を、使用している場合

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

:あなたは派生クラスから基本クラスの実装を呼び出すためにsuperを使用して、ちょうど戻り値を変更することで、自分自身の繰り返しを避けることができ

Python 3よりも少し魔法がかかりにくいので、super()の代わりにsuper(ASTIgnore, self)を使用する必要があります。

+0

これは 'tidy'メソッドではうまく機能します。私はこのテクニックを念頭に置いています。 **抽象化:**それ以外にも、多態性の側面全体から大きな利益を得る( 'stringify'、' print_tree'、 'emitPredicate'、' transform_ControlFlow'など)いくつかのサブクラスがそれをオーバーロードする可能性があります。つまり、メソッド操作は実際にはそれを呼び出すタイプに依存します。しかし、Single Responsibility Principleについて読んだ後、私は心配しています。これはSRPの総違反ですか? –

+0

私はこの責任において、単元責任原則は重要ではないと思います。これは主に大規模な設計(モジュールレベルなど)であり、すべてのクラスに適用する必要はありません。あなたの 'ASTNode'サブクラスは、常に互いに密接に結びついていると思います。デザインの変更があれば、クラスは通常一度に変更されるため、SRPの問題はあまりありません。 – Blckknght

+0

そうですよ! 'ASTNode'サブクラスのほとんどは単純に' pass'ですが、新しいデータ属性で基底を拡張するものもあります。コアメソッドをクラスから分離しようとすると、あるタイプの_typeチェックが必要です(例えば、 'isIgnored()'、 'isControl()'など)。 ** ___ **私はSOLIDのすべてのこととPythonの慣用句についてもっと詳しく読む必要があります。ありがとう!私は基本クラス( '__init__'、' tidy')への明示的な呼び出しを 'super'に置き換えました。 –

関連する問題