3
\$\begingroup\$

Here is a piece of code that draws superellipses:

#include "std_lib_facilities.h"
#include <iostream>
#include "Simple_window.h"

#define PI 3.14159265359

template <typename T> 
int sgn(T val);

vector<Point> generateSuperEllipse(double CoeffA, double CoeffB, double expN, double expM, int centX, int centY, double precision);
/*
    Class: SuperEllipse
    Each initialized object
    generates a superellipse
    using the parameters of the
    constructor. The two member
    functions draw the generated
    points.
*/
class SuperEllipse{
    public:
    SuperEllipse(double CoeffA, double CoeffB, double expN, double expM, int centX, int centY, double precision){
        // generate ellipse points
        coordinates = generateSuperEllipse(CoeffA, CoeffB, expN, expM, centX, centY, precision);
    }
    // draw a Star-like pattern connecting superellipse's single point to the rest of its equally spaced points 
    void drawStar();
    // draw a superellipse out of the generated points contained in private member coordinates
    void drawSuperEllipse();
private: 
    vector<Point> coordinates;
};

//------------------------------------------------------------------------------------------------------------------------------
int main(){
    try{
    // Superellipse parameters
    // exponents
    double n = 2/3.;
    double m = 2/3.;
    // coefficient 
    double A = 200.0;
    double B = 200.0;
    // center point of the graph
    const int centX = x_max() / 2.;
    const int centY = y_max() / 2.;
    // incrementation step
    double di = 0.01;

    SuperEllipse se(A, B, n, m, centX, centY, di);
    se.drawSuperEllipse();
    // push next button to see the star
    se.drawStar();

}catch(exception& e){
    cerr << e.what() <<endl;
    getchar();              
}catch(...){
    cerr <<"Default exception!"<<endl;
    getchar();              
}
return 0;
}
//------------------------------------------------------------------------------------------------------------------------------
/*
    Function: sgn()
    Use: int singedOneorZero = sgn(anyType);
    Returns either -1, 0 or 1 if the input
    value is < 0 , == 0 or > =, respectivelly.
*/
template <typename T> 
int sgn(T val){
    return (T(0) < val) - (val < T(0));
}

/*
    Function: generateSuperEllipse()
    Use: vector<Point> generateSuperEllipse(double coeffA, double coeffB, 
                                    double expN, double expM, 
                                    int centX, int centY, 
                                    double precision)
    This function generates a set of superellipse coordinates based
    on the following parametric equations:
    x(theta) = coeffA * pow(abs(cos(theta)), 2 / n) * sgn(cos(theta)) + centX;
    y(theta) = coeffB * pow(abs(sin(theta)), 2 / m) * sgn(sin(theta)) + centY;
    and returns a vector containing them.
*/
vector<Point> generateSuperEllipse(double coeffA, double coeffB, double expN, double expM, int centX, int centY, double precision = 0.01){
if(precision < 0.01 || precision > 1) error("Precision out of range!");

vector<Point> superEllipseCoordinates;
Point temp, sEllipseCoordinate;

for(double d = -1; d < 1; d += precision){
        double theta = d * PI; 
        int x = coeffA * pow(abs((double)cos(theta)), (double)2.0 / expM) * sgn(cos(theta));
        int y = coeffB * pow(abs((double)sin(theta)), (double)2.0 / expN) * sgn(sin(theta));
        sEllipseCoordinate = Point(x + centX/2. , y + centY);
        // check for duplicate points
        if(sEllipseCoordinate != temp) superEllipseCoordinates.push_back(sEllipseCoordinate);
        temp = sEllipseCoordinate;  
    }
return superEllipseCoordinates;
}


