HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Drawing superellipses

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
drawingsuperellipsesstackoverflow

Problem

Here is a piece of code that draws superellipses:

```
#include "std_lib_facilities.h"
#include
#include "Simple_window.h"

#define PI 3.14159265359

template
int sgn(T val);

vector 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 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 =, respectivelly.
*/
template
int sgn(T val){
return (T(0) generateSuperEllipse(double coeffA, double coeffB,
double expN, double expM,
int centX, int centY,
double precisio

Solution

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.

Code Snippets

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));
}
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;
}
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();

Context

StackExchange Code Review Q#104026, answer score: 2

Revisions (0)

No revisions yet.