Совет по рефакторингу: как избежать проверки типов в этой ОО-конструкции

Question

Я ищу совет по рефакторингу, чтобы улучшить дизайн моего класса и избежать проверки типов.

Я использую шаблон проектирования 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 () и затем приведение к производному типу. Какие варианты можно изменить, чтобы избежать проверки типа и приведения?

Answer 1

ИМО, отправной точкой рефакторинга будут следующие утверждения:

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

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

Тот факт, что подобные заявления повторяются, ИМО, является признаком необходимости их рефакторинга.

Если бы вы могли включить (1) в метод вашего базового класса, а затем переопределить его в производном классе, чтобы учесть конкретное поведение (2), то вы могли бы просто поместить это в Execute.

Поправьте меня, если я ошибаюсь: идея состоит в том, что у меню есть пункты, и с каждым элементом связано действие, которое запускается при обнаружении какого-либо события.

Теперь, когда выбранный вами элемент является подменю, действие Execute имеет значение: активировать подменю (я использую активацию в общем смысле). Когда предмет не является подменю, тогда Execute - это другой зверь.

У меня нет полного понимания вашей системы меню, но мне кажется, у вас есть своего рода иерархическое меню / подменю (позиции) и некоторые действия, которые запускаются в зависимости от типа узла.

Я предполагаю, что отношение меню / подменю - это иерархия, которая позволяет вам определять листовые узлы (когда у вас нет подменю) и нелистовые узлы (подменю). Конечный узел вызывает действие, а неконечный узел вызывает действие другого типа, которое связано с активацией подменю (это действие восходит к системе меню, поэтому вы не включаете в нее знания о системе меню, вы просто передать действие в систему меню).

Не знаю, имеет ли это для вас смысл.

Answer 2

Альтернативой может быть предоставление метода в Position, который позволяет помещать меню в стек, и вызов этого метода в начале Menu: Execute.Тогда тело OnEnterPressed просто становится

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

Answer 3

Что вам нужно, так это возможность выражать «либо действие, либо меню», что очень громоздко для написания с использованием полиморфизма, если действия и меню имеют очень разные интерфейсы.

Вместо того, чтобы пытаться принудительно ввести их в общий интерфейс (Execute - плохое название метода подменю), я бы пошел дальше, чем вы, и использовал бы dynamic_cast.

Также dynamic_cast всегда превосходит флаг и static_cast. Действия не говорят миру, что они не являются подменю.

Переписано в самом идиоматическом C ++, это дает следующий код. Я использую std::list из-за его удобных методов splice, 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, см. этот вопрос (tl; dr: все в порядке, чтобы склеивать и сохранять старые итераторы, если вы не используете слишком старый компилятор ).]

Answer 4

Вероятно, это не тот ответ, который вы искали, но, на мой взгляд, ваше решение намного превосходит любое решение, которое не включает проверку типов.

Большинство программистов на C ++ обижены идеей, что вам нужно проверить тип объекта, чтобы решить, что с ним делать. Тем не менее, в других языках, таких как Objective-C и большинство слабо типизированных языков сценариев, это на самом деле очень поощряется.

В вашем случае я думаю, что использование проверки типа хорошо выбрано, поскольку вам нужна информация о типе для функциональности Position. Перемещение этой функциональности в один из подклассов MenuItem ИМХО нарушило бы разделение компетенций. Position относится к части вашего меню для просмотра и управления. Я не понимаю, почему классовые модели Menu или MenuItem должны быть связаны с этим. Переход к решению без проверки типов приведет к снижению качества кода с точки зрения ориентации объекта.

Answer 5

Язык уже предоставляет этот механизм - это dynamic_cast.Тем не менее, в более общем смысле, недостаток, свойственный вашему дизайну, таков:

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

Он должен идти в функцию Execute () и рефакторинг по мере необходимости, чтобы это произошло.