Code Review

[kevinchkin]

Hi Guys,

I was reading on "Strategy design pattern." I attempted to write a program that uses it. Can someone please point if it looks all right? or what changes should I make to improve it?

Overview
Basically, I am using it to draw the line using "Bresenham's line drawing algorithm." Since, there will be different logic that I will have to use to draw the line at different slopes, so for that I am using "midPointAlgorithm" class as my strategy pattern. Then I have another class "Shapes". I use the "Shapes" class pointer to point to the "line" derived class to ultimately draw the line.

Note: To make it easier to read, I removed the implementation of the logic of each function.

File: strategy.h (calculatePoints is the function that actually implements the logic of drawing lines at different slopes and calls the draw function to draw the line)

typedef tuple<int,int,int,int,float,float,float,float,float,float> midPointArg;

class midPointAlgorithm { 

    protected:
        midPointArg mArgs ; 

        enum { X0, Y0, X1, Y1, R0, G0, B0, R1, G1, B1 } ; 

        int     x0,y0,x1,y1     ;   
        int     changeColor  ;  // for color interpolation. Would be wither x1-x0 or y1-y0    

        float   r0,g0,b0,r1,g1,b1; // color co-ordinates that needs to be interpolated along the line 
        float   r, g, b        ;  // actual color that has been used to draw the pixel 
        float   dr, dg, db     ;  // change in color 
        float   point          ;  // pixel point size     
    
        void draw(int x, int y,float r, float g, float b,int point) ; // draw a pixel  

        virtual void calculatePoints(void) = 0 ;  // calculate x,y co-ordinates to draw the pixel, (draw function is called from this)  

    private:

        void initArguments() ;  // initialize x0,y0,x1 ... values 

    public:
        explicit midPointAlgorithm(const midPointArg &args, float p = 2.0) : mArgs(args),point(p) { 
            initArguments() ; 
        }
} ;

class slopePositiveOne : public midPointAlgorithm {

    protected:
        virtual void calculatePoints()  ; // calculate x,y co-ordinates when slope m is 0< m <1 

    public:

        explicit slopePositiveOne(const midPointArg &args, float p = 2.0) : midPointAlgorithm(args,p) {
            calculatePoints() ;
        }
} ;

class slopePositiveInfinity : public midPointAlgorithm {

    protected:
        virtual void calculatePoints()  ;

    public:
        explicit slopePositiveInfinity(const midPointArg &args, float p = 2.0) : midPointAlgorithm(args,p) {
            calculatePoints() ;
        }
} ;

class slopeNegativeOne : public midPointAlgorithm {

    protected:
        virtual void calculatePoints()  ;

    public:
        explicit slopeNegativeOne(const midPointArg &args, float p = 2.0) : midPointAlgorithm(args,p) {
            calculatePoints() ;
        }
} ;

class slopeNegativeInfinity : public midPointAlgorithm {

    protected:
         virtual void calculatePoints()  ;

    public:
        explicit slopeNegativeInfinity(midPointArg &args,float p = 2.0) : midPointAlgorithm(args,p) {
            calculatePoints() ;
        }
} ;

Shapes Class

class Shapes {
    
    public:
        virtual void draw() = 0 ; 
        
} ;

class Line : public Shapes {

    private:
        enum { X0, Y0, X1, Y1 } ;

        midPointAlgorithm  *mpA   ;
        midPointArg         mArg  ;
        float              point  ;

        void condition()          ;  // to determine the slope of the line and find out where the point lies 

    public:
        explicit Line(const midPointArg &args, float p = 2.0) : mArg(args),point(p),mpA(NULL) {
        }

        ~Line() {
            if (mpA != NULL) {
                delete mpA ;
            }
        }

        void draw() ; 
} ;

File: strategy.cc (I am just copying the relevant code which calls the
different algorithm)

void Line :: condition() { 

    int &x0 = get<X0>(mArg) ; 
    int &y0 = get<Y0>(mArg) ; 
    int &x1 = get<X1>(mArg) ; 
    int &y1 = get<Y1>(mArg) ;   

    int dy = y1 - y0; 
    int dx = x1 - x0; 

    if (abs(dx) >= abs(dy)) { 

        if (dx < 0) { 
            swap(x0, x1);
            swap(y0, y1);
        }   

        dy = y1 - y0; 
        if (dy >= 0) { 
                mpA = new slopePositiveOne(mArg,point) ; 
        }   
        else { 
                mpA = new slopeNegativeOne(mArg,point) ; 
        }   
    }   
    else { 
        if (dy < 0) { 
                swap(x0,x1) ; 
                swap(y0,y1) ; 
        }   
        dx = x1 - x0 ; 
        if (dx >= 0) {
                mpA = new slopePositiveInfinity(mArg,point) ; 
        }   
        else { 
                mpA = new slopeNegativeInfinity(mArg,point) ; 
        }   
    }   
}

