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

Striped closed poly-line

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

Problem

I'm working on class Striped_polyline which represents a closed poly-line filled with equidistant horizontal lines1:

stripedpoly.cpp:

#include  
#include 
#include 
#include "Graph.h"
#include "Simple_window.h"
#include "stripedpoly.h"

int main(){
    Point tl(x_max()/2,0);
    int width = 700;
    int height = 700;
    string label = "Striped Closed poly-line";
    Simple_window sw(tl, width, height, label);
    try{ 
        // generate points for the poly-line
        vector polyPoints;
        generatePoints(polyPoints);
        // create a poly-line
        Striped_closed_polyline scp;
        for (auto it = polyPoints.begin(); it != polyPoints.end(); ++it) scp.add(*it); 
        scp.set_spacing(5);
        sw.attach(scp);
        sw.wait_for_button();
    }catch(exception& e){
        cerr << e.what() << endl;
        getchar();
    }catch(...){
        cerr <<"Default exception!"<< endl;
        getchar();
    }
}


stripedpoly.h:

```
// Helper functions
static void generatePoints(vector& p){
p.push_back(Point(50,50));
p.push_back(Point(200,50));
p.push_back(Point(250,100));
p.push_back(Point(200,200));
p.push_back(Point(100,225));
p.push_back(Point(50,200));
p.push_back(Point(25,100));
}
// compare function for sorting by the x-coordinates of a Point
struct XLessThan{
inline bool operator()(Point& p1, Point& p2){ return (p1.x p1.y) std::swap(start, end);
yMin = start.y;
yMax = end.y;
}
bool operator= 0 && s = 0 && t polyEdges;
Point first = point(0);
Point last = point(number_of_points()-1);
polyEdges.push_back(Edge(last, first));
for (size_t i = 1; i polyPoints2;
for (size_t i = 0; i intersections;
for (size_t i = 0; i < polyEdges.size(); ++i){
for (size_t j = polyEdges[i].yMin; j < polyEdges[i].yMax; j += spacing){
Edge horizontal(Point(polyXmin, j), Point(polyXmax, j));
// check if polyEdge not horizontal
if (polyEd

Solution

Don't use using namespace std;. Seriously

The lack of std:: implies you have using namespace std; in a header somewhere. This is bad for the following reasons:

-
Having it localized to one translation unit is one thing, but putting it into a header propogates it to whoever chooses to include your header. This is a Bad Thing™ in of itself.

-
Many trivial programs will not suffer from name conflicts; but a program that deals with shapes and drawing increases the chances you'll have a name conflict like distance or vector.

-
Using a third party library (if you are) increases the chances of conflicts exponentially. If you wrote it yourself, consider a namespace akin to std or sfml.

Where are your newlines and spaces?

Your code is incredibly hard to read. I'm not going to tell you specifically how you should format your code, but at least consider formatting it to increase readability.

emplace_back over push_back

push_back will delegate to emplace_back. The advantage emplace_back has over push_back is that it will forward the arguments to the constructor, so you don't have to specify the type.

polyEdges.emplace_back(last, first);


Explain your algorithm; choose better variable names

intersectPoint is incredibly difficult to read. p0_x is easy to deduce, but what is s1_x? What is this line doing?

s = (-s1_y * (p0_x - p2_x) + s1_x * (p0_y - p2_y)) / (-s2_x * s1_y + s1_x * s2_y);


Even putting the name of the algorithm in a comment will give the reader enough context to figure it out.

Redundancy

You have wrapper functions that pass arguments along but this is pointless. Why duplicate the interface? As you can imagine, the more granular you get, the more bloated your interface will be.

Code Snippets

polyEdges.emplace_back(last, first);
s = (-s1_y * (p0_x - p2_x) + s1_x * (p0_y - p2_y)) / (-s2_x * s1_y + s1_x * s2_y);

Context

StackExchange Code Review Q#107419, answer score: 7

Revisions (0)

No revisions yet.