Is it possible combining symmetric code pieces?

Question

I was implementing avl trees for my school projects and found myself writing almost same code twice for symmetric situations. For example, this function performs rotation of two nodes to balance the tree. If clause handles the case in which lower node is a left child of higher one, and the else clause handles the opposite:

void avl<T>::rotate(node<T> *x, node<T> *y)
{
  if (x == y->l)
  {
    x->p = y->p;
    if (y->p != nullptr)
      if(y->p->l == y)
        y->p->l = x;
      else
        y->p->r = x;
    else
      this->setHead(x);
    y->p = x;
    y->l = x->r;
    if(x->r != nullptr)
      x->r->p = y;
    x->r = y;
    y->dl = y->calcd('l');
    x->dr = x->calcd('r');
    if(x->p != nullptr)
      if(x->p->l == x)
        x->p->dl = x->p->calcd('l');
      else
        x->p->dr = x->p->calcd('r');
  }
  else
  {
    x->p = y->p;
    if (y->p != nullptr)
      if(y->p->r == y)
        y->p->r = x;
      else
        y->p->l = x;
    else
      this->setHead(x);
    y->p = x;
    y->r = x->l;
    if(x->l != nullptr)
      x->l->p = y;
    x->l = y;
    y->dl = y->calcd('l');
    x->dr = x->calcd('r');
    if(x->p != nullptr)
      if(x->p->r == x)
        x->p->dr = x->p->calcd('r');
      else
        x->p->dl = x->p->calcd('l');

  }
}

As you can see, the else clause is exactly similar to the if clause with 'l' and 'r' swapped. Is there a way to combine them. What can I do to improve on it? Is there some design mistake in my code?

Answer 1

Use pointers to members. The only difference between the two branches is which member you're accessing, so that's the easy way to abstract that out:

using Child = node<T> node<T>::*;

void rotate_impl(node<T>* x, node<T>* y, Child* left, Child* right)
{
    x->p = y->p;
    if (y->p != nullptr) {
      if(y->p->*left == y) {
        y->p->*left = x;
      }
      else {
        y->p->*right = x;
      }
    }
    else {
      this->setHead(x);
    }

    y->p = x;
    y->*left = x->*right;

    if(x->*right != nullptr) {
      (x->*right)->p = y;
    }

    x->*right = y;
    y->dl = y->calcd('l');
    x->dr = x->calcd('r');
    if(x->p != nullptr) {
      if(x->p->*left == x) {
        x->p->dl = x->p->calcd('l');
      }
      else {
        x->p->dr = x->p->calcd('r');
      }
   }
}

void avl<T>::rotate(node<T> *x, node<T> *y)
{
  if (x == y->l) {
    rotate_impl(x, y, &node<T>::l, &node<T>::r);
  }
  else {
    rotate_impl(x, y, &node<T>::r, &node<T>::l);
  }
}

I also took the liberty of adding braces to your code. You can thank me later.

Answer 2

All problems in computer science can be solved by another level of indirection, except of course for the problem of too many indirections.
(David Wheeler)

You can do something like this (thoroughly untested):

node<T>** y_sibling_1 = x == y->l ? &y->p->l : &y->p->r;
node<T>** y_sibling_2 = x == y->l ? &y->p->r : &y->p->l;
node<T>** x_child = x == y->l ? &x->r : &x->l;
node<T>** y_child = x == y->l ? &y->l : &y->r;

x->p = y->p;
if (y->p != nullptr)
    if(*y_sibling_1 == y)
        *y_sibling_1 = x;
    else
        *y_sibling_2 = x;
else
    this->setHead(x);
y->p = x;
*y_child = *x_child;
if(*x_child != nullptr)
    (*x_child)->p = y;
*x_child = y;
y->dl = y->calcd('l');
x->dr = x->calcd('r');
if(x->p != nullptr)
    if(x->p->l == x)
        x->p->dl = x->p->calcd('l');
    else
        x->p->dr = x->p->calcd('r');

(Note that your final conditionals are equivalent in both cases.)

Whether this has too many indirections is a question of personal opinion.

Answer 3

You probably want templates here, so you'd have calcd<true>() and calcd<false>().

With that change in place, you can write

template<bool xChildy> 
void avl<T>::rotateImpl(node<T> *x, node<T> *y); 

void avl<T>::rotate(node<T> *x, node<T> *y)
{
  if (x == y->l)  {
    rotateImpl<true>(x,y);
  }
  else  {
    rotateImpl<false>(x,y);
  }
}

Answer 4

I like Mike's template approach. But for the records, I propose here another alternative.

First, consider that each node has two children. Calling them "left" and "right" is only a viewpoint of mind. You could as well put them in an array:

template <class T>
class node {
public: 
    ...
    enum side{ left,right,last};   // just to make clear my intent here
    node *p, *c[last], *d[last];   // no more l and r !!         
    node* calcd(enum side x) {}    // lets use our index here instead of char  
    ...
};

Then you could factor out the side dependent code. I like lambdas, so here's a first proposal:

template <class T>
void avl<T>::rotate(node<T> *x, node<T> *y)
{
    // this lambda works directly with locals of rotate() thanks to [&]
    // so put in the side dependent code, and put the side as parameter
    // for easy switch.  For simplicity I used l and r to facilitate
    // transposition, but reafctoring it in a neutral side1 and sinde2 
    // could help readbility
    auto rt = [&](enum node<T>::side l, enum node<T>::side r)->void {
        x->p = y->p;
        if (y->p != nullptr)
            if(y->p->c[l] == y)   // l is a parameter here instead of member name
                y->p->c[l] = x;
            else
                y->p->c[r] = x;
        else
            this->setHead(x);
        y->p = x;
        y->c[l] = x->c[r];
        if (x == y->c[l])
        {
            if(x->c[r] != nullptr)
                x->c[r]->p = y;
            x->c[r] = y;
            y->d[l] = y->calcd(sd[l]);
            x->d[r] = x->calcd(sd[r]);
            if(x->p != nullptr)
                if(x->p->c[l] == x)
                    x->p->d[l] = x->p->calcd(sd[l]);
                else
                    x->p->d[r] = x->p->calcd(sd[r]);
        }
    }; // end of the definition of the lambda

  // the code of the function is then reduced to this:  
  if (x == y->c[node<T>::left])
     rt(node<T>::left,node<T>::right);
  else
     rt(node<T>::right, node<T>::left); 
}