Edit: changes as suggested by @moorecm & jsmith

[moorecm]

I took a brief look and have a few thoughts. I'm assuming that the algorithm is working so I didn't spend much time on that.

The use of the pattern looks good. Some of the names aren't completely clear but I am looking at this out of context; some informative comments could really help.

The use of explicit seems kind of odd, unless I'm mistaken, wouldn't that suppress implicit type conversions for the constructor's arguments. Wouldn't that mean that the constructor won't take 1 as a float argument and rather require 1.0? If that's what happens, it might be more of a hassle than anything.

The tuple doesn't seem to provide much, either. I like how the enumeration is used to index it, but this wouldn't be necessary and arguably be less obfuscated without it. It also adds boost as a requirement for the project (if it wasn't already). It's nice that it saves a little typing but, personally, I would just list the arguments. Unless you plan to use templates and/or swap out the types often.

I think the Shapes and Line classes still need reviewed. For example, why is there a drawLine() method in Line rather than a more generic virtual draw() method declared in Shapes? What is the purpose for the Shapes base class?

Overall, I dig it. Nice explanation and nice work.

[jsmith]

It is not good to call a virtual function in a constructor.

[kevinchkin]

Thanks so much for the reply guys .

@moorecm: You are absolutely right. I don't need explicit because of the fact that there are two arguments to the constructor. So there can never be an implicit type conversion. As for tuple, I am using it because I thought that it won't look good if I pass in 10 arguments to a function. Actually, I am using tr1::tuple from #include <tr1/tuple>, which I think is now in the standard no? I added Shapes class because I thought that eventually when I add some more subclasses to it like triangles etc, it would be a good overall hierarchy. And virtual draw seems like much better idea than what I have right now.

@jsmith: Right now I am invoking it as

Shapes *sp = new Lines(tpl,2.0) ; 

Should I change above to:

Shapes *sp = new Lines(tpl,2.0);
sp->draw() ; // and from draw call calculatePoints by making calculatePoints as public? 
                       // could you please suggest what change should I make?    

Please let me know.

[jsmith]

But you need (want?) explicit because all but one of the parameters have a default, so there can be
implicit conversion.

You just need to not call virtual methods during construction because the vtables have not yet been set up.
Doing what you said above would work, though it might be suboptimal if you call calculatePoints() every
time draw() is called, but the result of calculatePoints() never changes.

EDIT: as for 10 params, what you have now doesn't really change anything. For example:

// Passing 10 parameters:
slopePositiveOne s( 1, 2, 3, 4, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0 );

// vs.

// Passing 1 tuple:
slopePositiveOne s( midPointArg( 1, 2, 3, 4, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0 ) );

Which one is easier to read?

[kevinchkin]

@jsmith: Yes, you are right, I will change it back to explicit. And as for tuples, I am very new to it. I thought that whenever we have a situation like I had above, I should go for tuple. But if that's not the right way, then could you please tell me when exactly should I use tuples?

[jsmith]

Tuples are good when you want to aggregate a bunch of values into a simple value class but you don't want to
bother writing your own struct/class for it.

The general drawback to tuples is that the values it aggregates lose meaning because they don't have
explicit variable "names": the 0th element of a tuple is accessed by get<0>() instead of by "x_coordinate".

You mentioned:

As for tuple, I am using it because I thought that it won't look good if I pass in 10 arguments to a function.

and I was just pointing out that you didn't really solve the problem.

The problem now is that whoever gets to use the values contained in midPointArg have to access them by
get<0>(), get<1>(), etc, which makes their code non-self-documenting and probably meaningless without
a ton of comments.

[kevinchkin]

The problem now is that whoever gets to use the values contained in midPointArg have to access them by
get<0>(), get<1>(), etc, which makes their code non-self-documenting and probably meaningless without
a ton of comments.

Even if I use enum's to access the elements? :-(

        enum { X0, Y0, X1, Y1, R0, G0, B0, R1, G1, B1 } ; 
        get<X0>(tpl); // ??