リファクタリングのアドバイス：このOOデザインの型チェックを回避する方法

Question

私はクラスデザインを改善し、型チェックを避けるためにリファクタリングに関するアドバイスを探しています。

私はコマンドデザインパターンを使用してメニューツリーを構築しています。メニュー内のアイテムは、様々なタイプ（例えば、「保存」のような即時アクション、その状態に応じてチェック/アイコンで表示されるトグルオン/オフプロパティ[イタリック体]など）であってもよい。重大なことに、サブメニューもあります。は、画面上の現在のメニューを（代わりに表示するのではなく）に置き換えます。これらのサブメニューには、独自のメニュー項目のリストが含まれています。これは、より多くのネストされたサブメニューを持つ可能性があります。

コードは（プレゼンテーションの簡略化のためのすべてのパブリック）のようなものです：

// Abstract base class 
struct MenuItem 
{ 
    virtual ~MenuItem() {} 
    virtual void Execute()  = 0; 
    virtual bool IsMenu() const = 0; 
}; 

// Concrete classes 
struct Action : MenuItem 
{ 
    void Execute() { /*...*/ } 
    bool IsMenu() const { return false; } 
    // ... 
}; 

// ... other menu items 

struct Menu : MenuItem 
{ 
    void Execute() { /* Display menu */ } 
    bool IsMenu() const { return true; } 
    // ... 
    std::vector<MenuItem*> m_items; 
    typedef std::vector<MenuItem*>::iterator ItemIter; 
}; 

メインメニューはメニューのほんのインスタンスであり、別々のクラスが行く方法など、メニューの位置を追跡しますサブメニューのうち、：

struct Position 
{ 
    Position(Menu* menu) 
    : m_menu(menu) 
    { 
    // Save initial position 
    m_pos.push_back(MenuPlusIter(m_menu, m_menu->m_items.begin())); 
    } 

    // Ignore error conditions for simplicity 
    void OnUpPressed() { m_pos.back().iter--; } 
    void OnDownPressed() { m_pos.back().iter++; } 
    void OnBackPressed() { m_pos.pop_back(); } 

    void OnEnterPressed() 
    { 
    MenuItem* item = *m_pos.back().iter; 
    // Need to behave differently here if the currently 
    // selected item is a submenu 
    if(item->IsMenu()) 
    { 
     // dynamic_cast not needed since we know the type 
     Menu* submenu = static_cast<Menu*>(item); 

     // Push new menu and position onto the stack 
     m_pos.push_back(MenuPlusIter(submenu, submenu->m_items.begin())); 

     // Redraw 
     submenu->Execute(); 
    } 
    else 
    { 
     item->Execute(); 
    } 
    } 

private: 
    struct MenuPlusIter 
    { 
     Menu*   menu; 
     Menu::ItemIter iter; 

     MenuPlusIter(Menu* menu_, Menu::ItemIter iter_) 
      : menu(menu_) 
      , iter(iter_) 
     {} 
    }; 

    Menu* m_menu; 
    std::vector<MenuPlusIter> m_pos; 
}; 

キー機能を使用すると、MenuItemにへの呼び出しで明示的な型チェックを参照してください位置:: OnEnterPressed（）、:: IsMenu（）してから派生型へのキャストです。タイプチェックを避けてキャストするためにこれをリファクタリングするオプションは何ですか？

Answer 1

IMO、リファクタリングの出発点は、これらの文のようになります。

1. m_pos.push_back(MenuPlusIter(m_menu, m_menu->m_items.begin())); 

2. m_pos.push_back(MenuPlusIter(submenu, submenu->m_items.begin())); 

文の同じ種類自体を繰り返しているという事実は、IMO、それをリファクタリングする必要性のためのサインです。

基本クラスのメソッドでfactor（1）を行い、派生クラスでそれをオーバーライドして特定の動作（2）を考慮すると、Executeに入れることができます。

私が間違っていると私を訂正してください：アイデアはメニューに項目があり、各項目には何らかのイベントが検出されたときにトリガーされるアクションが関連付けられています。

ここで、選択した項目がサブメニューの場合、Executeアクションには意味があります。サブメニューをアクティブにします（私は一般的な意味でactivateを使用しています）。項目がサブメニューでない場合、Executeは別の獣です。

メニューシステムの完全な理解はありませんが、階層メニュー/サブメニュー（位置）の一種と、ノードの種類に応じてトリガーされるアクションがあります。

メニュー/サブメニューの関係は、リーフノード（サブメニューを持たない場合）と非リーフノード（サブメニュー）を定義できる階層です。リーフノードはアクションを呼び出し、非リーフノードはサブメニューのアクティブ化を扱う別の種類のアクションを呼び出す（このアクションはメニューシステムに戻るので、メニューシステムに関する知識をカプセル化しないアクションをメニューシステムに中継する）。

これがあなたにとって理にかなっているかどうかわかりません。

