Refactoring advice: How to avoid type checking in this OO design

Question

I'm looking for advice on refactoring to improve my class design and avoid type checking.

I am using the Command design pattern to construct a menu tree. An item in the menu could be of various types (e.g., an immediate action [like "Save"], a toggle on/off property which displays with check/icon depending on its state [like "italics"], etc.). Crucially, there are also submenus, which replace (rather than displaying off to the side of) the current menu on the screen. These submenus of course contain their own list of menu items, which could have more nested submenus.

The code is something like (all public for simplicity in presentation):

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

The main menu is just an instance of Menu, and a separate class keeps track of the menu position, including how to go into and out of submenus:

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

The key function is Position::OnEnterPressed(), where you see an explicit type check in the call to MenuItem::IsMenu() and then a cast to the derived type. What are some options to refactor this to avoid the type check and cast?

Answer 1

IMO, the refactoring starting point would be these statements:

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

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

The fact that the same kind of statement repeat itself is, IMO, the sign for the need to refactor that.

If you could factor (1) in a method of your base class, and then override it in the derived class to take into account the specific behavior (2), then you could just put this in Execute.

Correct me if I am wrong: the idea is the a menu has items, and each item has got an action associated to it that is triggered when some event is detected.

Now, when the item you select is a submenu, the the Execute action has the meaning: activate the submenu (I am using activate in a generic sense). When the item is not a submenu, then Execute is a different beast.

I don't have a full comprehension of your menu system, but it seems to me you have a sort of hierarchy menu/submenu (the positions), and some actions that are triggered depending of the kind of node.

What I envision is that the menu/submenu relationship is a hierarchy that allows you to define leaf-nodes (when you don't have a submenu), and non-leaf-nodes (the submenu). A leaf node invoke an action, a non-leaf-node invoke a different kind of action which deals with activating a submenu (this action goes back to the menu system, so you do not encapsulate knowledge about the menu system in it, you simply relay the action to menu system).

Don't know if this makes sense to you.

Answer 2

An alternative would be to expose a method in Position which enables a Menu to be pushed onto the stack, and call that method at the start of Menu:Execute. Then the body of OnEnterPressed just becomes

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

Answer 3

This probably isn't the response you were looking for, but in my opinion your solution is by far superior to any solution that doesn't involve type checking.

Most C++ programmers are offended by the idea that you need to check an object's type in order to decide what to do with it. Yet in other languages like Objective-C and most weakly typed script languages this is actually highly encouraged.

In your case I think using the type check is well chosen since you need the type information for the functionality of Position. Moving this functionality into one of the MenuItem subclasses would IMHO violate competence separation. Position is concerned with the viewing and controller part of your menu. I don't see why the model classes Menu or MenuItem should be concerned with that. Moving to a no-typecheck solution would decrease code quality in terms of object orientation.

Answer 4

What you need is the ability to express "either an action or a menu", which is very cumbersome to write using polymorphism if actions and menus have very different interfaces.

Instead of trying to force them into a common interface (Execute is a poor name for the submenu method), I'd go further than you and use dynamic_cast.

Also, dynamic_cast is always superior to a flag and static_cast. Actions do not neet to tell the world that they aren't submenus.

Rewritten in the most idiomatic C++, this gives the following code. I use std::list because of its convenience methods splice, insert and remove which don't invalidate iterators (one of the few good reasons for using linked lists). I also use std::stack for keeping track of the open menus.

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

I tried to keep the code straightforward. Feel free to ask if something is unclear.

[About iterator invalidation when doing splice, see this question (tl;dr: it is OK to splice and keep the old iterators provided you don't use a too old compiler).]

Answer 5

The language already provides this mechanism- it's dynamic_cast. However, in a more general sense, the inherent flaw in your design is this:

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

It should go in the Execute() function, and refactor as necessary to make that happen.