// Class Superellipse members implementation
/*
    Member Function: drawStar()
    Use: SuperEllipse sEobject.drawStar();
    Generates a Simple_window 
    object and draws a star-like pattern
    by connecting a single one of 
    the, already generated superellipse
    points to the equally spaced remaining point.
    Finally it attaches the drawing
    to the window object.
*/
void SuperEllipse::drawStar(){
    const int swWidth = 800;
    const int swHeight = 800;
    Simple_window sw(Point((x_max()- swWidth) / 2. , (y_max() - swHeight) / 2.), 
                 swWidth, swHeight, "Chapter 12 Exercise 12");
    // If the initial value of the counter: i is not divided by a number (100) it connects 
    // a lot of points and results in filling the superellipse 
    for(size_t i = coordinates.size() / 100; i < coordinates.size(); ++i){
        for(size_t j = i+1; j < coordinates.size(); ++j){
            Graph_lib::Line* star = new Line(Point(coordinates[0]),Point(coordinates[j]));
            sw.attach(*star);
        }
    }
    sw.wait_for_button();
}

/*
    Member Function: drawSuperEllipse();
    Use: SuperEllipse sEobject.drawSuperEllipse();
    Generates a Simple_windoq object
    and draws an open polyline of the 
    coordinates passed as parameter (in the case
    superellipse). Finally it attaches the drawing
    to the window object.
*/
void SuperEllipse::drawSuperEllipse(){
    const int swWidth = 800;
    const int swHeight = 800;
    Simple_window sw(Point((x_max()- swWidth) / 2. , (y_max() - swHeight) / 2.), 
                 swWidth, swHeight, "Chapter 12 Exercise 12");

    Graph_lib::Open_polyline superEllipse;
    for(size_t i = 0; i < coordinates.size(); ++i)  superEllipse.add(coordinates[i]); 
        sw.attach(superEllipse);
        sw.wait_for_button();
}

Drawing Results:

Superellipse:

Superellipse

Star-like pattern:

Star-like pattern

Questions:

  1. Is there a better way to implement the two drawing member function without incorporating the Simple_window sw object inside the functions (possibly passed as parameter or part of the class)?
  2. Any remarks regarding class and member function implementation?
  3. Any style improvement notes?
  4. How can I delete the dynamically allocated memory in function drawStar()? (Should it be done at the end of the function or as part of an explicitly defined destructor?)

All the needed files for compilation are here. The FLTK can be found here.

\$\endgroup\$
  • \$\begingroup\$ I started to write an answer. Is it possible to modify the code you link to? At the very least, the Window class needs a few modifications. \$\endgroup\$ – Armaghast Sep 7 '15 at 17:01
  • \$\begingroup\$ @Armaghast in the above code all the included headers don't need any modification (specifically window is not used directly and the modification is not recommended ). If you have problems running it: I would be happy to help, as they are indeed some fine points that need attention. \$\endgroup\$ – Ziezi Sep 7 '15 at 17:05
2
\$\begingroup\$

Here are some observations that may help you improve your code.

Don't wrap your entire program in a try block

