Group two functions that differ in only 1 line of code

Question

I have two performance-critical functions like this:

insertExpensive(Holder* holder, Element* element, int index){
    //............ do some complex thing 1 
    holder->ensureRange(index);//a little expensive
    //............ do some complex thing 2
}
insertCheap(Holder* holder, Element* element, int index){
    //............ do some complex thing 1
    //............ do some complex thing 2
}

How to group 2 functions together to increase maintainability?

My poor solutions:

Solution 1.

insertExpensive(Holder* holder, Element* element, int index){
    do1();
    holder->ensureRange(index);//a little expensive
    do2();
}
insertCheap(Holder* holder, Element* element, int index){
    do1();
    do2();
}

It would be ugly. It also impractical if do2 want some local variables from do1.

Solution 2.

insert(Holder* holder, Element* element, int index, bool check){
    //............ do some complex thing 1 
    if(check)holder->ensureRange(index);//a little expensive
    //............ do some complex thing 2
}

It costs a conditional checking for every call.

Solution 3. (draft)

template<bool check> insert(Holder* holder, Element* element, int index){
    //............ do some complex thing 1       (Edit2 from do1());
    bar<check>();
    //............ do some complex thing 2       (Edit2 from do2());
}
template <>
inline void base_template<true>::bar() {  holder->ensureRange(index); }
template <>
inline void base_template<false>::bar() {  }

Overkill and unnecessary complexity?

Edit 1: The priority of criteria for how good an approach is, are sorted as followed:-
1. Best performance
2. Less duplicate of code
3. Less total line of code
4. Easier to read for expert & beginner

Edit 2: edit the 3rd solution. Thank mvidelgauz and Wolf.

Answer 1

Your solution 2 is actually not yet that bad. If this code is within a header, it is implicitly considered as inline code. (I've written it explicitly) If you call it with either true or false, the compiler can remove the if-statement, though it depends on a range of factors to know if it will do so. (Size of the whole body after inlining, visibility of the constant, tuning ...)

inline void insert(Holder* holder,Element* element,int index, bool check){
    do1();
    if (check)
        holder->ensureRange(index);//a little expensive
    do2();
}

The solution 3 is actually what you want to achieve, as templates require a new function instantiation for each different call, so it would remove the dead code. However it can be written very similar to how you wrote solution 2.

template <bool check>
inline void insert(Holder* holder,Element* element,int index){
    do1();
    if (check)
        holder->ensureRange(index);//a little expensive
    do2();
}

If you have C++17, you no longer have to depend on the compiler to remove dead code as you can enforce it to skip certain code via constexpr-if. This construction will guarantee that the code in the if-statement is removed as it ain't even have to compile.

template <bool check>
inline void insert(Holder* holder,Element* element,int index){
    do1();
    if constexpr (check)
        holder->ensureRange(index);//a little expensive
    do2();
}

Answer 2

insert(Holder* holder,Element* element,int index, bool ensureRange){
    //............ do some complex thing 1 
    if (ensureRange){
        holder->ensureRange(index);//a little expensive
    }
    //............ do some complex thing 2
}

And if you can make decision at compile time and want to employ templates:

template<bool check> insert(Holder* holder,Element* element,int index){
    //............ do some complex thing 1;
    if(check)holder->ensureRange(index);
    //............ do some complex thing 2
}


insert<true>(...); //expensive operation
insert<false>(...); //cheap operation

Answer 3

Let's assume you make the following structure:

A class which define two protected methods do1 and do2 and a public abstract method Insert.

class BaseHolder
{
proteted:

void do1(/* complete with necessary parameters*/)
{

}

void do2(/* complete with necessary parameters*/)
{

}

public:
abstract void Insert(Holder* holder, Element* element, int index);

};

class Expensive : BaseHolder
{
public:
void Insert(Holder* holder, Element* element, int index)
{
    do1();
    holder->ensureRange(index);
    do2();
}
};

class Cheap : BaseHolder
{
public:
void Insert(Holder* holder, Element* element, int index)
{
    do1();
    do2();
}
};

Sorry if I have made some syntax mistakes, but this is the way I see a solution.

---

Another possiblity is to make a custom Cheap and Expensive classes which both wrapp a Holder, and in an Expensive constructor do the validate for range:

class Base
{
protected:
Holder* _holder;
public:
Holder* GetHolder(){ return _holder; }
}

class Cheap : Base
{
    public:
    Cheap(Holder* holder)
    {
        _holder = holder;
    }
};

class Expensive : Base
{
    public:
    Expensive(Holder* holder)
    {
         holder->ensureRange(index);
         _holder = holder;
    }
};

An use Cheap and Expensive objects as parameters to Insert method. I guess the second solution is better than the first one.

---

And a solution more like the first one, but which uses the template method design pattern, as the best comes at the end:

class BaseHolder
{
proteted:

void do1(/* complete with necessary parameters*/)
{

}

void do2(/* complete with necessary parameters*/)
{

}
virtual void check(Holder* holder, int index){  };
public:
void Insert(Holder* holder, Element* element, int index)
{
    do1();
    check(holder, index);
    do2();    
};

class Expensive : BaseHolder
{
protected:
override void check(Holder* holder, int index)
{ 
    holder->ensureRange(index);
}
};

class Cheap : BaseHolder
{
};

This solution defines the check method only on an Expensive object, it has the most code reuse and it is definetly one of the cleanest approaches. Unfortunately this is about oop design more than about the performance where it is not the best, but you will have to think which is your priority, as you just concluded.

Answer 4

Technically, I'd prefer a solution like shown in mvidelgauz's answer, but the added arguments look bad when the function has to be actually called because of the not self-explaining literals true or false. So I'd combine three functions: an internal (private) function that provides the check flag as an argument, and two external functions that provide obvious names about the function, I'd prefer something like checkedInsert, uncheckedInsert. These public functions then will call the 4-argument implementation.

In the following code snippet reduced most of the parameters to make the solution most obvious. It should be seen as a fragment of a class definition:

public:
/// performs a fast insert without range checking. Range-check yourself before!
void checkedInsert(Element* element)
{
    insertWithCheckOption(element, true);
}

/// performs a range-checked insert
void uncheckedInsert(Element* element)
{
    insertWithCheckOption(element, false);
}

private:
/// implements insert with range check option
void insertWithCheckOption(Element* element, bool doRangeCheck)
{
    // ... do before code portion ...
    if (doRangeCheck) {
         // do expansive range checking  ...
    }
    // ... do after code portion ...
}

This solution provides both: the best user interface and a coherent implementation.
BTW: I really doubt that real performance issues will ever be caused by a single conditional check.

Answer 5

As another alternative, you can simply use a default function argument, with a default value corresponding to no range check

void insert(Holder* holder, Element* element, int index, 
            bool performRangeCheck = false)
{
  /* do some complex thing 1 */

  if(performRangeCheck)
  {
    holder->ensureRange(index);
  }

  /* do some complex thing 2 */
}

// ...

insert(holder, element, 2);       // no range check
insert(holder, element, 2, true); // perform range check