How do I refactor code into a subroutine but allow for early exit?

Question

There's a really obvious refactoring opportunity in this (working) code.

bool Translations::compatibleNICodes(const Rule& rule, 
                                     const std::vector<std::string>& nicodes)
{
    bool included = false;

    // Loop through the ni codes.
    for(std::vector<std::string>::const_iterator iter = nicodes.begin();
        iter != nicodes.end();
        ++iter)
    {
        // Match against the ni codes of the rule
        if(rule.get_ni1() == *iter)
        {
            // If there's a match, check if it's flagged include or exclude
            const std::string flag = rule.get_op1();
            // If include, code is included unless a later rule excludes it
            if(flag == "INCLUDE"){ included = true; }
            // If exclude, code is specifically excluded
            else if(flag == "EXCLUDE"){ return false; }
        }
        if(rule.get_ni2() == *iter)
        {
            const std::string flag = rule.get_op2();
            if(flag == "INCLUDE"){ included = true; }
            else if(flag == "EXCLUDE"){ return false; }
        }
        if(rule.get_ni3() == *iter)
        {
            const std::string flag = rule.get_op3();
            if(flag == "INCLUDE"){ included = true; }
            else if(flag == "EXCLUDE"){ return false; }
        }
        if(rule.get_ni4() == *iter)
        {
            const std::string flag = rule.get_op4();
            if(flag == "INCLUDE"){ included = true; }
            else if(flag == "EXCLUDE"){ return false; }
        }
        if(rule.get_ni5() == *iter)
        {
            const std::string flag = rule.get_op5();
            if(flag == "INCLUDE"){ included = true; }
            else if(flag == "EXCLUDE"){ return false; }
        }
    }
    return included;
}

I want to turn it to something like:

bool Translations::compatibleNICodes(const Rule& rule, 
                                     const std::vector<std::string>& nicodes)
{
    bool included = false;

    // Loop through the ni codes.
    for(std::vector<std::string>::const_iterator iter = nicodes.begin();
        iter != nicodes.end();
        ++iter)
    {
        // Match against the ni codes of the rule
        included |= matchNICode(rule.get_ni1(), rule.get_op1);
        included |= matchNICode(rule.get_ni2(), rule.get_op2);
        included |= matchNICode(rule.get_ni3(), rule.get_op3);
        included |= matchNICode(rule.get_ni4(), rule.get_op4);
        included |= matchNICode(rule.get_ni5(), rule.get_op5);
    }
    return included;
}

bool Translations::matchNICode(const std::string& ni, 
                               const std::string& op)
{
    if(ni == *iter)
    {
        if(op == "INCLUDE"){ return true; }
        else if(op == "EXCLUDE"){ /*Return worse than false*/ }
    }
    return false;
}

The problem is that I can't get around the problem that I want to exit early if it's an exclude statement.

Note that I can't change the structure of the Rule class.

Any advice?

Answer 1

Obviously the code would be much cleaner and simpler, if you could iterate through the ni and op members of Rule in a loop. If you can't refactor Rule, maybe you could create a wrapper around it to achieve this goal.

If you have a single method with such code, I wouldn't bother though. IMO this would only pay off if you can eliminate the duplicated code in several similar methods.

Answer 2

At first, I would rethink methods get_ni1 ... as get_ni(1). Similarly for get_opN as get_op(N). get_ni and get_op then can do different things according to a switch-case. This way the list of if can be put into a for loop. When exclude appears, you can break. The problem is that breaking from nested loops is not possible in C++. But you can use another bool so that when you break from inner for, at the end you check this bool (initially false) and if it is true, you break from outer for too. And finally included is returned.

Now if you want the whole

 const std::string flag = rule.get_op5();
  if(flag == "INCLUDE"){ included = true; }
  else if(flag == "EXCLUDE"){ return false; }

as a subroutine, you can make it return boolean value and check for it. The caller "decides" if break/return or not. Of course the caller must pass i also (if looping over i for get_op changed as described before).

But unless you use that thing in other functions too, the prev "refactoring" with for and the change of get_ni and get_op make according to me not necessary to turn that code into a subroutine.

EDIT

An example (semi-pseudocoded)

totalexit = false;
for( ...iter... )
{
   for( i = 1; i <= 5; i++ ) {
      if (rule.get_ni(i) == *iter) {
         // ...
         if (flag == "INCLUDE") { included = true; }
         else (flag == "EXCLUDE") { included = false;
            totalexit = true; break; }
         // you can return directly from here indeed
         // but this show a general way of breaking from nested loops
      }
   }
   if (totalexit) break;
}
return included;

EDIT Added a comment in the code: return false can be used from the inner loop in this approach, even if you make that piece a function (see my comment); but in case you need to do something before returning, this solution could be useful.

Final Edit

The code you've added and requirement (no change to Rule) made me think about another possible solution I've described in the comment to the question. Basically it is an analysis of what is returned to the caller of compatibleNICodes. It is true iff all the calls to matchNICode are true, false otherwise. In the false case, it is matchNICode that "wants" to return false, but this is not essential, since false is false, and true is true.

