3

クラスの設計を改善し、型チェックを回避するためのリファクタリングに関するアドバイスを探しています。

Command デザイン パターンを使用してメニュー ツリーを作成しています。メニューの項目にはさまざまな種類があります (たとえば、即時アクション [「保存」など]、状態に応じてチェック/アイコンを表示するトグル オン/オフ プロパティ [「斜体」など] など)。重要なことに、画面上の現在のメニューを (横に表示するのではなく)置き換えるサブメニューもあります。もちろん、これらのサブメニューには独自のメニュー項目のリストが含まれており、さらにネストされたサブメニューを持つことができます。

コードは次のようなものです（プレゼンテーションを簡単にするためにすべて公開されています）：

// 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;
};

メイン メニューは Menu の単なるインスタンスであり、サブメニューに出入りする方法を含め、別のクラスがメニューの位置を追跡します。

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;
};

重要な関数は Position::OnEnterPressed() です。ここでは、MenuItem::IsMenu() への呼び出しで明示的な型チェックが行われ、派生型にキャストされます。タイプチェックとキャストを回避するためにこれをリファクタリングするためのいくつかのオプションは何ですか?

4

5 に答える 5

4

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、それをリファクタリングする必要があることを示しています。

基本クラスのメソッドで (1) 因数分解し、派生クラスでそれをオーバーライドして、特定の動作 (2) を考慮することができる場合は、これを に入れることができますExecute。

間違っている場合は訂正してください。メニューにはアイテムがあり、各アイテムには、何らかのイベントが検出されたときにトリガーされるアクションが関連付けられているという考えです。

ここで、選択したアイテムがサブメニューの場合、Executeアクションにはサブメニューをアクティブにするという意味があります (一般的な意味でアクティブを使用しています)。アイテムがサブメニューでない場合Executeは、別の獣です。

私はあなたのメニューシステムを完全に理解していませんが、一種の階層メニュー/サブメニュー (位置) と、ノードの種類に応じてトリガーされるいくつかのアクションがあるようです。

私が思い描いているのは、メニュー/サブメニューの関係は、リーフ ノード (サブメニューがない場合) と非リーフ ノード (サブメニュー) を定義できる階層であるということです。リーフ ノードはアクションを呼び出し、非リーフ ノードはサブメニューのアクティブ化を処理する別の種類のアクションを呼び出します (このアクションはメニュー システムに戻るため、メニュー システムに関する知識をカプセル化せず、単にアクションをメニューシステムに中継します)。

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

于 2011-08-04T16:47:40.770 に答える
2

別の方法として、Menu をスタックにプッシュできるようにするメソッドを Position に公開し、Menu:Execute の開始時にそのメソッドを呼び出すこともできます。次に、 OnEnterPressed の本体は次のようになります

(*m_pos.back().iter)->Execute();
于 2011-08-04T16:43:22.623 に答える
1

必要なのは、「アクションまたはメニューのいずれか」を表現する機能です。これは、アクションとメニューのインターフェイスが大きく異なる場合、ポリモーフィズムを使用して記述するのが非常に面倒です。

それらを共通のインターフェース（サブメニューメソッドの名前としては不適切です）に強制しようとする代わりにExecute、私はあなたよりも先に進んでを使用しますdynamic_cast。

また、常にdynamic_castフラグや。よりも優れています。アクションは、サブメニューではないことを世界に伝える必要はありません。static_cast

最も慣用的なC++で書き直された、これは次のコードを与えます。私std::listはその便利な方法のためspliceに使用し、イテレータを無効insertにremoveしません（リンクリストを使用するいくつかの正当な理由の1つ）。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を参照してください（tl; dr：古いコンパイラを使用しない限り、古いイテレータを接続して保持してもかまいません）。]

于 2011-08-04T17:33:50.757 に答える
1

これはおそらくあなたが探していた応答ではありませんが、私の意見では、あなたのソリューションは型チェックを含まないソリューションよりもはるかに優れています。

ほとんどの C++ プログラマーは、オブジェクトをどう処理するかを決定するためにオブジェクトの型をチェックする必要があるという考えに腹を立てています。しかし、Objective-C や最も弱い型付けのスクリプト言語などの他の言語では、これが実際に強く推奨されます。

あなたの場合、の機能には型情報が必要なので、型チェックの使用が適切に選択されていると思いますPosition。この機能をMenuItemサブクラスの 1 つに移動すると、IMHO は能力の分離に違反します。Positionメニューの表示とコントローラーの部分に関係しています。なぜモデルクラスがそれを気にする必要があるのMenuか​​ わかりません。MenuItem型チェックなしのソリューションに移行すると、オブジェクト指向の観点からコードの品質が低下します。

于 2011-08-04T16:47:21.387 に答える
0

言語はすでにこのメカニズムを提供していますdynamic_cast。ただし、より一般的な意味で、設計に固有の欠陥は次のとおりです。

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

これは Execute() 関数に入れ、必要に応じてリファクタリングしてそれを実現する必要があります。

于 2011-08-04T16:46:04.503 に答える