It's generally not good to have the entire body of main be a try block. The reason is that if an exception is thrown, it's more difficult for the reader of the program to figure out which statements might have thrown. We know that none of the simple variable declarations throw, so it seems to me (although we don't know the contents of the two include files) that only the last three lines might throw.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It is particularly bad to put it into a header file, so please don't do that.

Simplify your constructor

The constructor code is implemented in an odd two-part fashion with the constructor calling an external standalone function. Better would be to simply make the body of standalone function be the constructor body and eliminate the standalone function.

Move loop invariants out of the loop

A number of factors within the loop are potentially calculated many times. Your loop could be rewritten like this:

expN = 2.0 / expN;
expM = 2.0 / expM;
precision *= PI;
for (double theta = -PI; theta < PI; theta += precision) {
    int x = coeffA * pow(abs(cos(theta)), expM) * sgn(cos(theta));
    int y = coeffB * pow(abs(sin(theta)), expN) * sgn(sin(theta));
    superEllipseCoordinates.emplace_back(Point(x+centX/2., y+centY));
}

Note also that variable d is no longer needed and that I've used emplace_back with an inline constructor. There does not seem to be any real reason to look for duplicates or to name the value.

Consider an alternative form for draw and drawStar

Rather than creating (and destroying) a Simple_window object within your code, it might be better to do that outside and have those functions return something useable by a Simple_window. For drawStar that might look like this:

Graph_lib::Open_polyline SuperEllipse::getPolyline()
{
    Graph_lib::Open_polyline poly;
    for (size_t i = 0; i < coordinates.size(); ++i)
        poly.add(coordinates[i]);
    return poly;
}

Using it would be like this:

const int swWidth = 800;
const int swHeight = 800;
Simple_window sw(Point((x_max() - swWidth) / 2., (y_max() - swHeight) / 2.),
                     swWidth, swHeight, "Chapter 12 Exercise 12");
sw.attach(se.getPolyline());
sw.wait_for_button();

Don't leak memory

I don't know how your Graph_lib or Simple_window handle memory, but I see that drawStar() allocates memory with new but never frees it. This is suspicious and is probably a memory leak.

Beyond that, it's hard to say much because we don't know what's in the include files.

\$\endgroup\$
  • \$\begingroup\$ getPolyline can't work like this: you are returning nothing, and adding return superEllipse; would be a bug. I suggest a dynamically allocation, thus returning a Graph_lib::Open_polyline*, and then let the Window take ownership of its child, as usual (which is currently not the case). \$\endgroup\$ – Armaghast Sep 7 '15 at 18:04
  • \$\begingroup\$ @Armaghast: fixed now. Thanks for pointing that out. \$\endgroup\$ – Edward Sep 7 '15 at 18:07
  • \$\begingroup\$ @Edward Firstly thank you for your answer! The only way I can think off for freeing the dynamically allocated memory in drawStar() is if I save the created objects in a container (vector for example) and then run a for loop with delete followed by indexed element. Is there a other/ better way to free that memory leak? \$\endgroup\$ – Ziezi Sep 7 '15 at 19:13
  • 1
    \$\begingroup\$ Putting things in a container is probably the best way to do it. You could then return that container from the call (as I showed with getPolyline()) and not leak memory. \$\endgroup\$ – Edward Sep 7 '15 at 19:53
  • \$\begingroup\$ Still doesn't work, as Graph_lib::Open_polylinecannot be copied. Using dynamic allocation + return a (smart) pointer is the way to go - those methods are widget factory after all; the Window should clean the memory afterwards. \$\endgroup\$ – Armaghast Sep 8 '15 at 8:49
1
\$\begingroup\$

This answer is a correction to Edward's one (comments are simply not enough).

I fully agree on the "Don't wrap your entire program in a try block", "Don't abuse using namespace std", "Move loop invariants out of the loop", and will not repeat those points.

Simplify your constructor

You can keep generateSuperEllipse(), but make it a private & static method of SuperEllipse. Use member initialization:

SuperEllipse(double CoeffA, double CoeffB, double expN, double expM, int centX, int centY, double precision) : 
    coordinates(generateSuperEllipse(CoeffA, CoeffB, expN, expM, centX, centY, precision))  // generate ellipse points
{   
}

Thus you can make make coordinates a const vector<Point>.

Shape generation

Indeed, the drawStar()/drawSuperEllipse() should not handle the creation of the window. Those methods should return something that looks like a Graph_lib::Open_polyline, and let the caller add the shape to the window he/she wants to.

However, Edward's solution isn't working. A Graph_lib::Open_polyline cannot be copied (which is frequent for Widget-like elements):

Shape(const Shape&) = delete;

You need to use dynamic allocation (better here than a move, due to polymorphism):

Graph_lib::Open_polyline* SuperEllipse::makePolyline()
{
    Graph_lib::Open_polyline* poly = new Graph_lib::Open_polyline;
    for (size_t i = 0; i < coordinates.size(); ++i)
        poly->add(coordinates[i]);
    return poly;
}

Note that I renamed it makePolyline(), due to the dynamic allocation, as it carry the notion the caller now own the Graph_lib::Open_polyline*

Of course, makeStar should be using a Open_polyline

Onto fixing the memory leak

Graph_lib::Open_polyline is a Shape, which behaves very closely to a Widget in your library.

In this case, the parent is responsible for cleaning up its children's memory (which, in turn clean up their children memory, etc.). The container you are talking about already exists - it's Window vector<Shape*> shapes;.

2 steps are thus needed:

  • Windowshould have a method void attach(Shape* s); instead of void attach(Shape& s);

  • ~Window() should destroy every elements of vector<Shape*> shapes;:

    ~Window()
    {
        for(auto it = shapes.begin(); it != shapes.end(); it++)
            destroy *it;
    }
    

I suggest doing the same with void attach(Widget& w); and the void detach(...) methods

Note: I am now enforcing the rule "the Window owns the Shapes", which wasn't the case before. Thus, it can change the behavior of old code using this library.

An underlying issue

I believe Shape should inherit Widget, and the Window-related classes should only deal with Widgets. (void detach(Widget& w); is declared, yet not defined, so I guess you're not using Widgets)

But we're now refactoring code outside of what was posted. I am unsure whether your code review is only about the SuperEllipse class that was posted, or also about the code you link to.

Further improvement: explicit ownership/automatic deallocation with C++11

An improvement over my proposed solution is makePolyline() / makeStar() should return a std::unique_ptr :

std::unique_ptr<Graph_lib::Open_polyline> SuperEllipse::makePolyline()
{
     std::unique_ptr<Graph_lib::Open_polyline> poly(new  Graph_lib::Open_polyline>);
    for (size_t i = 0; i < coordinates.size(); ++i)
        poly->add(coordinates[i]);
    return poly;
} 

Likewise (assuming you keep Shapeand Widgetseparate), use std::vector<std::unique_ptr<Shape>> shapes; and void attach(std::unique_ptr<Widget> w); (as an overload) in Window: the memory is automatically cleaned up when ~Window() is called.

\$\endgroup\$
0
\$\begingroup\$

The following code reflects your both advices, please feel free to edit and

// non-used headers contained in std_lib_facilities.h, removed
// no explicit use of namespace std (except if its already in some of the headers connected to "Simple_window")
#include <cmath>
#include <stdexcept>
#include <string>
#include <vector>
#include <iostream>
#include "Simple_window.h"

#define PI 3.14159265359

class regularPolygon{
public:
    regularPolygon(Point centerOfPolygon, int numbOfSides, int size)
        : axisOfRotation(centerOfPolygon), numberOfRotations(numbOfSides), sizeOfSides(size)
        { generatePolygon(); }
    // explicitly defined destructor that frees the dynamically allocated memory for the polygons
    ~regularPolygon()
        { for(auto it = generatedPolygons.begin(); it != generatedPolygons.end(); it++) delete *it; }
    // member function returning a polygon object drawn using the vector: coordinates 
    Graph_lib::Polygon* makePolygon();
private:
    Point axisOfRotation;
    Point initial;
    int numberOfRotations;
    int sizeOfSides;
    float angRads;

    void rotateCoordinate(vector<Point>& rotated, Point& axisOfRotation, Point initial, 
                       float angRads, int numberOfRotations);
    // wrapper function (if it's declared as static the definition gets underlined in red)
    void generatePolygon();
    // container holding the generated coordinates
    vector<Point> coordinates;
    // container holding the generated polygons used to free the dynamically alocated memory
    // common to all instantiated objects
    static vector<Graph_lib::Polygon *> generatedPolygons;
};


//-----------------------------------------------------------------------------------------------------------------------

int main(){

    // simple_window object
    const int swWidth = 800;
    const int swHeight = 800;
    Simple_window sw(Point((x_max()- swWidth) / 2. , (y_max() - swHeight) / 2.), 
                     swWidth, swHeight, "Chapter 12 Exercise 11");

// error-handling only around the newly introduced objects  
try{
    // center (pivot) point 
    Point center(sw.x_max() / 2., sw.y_max() / 2.);
    // Equilateral triangle
    const int sideOfTriangle = 125;
    regularPolygon equilateralTriangle(center, 3, sideOfTriangle);
    sw.attach(*equilateralTriangle.makePolygon());
    // Square
    const int sizeOfSquare = sideOfTriangle;
    regularPolygon square(center, 4,  sizeOfSquare);
    sw.attach(*square.makePolygon());
    // Pentagon 
    const int sizeOfPentagon = sideOfTriangle;
    regularPolygon pentagon(center, 5, sizeOfPentagon);
    sw.attach(*pentagon.makePolygon());

    sw.wait_for_button();
    // should I call the class makePolygon() destructor explicitly, here?
}catch(exception& e){
    cerr << e.what() <<endl;
    getchar();              
}catch(...){
    cerr <<"Default exception!"<<endl;
    getchar();              
}
return 0;
}
//-----------------------------------------------------------------------------------------------------------------------

/*
    Member Function: rotateCoordinate()
    Use: Private member function wrapped by the
    function generatePolygon();
    This function performs the same rotation on
    a consevutively generated points, through recursion.
    These points are saved in a vector, passed
    by reference.
*/
void regularPolygon::rotateCoordinate(vector<Point>& rotated, Point& axisOfRotation, Point initial, 
                            float angRads, int numberOfRotations){
    // base case: when all rotations performed return vector holding the rotated points
    if(numberOfRotations <= 0) return;
    else{
        // apply transformation on the initial point
        int x = cos(angRads) * (initial.x - axisOfRotation.x) - sin(angRads) * (initial.y - axisOfRotation.y) + axisOfRotation.x;
        int y = sin(angRads) * (initial.x - axisOfRotation.x) + cos(angRads) * (initial.y - axisOfRotation.y) + axisOfRotation.y;
        // save the result
        rotated.push_back(Point(x, y));
        // call the same function this time on the rotated point and decremented number of rotations
        rotateCoordinate(rotated, axisOfRotation, Point(x,y), angRads, numberOfRotations -1);
    }
}

/*
    Member Function: generatePolygon()
    Use: Private function used in the body
    of the constructor to generate coordinates
    for the polygon.
*/
void regularPolygon::generatePolygon(){
    if(axisOfRotation.x < 0 || axisOfRotation.y < 0) error("Pivot point coordinates out of lower bound!");
    if(numberOfRotations < 3) error("Not enough polygon sides!");
    if(sizeOfSides < 0) error("Negative side lenght!");

    coordinates.reserve(numberOfRotations);
    // calculate the angles of rotations
    angRads = PI / 180. * (360. / numberOfRotations);  
    // coordinates of first point shifted by the side length (it should be by the bisector (to be done latter))  
    initial.x = axisOfRotation.x - sizeOfSides;
    initial.y = axisOfRotation.y - sizeOfSides; 
    // generate the coordinates of the polygon 
    rotateCoordinate(coordinates, axisOfRotation, initial, angRads, numberOfRotations);
}

/*
    Member Function: makePolygon();
    Use: Private function used to 
    create (return) a pointer to a polygon
    object created from the generated
    coordinates.
*/
Graph_lib::Polygon* regularPolygon::makePolygon(){
    Graph_lib::Polygon* poly = new Graph_lib::Polygon;
    for(size_t i = 0; i < coordinates.size(); ++i) poly->add(coordinates[i]);
    // collected in the container to be deleted at the end
    generatedPolygons.push_back(poly);
    return poly;
}

The only problematic thing is to declare the function generatePolygon() as static, it results in red underline across all of the body of definition, containing the error message: "static members should belong to objects"

\$\endgroup\$

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.