Orthogonal variables code duplication problem

Question

I've started refactoring some legacy code recently and came across two functions for drawing a coordinate grid, the problem is that these functions differ only in orthogonal variables they treat, something like that

void DrawScaleX(HDC dc, int step, int x0, int x1, int y0, int y1)
{
    for(int x = x0; x < x1; x += step)
    {
         MoveToEx(dc, x, y0, NULL);
         LineTo(dc, x, y1);
    }
}
void DrawScaleY(HDC dc, int step, int x0, int x1, int y0, int y1)
{
    for(int y = y0; y < y1; y += step)
    {
         MoveToEx(dc, x0, y, NULL);
         LineTo(dc, x1, y);
    }
}

So if I decide to add some fancy stuff, like antialiasing or merely change drawing pencil or whatever I'll have to put the same code in both of them and it's code duplication and it's bad we all know why.

My question is how would you rewrite these two functions into a single one to avoid this problem?

Answer 1

Here is my own solution

class CoordGenerator
{
public:
    CoordGenerator(int _from, int _to, int _step)
     :from(_from), to(_to), step(_step), pos(_from){}
    virtual POINT GetPoint00() const = 0;
    virtual POINT GetPoint01() const = 0;
    bool Next()
     {
      if(pos > step) return false;
      pos += step;
     }
protected:
    int from;
    int to;
    int step;
    int pos;
};

class GenX: public CoordGenerator
{
public:
    GenX(int x0, int x1, int step, int _y0, int _y1)
     :CoordGenerator(x0, x1, step),y0(_y0), y1(_y1){}
    virtual POINT GetPoint00() const
     {
      const POINT p = {pos, y0};
      return p;
     }
    virtual POINT GetPoint01() const
     {
      const POINT p = {pos, y1};
      return p;
     }
private:
    int y0;
    int y1;
};

class GenY: public CoordGenerator
{
public:
    GenY(int y0, int y1, int step, int _x0, int _x1)
     :CoordGenerator(y0, y1, step),x0(_x0), x1(_x1){}
    virtual POINT GetPoint00() const
     {
      const POINT p = {x0, pos};
      return p;
     }
    virtual POINT GetPoint01() const
     {
      const POINT p = {x1, pos};
      return p;
     }
private:
    int x1;
    int x0;
};

void DrawScale(HDC dc, CoordGenerator* g)
{
    do
    {
     POINT p = g->GetPoint00();
     MoveToEx(dc, p.x, p.y, 0);
     p = g->GetPoint01();
     LineTo(dc, p.x, p.y);
    }while(g->Next());
}

But I it seems to me too complicated for such a tiny problem, so I'm looking forward to still see your solutions.

Answer 2

Why you just do not extract the body of the for cycle into a separate function? Then you can do the funny stuff in the extracted function.

void DrawScaleX(HDC dc, int step, int x0, int x1, int y0, int y1)
{
    for(int x = x0; x < x1; x += step)
    {
        DrawScale(dc, x, y0, x, y1);
    }
}

void DrawScaleY(HDC dc, int step, int x0, int x1, int y0, int y1)
{
    for(int y = y0; y < y1; y += step)
    {
        DrawScale(dc, x0, y, x1, y);
    }
}

private void DrawScale(HDC dc, int x0, int y0, int x1, int y1)
{
    //Add funny stuff here

    MoveToEx(dc, x0, y0, NULL);
    LineTo(dc, x1, y1);

    //Add funny stuff here
}

Answer 3

Well, an obvious "solution" would be to make a single function and add one extra parameter (of enum-like type). And then do an if() or switch() inside, and perform the appropriate actions. Because hey, the functionality of the functions is different, so you have to do those different actions somewhere.

However, this adds runtime complexity (check things at runtime) in a place that could be just better checked at compile time.

I don't understand what's the problem in adding extra parameters in the future in both (or more functions). It goes like this:

1. add more parameters to all functions
2. compile your code, it won't compile in a bunch of places because it does not pass new parameters.
3. fix all places that call those functions by passing new parameters.
4. profit! :)

If it's C++, of course you could make the function be a template, and instead adding an extra parameter, you add a template parameter, and then specialize template implementations to do different things. But this is just obfuscating the point, in my opinion. Code becomes harder to understand, and the process of extending it with more parameters is still exactly the same:

1. add extra parameters
2. compile code, it won't compile in a bunch of places
3. fix all places that call that function

So you've won nothing, but made code harder to understand. Not a worthy goal, IMO.

Answer 4

I think I'd move:

     MoveToEx(dc, x0, y, NULL);
     LineTo(dc, x1, y);

into their own function DrawLine(x0,y0,x0,y0), which you can call from each of the existing functions.

Then there's one place to add extra drawing effects?

Answer 5

A little templates... :)

void DrawLine(HDC dc, int x0, int y0, int x0, int x1)
{
    // anti-aliasing stuff
    MoveToEx(dc, x0, y0, NULL);
    LineTo(dc, x1, y1);
}

struct DrawBinderX
{
    DrawBinderX(int y0, int y1) : y0_(y0), y1_(y1) {}

    void operator()(HDC dc, int i)
    {
     DrawLine(dc, i, y0_, i, y1_);
    }

private:
    int y0_;
    int y1_;

};

struct DrawBinderY
{
    DrawBinderX(int x0, int x1) : x0_(x0), x1_(x1) {}

    void operator()(HDC dc, int i)
    {
     DrawLine(dc, x0_, i, x1_, i);
    }

private:
    int x0_;
    int x1_;

};

template< class Drawer >
void DrawScale(Drawer drawer, HDC dc, int from, int to, int step)
{
    for (int i = from; i < to; i += step)
    {
         drawer(dc, i);
    }
}

void DrawScaleX(HDC dc, int step, int x0, int x1, int y0, int y1)
{
    DrawBindexX drawer(y0, y1);
    DrawScale(drawer, dc, x0, x1, step);
}

void DrawScaleY(HDC dc, int step, int x0, int x1, int y0, int y1)
{
    DrawBindexY drawer( x0, x1 );
    DrawScale(drawer, dc, y0, y1, step);
}

Answer 6

Drawing a line is simply joining two points, and drawing a scaling incrementing (x0,y0) and(x1,y1) in a particular direction, through X, and/or through Y. This boils down to, in the scale case, which direction(s) stepping occurs (maybe both directions for fun).

template< int XIncrement, YIncrement >
struct DrawScale
{
  void operator()(HDC dc, int step, int x0, int x1, int y0, int y1)
  {
    const int deltaX = XIncrement*step;
    const int deltaY = YIncrement*step;
    const int ymax = y1;
    const int xmax = x1;
    while( x0 < xmax && y0 < ymax )
    {
        MoveToEx(dc, x0, y0, NULL);
        LineTo(dc, x1, y1);
        x0 += deltaX;
        x1 += deltaX;
        y0 += deltaY;
        y1 += deltaY;
    }
  }
};
typedef DrawScale< 1, 0 > DrawScaleX;
typedef DrawScale< 0, 1 > DrawScaleY;

The template will do its job: at compile time the compiler will remove all the null statements i.e. deltaX or deltaY is 0 regarding which function is called and half of the code goes away in each functor.

You can add you anti-alias, pencil stuff inside this uniq function and get the code properly generated generated by the compiler.

This is cut and paste on steroids ;-)

-- ppi