CODEWITHSUNDEEP
c++dry
R0m1
R0m1

Reputation: 240

How to avoid copy and paste when two functions are very similar?

I often come across methods that have the same structure and logic, but with some differences and I don't find a proper way not to repeat myself.

For example :

void ChannelSelection::selectAlmostOkChannels(int currentInkId)
{
    bool selected = true;
    foreach (auto report, m_reports) {
        if (report.scoreByInk.find(currentInkId) != report.scoreByInk.end()) {
            auto tmpStatus = Assessment::getStatusFromScore(report.scoreByInk.value(currentInkId));
            if (tmpStatus == Assessment::Ok)
                selected = false;
            else if (tmpStatus == Assessment::NotOk)
                m_autoSelection[report.name].setSelected(currentInkId, false);
        }
    }
    m_currentSelection.insert(currentInkId, selected);
}

void ChannelSelection::selectNotOkChannels(int currentInkId)
{
    bool selected = true;
    foreach (auto report, m_reports) {
        if (report.scoreByInk.find(currentInkId) != report.scoreByInk.end()) {
            auto tmpStatus = Assessment::getStatusFromScore(report.scoreByInk.value(currentInkId));
            if (tmpStatus == Assessment::Ok || tmpStatus == Assessment::AlmostOk)
                selected = false;
        }
    }
    m_currentSelection.insert(currentInkId, selected);
}

As you can see, these 2 functions are very similar (only the inner if differs). How could I nicely remove duplication in this code?

One of the solution I thought of is using a functor, something like :

void ChannelSelection::selectChannels(int currentInkId, std::function<bool()> fn)
{
    bool selected = true;
    foreach (auto report, m_reports) {
        if (report.scoreByInk.find(currentInkId) != report.scoreByInk.end()) {
            auto tmpStatus = Assessment::getStatusFromScore(report.scoreByInk.value(currentInkId));
            selected = fn();
        }
    }
    m_currentSelection.insert(currentInkId, selected);
}

The caller gets the responsibility to implement the functor. Is there an alternative without that problem?

Upvotes: 6

Views: 252

Answers (2)

vahancho
vahancho

Reputation: 21240

You might merge two functions into a single one with additional conditional parameter like:

void ChannelSelection::selectChannels(int currentInkId, bool condition)
{
  bool selected = true;
  foreach (auto report, m_reports) {
    if (report.scoreByInk.find(currentInkId) != report.scoreByInk.end()) {
      auto tmpStatus = Assessment::getStatusFromScore(report.scoreByInk.value(currentInkId));
      if (condition) {
        if (tmpStatus == Assessment::Ok) {
          selected = false;
        } else if (tmpStatus == Assessment::NotOk) {
          m_autoSelection[report.name].setSelected(currentInkId, false);
        }
      } else if (tmpStatus == Assessment::Ok || tmpStatus == Assessment::AlmostOk) {
        selected = false;
      }
    }
  }
  m_currentSelection.insert(currentInkId, selected);
}

Calling it with condition == true will invoke the equivalent of selectAlmostOkChannels() function, and selectNotOkChannels() otherwise.

Upvotes: 1

KABoissonneault
KABoissonneault

Reputation: 2369

You don't have to make your parameterized selectChannels public. It can be a private implementation detail of both selectAlmostOkChannels and selectNotOkChannels, your public functions.

selectChannels can even be implemented as a function template, so that the generated code is equivalent to the hand-written "copy pasted" version, without the maintenance burden of code duplication

template<typename SelectFunction>
void ChannelSelection::selectChannels(int currentInkId, SelectFunction selectFn)
{
    bool selected = true;
    foreach (auto report, m_reports) {
        if (report.scoreByInk.find(currentInkId) != report.scoreByInk.end()) {
            auto tmpStatus = Assessment::getStatusFromScore(report.scoreByInk.value(currentInkId));
            selected = selectFn(tmpStatus);
            /* fill in */
        }
    }
    m_currentSelection.insert(currentInkId, selected);
}

void ChannelSelection::selectAlmostOkChannels(int currentInkId)
{
    selectChannels(currentInkId, [] (auto tmpStatus) -> bool {
        return /* fill in */;
    });
}

void ChannelSelection::selectNotOkChannels(int currentInkId)
{
    selectChannels(currentInkId, [] (auto tmpStatus) -> bool {
        return /* fill in */;
    });
}

You might have been taught that templates need to be in headers, but that's actually not the full story! Template definitions need to be visible where instantiated. Since your template is only used in the private implementation of your member functions, then your template definition can be in the same file that's implementing both your member functions

Upvotes: 5

Related Questions