4

Here is my problem:

I have and class structure like this:

class Base
{
    private:
        SomeType    something;
        bool        isSomeValue;

    public:
        virtual void myMethod() = 0;
};

class Child1 : public Base
{
    virtual void myMethod() override
    {
        // do step 1
        for (int i = 0; i < something.size(); ++i)
        {
             // do step 2

             if (isSomeValue)
             {
                  // do step 3
             }
        }
        // do step 4
    }
};


class Child2 : public Base
{
    virtual void myMethod() override
    {
        // do step 1
        for (int i = something.size() - 1; i >= 0; --i)
        {
             // do step 2

             if (isSomeValue)
             {
                  // do step 3
             }
        }
        // do step 4
    }
};


class Child3 : public Base
{
    virtual void myMethod() override
    {
        // do step 1
        for (int i = 0; i < something.size(); ++i)
        {
             // do step 2

             if (isSomeValue)
             {
                  // do step 3
             }
             else
             {
                 break;
             }
        }
        // do step 4
    }
};

As you can see in Child2 and Child3 differ from Child1 as in Child2 loop is going from reverse direction and in Child3 there is also else with break statements. I hope you agree that those algorithms differ only slightly and I almost code copy. Is there any better approach to such similar structures not to write similar code in lots of different places?

Here is DRY principle that I want to use though I don't do code copy, only write similar code. I just see similar construct in several classes. Could Template Method Pattern that is used smartly eliminate the similar code? Or any other approach?

  • 1
    If what you're looking to abstract is the loop control structure, I would think iterators would be appropriate. – Erik Eidt Mar 22 '16 at 15:14
  • 4
    There's value in supplying the real name of the myMethod and the classes. Semantics are part of the design and provide insight when looking at what's repeating. – Fuhrmanator Mar 22 '16 at 15:16
5

I would start to refactor the common parts, not the different ones:

class Base
{
    protected:
        SomeType    something;
        bool        isSomeValue;

        void step1();
        void step2(int i);
        void step3IfSomeValue(int i);
        void step4();

    public:
        virtual void myMethod() = 0;
};

class Child1 : public Base
{
    virtual void myMethod() override
    {
        step1();
        for (int i = 0; i < something.size(); ++i)
        {
             step2(i);
             step3IfIsSomeValue(i);
        }
        step4();
    }
};


class Child2 : public Base
{
    virtual void myMethod() override
    {
        step1();
        for (int i = something.size() - 1; i >= 0; --i)
        {
             step2(i);
             step3IfIsSomeValue(i);
        }
        step4();
    }
};


class Child3 : public Base
{
    virtual void myMethod() override
    {
        step1();
        for (int i = 0; i < something.size(); ++i)
        {
             step2(i);
             step3IfIsSomeValue(i);
             if (!isSomeValue)
                 break;
        }
        step4();
    }
};

Of course, as a second step, with some effort, you can actually generalize this three methods into one method, as pointed out by @JoulinRouge (note that this shows not the template method pattern).

However, you have to consider if that does really improve your code. Ask yourself: is this something with real risk of getting forgotten to be changed if you have to add some additional features, because you have "similar parts" in three methods but one? Or is the opposite true: does it get harder to change something because you (over)generalized three methods into one, and now you cannot change them individually any more? If you can answer that question, you know what to do.

  • I would just add one comment here: make your function do only one thing and the problem I have will almost never happen. Divide and conquer! Usually that kind of function are not more than 20 lines. And this answer uses that approach to fix the problem of DRY violation. – Narek Oct 26 '16 at 9:22
3

How about something like this?

void myMethod(Boolean reverse, Boolean doElse)
{
// do step 1

int start = 0;
int stop = 0;
if(reverse)
{
    start = something.size();
    stop = 0;
}
foreach(i in range(start, stop))
{
     // do step 2

     if (isSomeValue)
     {
          // do step 3
     }
     else
     {
        if(doElse)
            break;
     }
}
    // do step 4
}
}

it's less readable but you won't copy-paste code

  • It's a nice effort, but I was thinking something more along the lines of a custom map function. – Robert Harvey Mar 22 '16 at 15:07
  • 3
    I don't like it when people downvote such an answer without any comment - this answer is IMHO not so bad, as it demonstrates generalizing here is neither hard nor makes it the code unreadable (see my own answer). If it is the right solution or not depends on things we do not see in the OPs code because the example is artificial. – Doc Brown Mar 22 '16 at 16:47
1

Sometimes repeating yourself is a good idea. Or sometimes it's not actually repeating yourself.

Here it seems that your control flow differs, but the different steps in the comments are the same. If so, then your best bet is to write reusable functions that handle the steps themselves and can be called anywhere that's required, and then build different control flow structures around them.

You could build some elaborate objects or templates to do the control flow for you, but one does have to wonder what the point would be. You don't need the control flow to change during execution and you don't need to make this pluggable, so code the control flow in a nice, explicit manner and have Step1() Step2() etc. for where the code is genuinely the same.

  • The for loops differ as well. While the classes look superficially similar, they're actually not. You could bend this code into a pretzel trying to make it more DRY, but I'm not sure it's worth it unless there are a large number of child classes. – Robert Harvey Mar 22 '16 at 14:20
  • 1
    Yeah, the moment you start to abstract this too much you have to write abstractions which are far, far more complex than the original code. Not worth it. – Matthew Walton Mar 22 '16 at 15:04
0

An article you would find very interesting is Herb Sutter's Virtuality (NVI idiom)

Considering the following guidelines:


Guideline #1: Prefer to make interfaces nonvirtual, using Template Method design pattern.

You can use Template Method to make the interface stable and nonvirtual, while delegating customizable work to nonpublic virtual functions that are responsible for implementing the customizable behavior. After all, virtual functions are designed to let derived classes customize behavior; it's better to not let publicly derived classes also customize the inherited interface, which is supposed to be consistent.

Guideline #2: Prefer to make virtual functions private.

Virtual functions exist to allow customization; unless they also need to be invoked directly from within derived classes' code, there's no need to ever make them anything but private.


You could write:

class Base
{
public:
  void myMethod()
  {
    // step1

    loop();

    // step4
  }

private:
  virtual void loop() = 0;

  void step2(int);
  void step3(int);

  SomeType    something;
  bool        isSomeValue;
};

class Child1 : public Base
{
  virtual void loop() override
  {
    for (int i = 0; i < something.size(); ++i)
    {
      step2(i);

      if (isSomeValue)
        step3(i);
    }
  }
};


class Child2 : public Base
{
  virtual void loop() override
  {
    for (int i = something.size() - 1; i >= 0; --i)
    {
      step2(i);

      if (isSomeValue)
        step3(i);
    }
  }
};

class Child3 : public Base
{
  virtual void loop() override
  {
    for (int i = 0; i < something.size(); ++i)
    {
      step2(i);

      if (isSomeValue)
        step3(i);
      else
        break;
    }
  }
};

If one should proceed further depends on the considerations at the end of Doc Brown's answer.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.