So, if you put in And all the values returned by matchNICode, and makes this one return true or false (instead of "more than false"), you can write something like

if ( !(matchNICode(...1) && matchNICode(...2) && ...) ) return false;

and you can also change the final return included into return true since if the end of the loop is reached for real, included must be true (otherwise in your idea a sudden return from inside matchNICode should have happened).

Answer 3

One possible refactoring is the below, but I'm not sure if it is worth the trouble

#define NI_CLAUSE(ID) \
        if(rule.get_ni ## ID() == *iter) \
        { \
            const std::string flag = rule.get_op ## ID(); \
            if(flag == "INCLUDE"){ included = true; } \
            else if(flag == "EXCLUDE"){ return false; } \
        }

bool Translations::compatibleNICodes(const Rule& rule, 
                                     const std::vector<std::string>& nicodes)
{
    bool included = false;

    // Loop through the ni codes.
    for(std::vector<std::string>::const_iterator iter = nicodes.begin();
        iter != nicodes.end();
        ++iter)
    {
        NI_CLAUSE(1)
        NI_CLAUSE(2)
        NI_CLAUSE(3)
        NI_CLAUSE(4)
        NI_CLAUSE(5)
    }
    return included;
}

Answer 4

bool Translations::compatibleNICodes(const Rule& rule,
                                     const std::vector<std::string>& nicodes)
{
    bool included = false;

    struct
    {
      RULE_GET_NI get_ni;
      RULE_GET_OP get_op;
    } method_tbl[] =
    {
      { &Rule::get_ni1, &Rule::get_op1 },
      { &Rule::get_ni2, &Rule::get_op2 },
      { &Rule::get_ni3, &Rule::get_op3 },
      { &Rule::get_ni4, &Rule::get_op4 },
      { &Rule::get_ni5, &Rule::get_op5 },
    };
    // Loop through the ni codes.
    for(std::vector<std::string>::const_iterator iter = nicodes.begin();
        iter != nicodes.end();
        ++iter)
    {
        for(size_t n = 0; n < 5 /* I am lazy here */; ++n)
        {
            if((rule.*(method_tbl[n].get_ni))() == *iter)
            {
                // If there's a match, check if the rule is include or exclude
                const std::string flag = (rule.*(method_tbl[n].get_op))();
                // If include, code is included unless a later rule excludes it
                if(flag == "INCLUDE"){ included = true; }
                // If exclude, code is specifically excluded
                else if(flag == "EXCLUDE"){ return false; }
            }
        }
    }
    return included;
}

The answer was edited to include only final version.

BTW this problem is fun, just give me some more time and I come up with stl algorithm and functor...

Answer 5

I assume that the get_niX() or get_opX() methods have some kind of side effect; otherwise, as soon as you get a true, you would want to exit.

If the thing returned from matchNICode() is really worse than false, it may be an exception. In this case, it is quite simple:

bool Translations::compatibleNICodes(const Rule& rule, 
                                     const std::vector<std::string>& nicodes)
{
    bool included = false;

    try
    {
      // Loop through the ni codes.
      for(std::vector<std::string>::const_iterator iter = nicodes.begin();
          iter != nicodes.end();
          ++iter)
      {
        // Match against the ni codes of the rule
        included |= matchNICode(rule.get_ni1(), rule.get_op1);
        included |= matchNICode(rule.get_ni2(), rule.get_op2);
        included |= matchNICode(rule.get_ni3(), rule.get_op3);
        included |= matchNICode(rule.get_ni4(), rule.get_op4);
        included |= matchNICode(rule.get_ni5(), rule.get_op5);
      }
      return included;
    }
    catch (WorseThanFalseException& ex)
    {
      return false; // Or whatever you have to do and return
    }
}

bool Translations::matchNICode(const std::string& ni, 
                               const std::string& op)
{
    if(ni == *iter)
    {
        if(op == "INCLUDE"){ return true; }
        else if(op == "EXCLUDE"){ throw WorseThanFalseException(); } // Whatever this is
    }
    return false;
}

Answer 6

This is promised algorithms based solution and it is hardcore... And they are saying STL is here to simplify our programs (this is waaaay longer then the other solution I proposed).

struct FUNCTOR : std::unary_function<bool, std::string const &>
{
  public:
    FUNCTOR(Rule const &r) : included(false), rule(r)
    {
    }
    // returns true if exluded
    bool operator()(std::string const &s)
    {
      static METHODS methods[] =
      {
        { &Rule::get_ni1, &Rule::get_op1 },
        { &Rule::get_ni2, &Rule::get_op2 },
        { &Rule::get_ni3, &Rule::get_op3 },
        { &Rule::get_ni4, &Rule::get_op4 },
        { &Rule::get_ni5, &Rule::get_op5 },
      };
      return(std::find_if(&methods[0], &methods[5], FUNCTOR2(rule, s, included)) != &methods[5]);
    }
    operator bool()
    {
      return(included);
    }
  private:
    struct METHODS
    {
      std::string (Rule::*get_ni)() const;
      std::string (Rule::*get_op)() const;
    };
    struct FUNCTOR2 : std::unary_function<bool, METHODS>
    {
      public:
        FUNCTOR2(Rule const &r, std::string const &s, bool &incl) : rule(r), str(s), included(incl)
        {
        }
        // return true if exluded
        bool operator()(METHODS m)
        {
          if((rule.*m.get_ni)() == str)
          {
            // If there's a match, check if the rule is include or exclude
            const std::string flag = (rule.*m.get_op)();
            // If include, code is included unless a later rule excludes it
            if(flag == "INCLUDE")
              included = true;
            // If exclude, code is specifically excluded
            else if(flag == "EXCLUDE")
            {
              included = false;
              return(true);
            }
          }
          return(false);
        }
      private:
        Rule const &rule;
        std::string const &str;
        bool &included;
    };
    Rule const &rule;
    bool included;
};

bool Translations::compatibleNICodes(const Rule& rule,
                                     const std::vector<std::string>& nicodes)
{
  FUNCTOR f(rule);
  std::find_if(nicodes.begin(), nicodes.end(), f);
  return(f);
}

Answer 7

You could get around by creating some kind of tribool class and use lazy evaluation.

class TriState
{
public:
  TriState(): mState(KO) {}

  bool isValid() const { return mState != FATAL; }

  bool ok() const { return mState == OK; }

  void update(std::string const& value,
              std::string const& reference,
              std::string const& action)
  {
    if (mState == FATAL) return;

    if (value == reference)
    {
      if (action == "INCLUDE") mState = OK;
      else if (action == "EXCLUDE") mState = FATAL;
    }
  }

private:
  typedef enum { OK, KO, FATAL } State_t;
  State_t mState;
};

Then you can use the loop as such:

TriState state;

for (const_iterator it = nicodes.begin(), end = nicodes.end();
     it != end && state.isValid(); ++it)
{
   state.update(*it, rule.get_ni1(), rule.get_op1);
   state.update(*it, rule.get_ni2(), rule.get_op2);
   state.update(*it, rule.get_ni3(), rule.get_op3);
   state.update(*it, rule.get_ni4(), rule.get_op4);
   state.update(*it, rule.get_ni5(), rule.get_op5);
}

return state.ok();

Now, if the operation on rule have some kind of side effect that should be avoided, you use a wrapper to get lazy evaluation.

class Wrapper
{
public:
  Wrapper(Rule const& rule): mRule(rule) {}

  std::string const& get_ni(size_t i) const { switch(i) { ... } }
  std::string const& get_action(size_t i) const { switch(i) { ... } }

private:
  Rule const& mRule;
};

Refactor update:

void update(std::string const& value, Wrapper wrapper, size_t index)
{
  if (mState == FATAL) return;

  if (value == wrapper.get_ni(index))
  {
    if (wrapper.get_action(index) == "INCLUDE") mState = OK;
    else if (wrapper.get_action(index) == "EXCLUDE") mState = FATAL;
  }
}

Use a double loop:

TriState state;
Wrapper wrapper(rule);

for (const_iterator it = nicodes.begin(), end = nicodes.end();
     it != end && state.isValid(); ++it)
{
  for (size_t index = 1; index != 6 && state.isValid(); ++index)
    state.update(*it, wrapper, index);
}

return state.ok();

Guideline: Wrap what you can't modify! (look at the Adaptor family of Patterns)

Answer 8

Using a in/out parameter is a straightforward and efficient way to get two return values:

Plus I presume you need lazy evaluation of rule.get_opN()? To do that you'll need to use a pointer-to-member-function.

bool Translations::compatibleNICodes(const Rule& rule, 
                                     const std::vector<std::string>& nicodes)
{
    bool included = false;

    // Loop through the ni codes.
    for(std::vector<std::string>::const_iterator iter = nicodes.begin();
        iter != nicodes.end();
        ++iter)
    {
        // Match against the ni codes of the rule
        if (!matchNICode(rule.get_ni1(), rule, &Rule::get_op1, included)) return false;
        if (!matchNICode(rule.get_ni2(), rule, &Rule::get_op2, included)) return false;
        if (!matchNICode(rule.get_ni3(), rule, &Rule::get_op3, included)) return false;
        if (!matchNICode(rule.get_ni4(), rule, &Rule::get_op4, included)) return false;
        if (!matchNICode(rule.get_ni5(), rule, &Rule::get_op5, included)) return false;
    }
    return included;
}

bool Translations::matchNICode(const std::string& ni, const Rule& rule, 
                               std::string (Rule::* const opfn)(), bool& included)
{
    if (ni == *iter) {
        const std::string& op = (rule.*opfn)();
        if (op == "INCLUDE") {
            included = true;
        }
        else if (op == "EXCLUDE") {
            return false;
        }
    }
    return true;
}