Answer 2

代わりに、メニューをスタックにプッシュできるメソッドを「位置」に公開し、メニュー：実行の開始時にそのメソッドを呼び出すこともできます。その後OnEnterPressedの体はちょうど言語は既に、これはそれがdynamic_castだmechanism-提供

(*m_pos.back().iter)->Execute(); 

Answer 3

になります。

m_pos.push_back(MenuPlusIter(submenu, submenu->m_items.begin())); 

それは、実行（）関数内で移動し、それを実現するために、必要に応じてリファクタリングする必要があります。しかし、より一般的な意味では、あなたの設計に固有の欠陥はこれです。

Answer 4

これはおそらくあなたが探していたレスポンスではありませんが、私の意見では、ソリューションはタイプチェックに関係しないソリューションよりはるかに優れています。

ほとんどのC++プログラマは、オブジェクトのタイプをチェックして、それをどうするかを決める必要があるという考えに惑わされています。しかし、Objective-Cやほとんどの弱い型付けされたスクリプト言語のような他の言語では、これは実際には非常に奨励されています。

あなたのケースでは、Positionの機能のタイプ情報が必要なので、タイプチェックを使用することをお勧めします。この機能をサブクラスのいずれかに移すと、IMHOは能力分離に違反します。 Positionは、メニューの表示とコントローラ部分に関係しています。私はなぜモデルクラスMenuまたはMenuItemがそれに関係しているのか分からない。型チェックのないソリューションに移行すると、オブジェクトの向きに関してコードの品質が低下します。

Answer 5

"アクションまたはメニューのいずれか"を表現する能力が必要です。アクションとメニューのインターフェイスが非常に異なる場合、多態性を使用して記述するのは非常に面倒です。

一般的なインターフェイス（Executeはサブメニュー方法の名前が悪い）に強制するのではなく、私はあなたより先に進み、dynamic_castを使用します。

また、dynamic_castは、であり、常にであり、フラグはstatic_castです。行動は、彼らがサブメニュではないことを世界に伝えるためにまともではありません。

最も慣用的なC++で書き直すと、次のようになります。便利なメソッド、insert、およびremoveのため、イテレータを無効にしないため（を使用します）。開いているメニューを追跡するためにstd::stackも使用します。

struct menu_item 
{ 
    virtual ~menu_item() {} 

    virtual std::string label() = 0; 
    ... 
}; 

struct action : menu_item 
{ 
    virtual void execute() = 0; 
}; 

struct submenu : menu_item 
{ 
    // You should go virtual here, and get rid of the items member. 
    // This enables dynamically generated menus, and nothing prevents 
    // you from having a single static_submenu class which holds a 
    // vector or a list of menu_items. 
    virtual std::list<menu_item*> unfold() = 0; 
}; 

struct menu 
{ 
    void on_up() { if (current_item != items.begin()) current_item--; } 
    void on_down() { if (++current_item == items.end()) current_item--; } 

    void on_enter() 
    { 
     if (submenu* m = dynamic_cast<submenu*>(current_item)) 
     { 
      std::list<menu_item*> new_menu = m->unfold(); 

      submenu_stack.push(submenu_info(*current_item, new_menu)); 

      items.splice(current_item, new_menu); 
      items.erase(current_item); 
      current_item = submenu_stack.top().begin; 

      redraw(current_item, items.end()); 
     } 

     else if (action* a = dynamic_cast<action*>(current_item)) 
      a->execute(); 

     else throw std::logic_error("Unknown menu entry type"); 

     // If we were to add more else if (dynamic_cast) clauses, this 
     // would mean we don't have the right design. Here we are pretty 
     // sure this won't happen. This is what you say to coding standard 
     // nazis who loathe RTTI. 
    } 

    void on_back() 
    { 
     if (!submenu_stack.empty()) 
     { 
      const submenu_info& s = submenu_stack.top(); 

      current_item = items.insert(items.erase(s.begin, s.end), s.root); 
      submenu_stack.pop(); 

      redraw(current_item, items.end()); 
     } 
    } 

    void redraw(std::list<menu_item*>::iterator begin, 
       std::list<menu_item*>::iterator end) 
    { 
     ... 
    } 

private: 
    std::list<menu_item*> items; 
    std::list<menu_item*>::iterator current_item; 

    struct submenu_info 
    { 
     submenu* root; 
     std::list<menu_item*>::iterator begin, end; 

     submenu_info(submenu* root, std::list<menu_item*>& m) 
      : root(root), begin(m.begin()), end(m.end()) 
     {} 
    }; 

    std::stack<submenu_info> submenu_stack; 
}; 

私はコードを単純にしました。何かが不明な場合はお気軽にお問い合わせください。

[spliceを行う際に、イテレータの無効化については、this question（TLを; DR：あなたがあまりにも古いコンパイラを使用していない提供古いイテレータをスプライスし、維持するためにOKです）を参照してください。]