patterncppMinor
Compute random triplets of float pairs and average area of generated triangles
Viewed 0 times
randomgeneratedtripletscomputefloataverageandpairsareatriangles
Problem
I am trying to get in a lot of practice for interviews so I am trying to write as much code as possible, and getting feed back from excellent programmers like "you". Can I get some general feed back on this small bit of code? Any recommendations on style, good practice, etc. is greatly appreciated.
Normally I would put the class declarations in their own header files, and the definitions in a .cpp file, write a make file and build using make. However, this is a small program so I just place everything inside one module.
```
/****
* Define a data type for triangles in the unit square, including a
* function that computes the area of a triangle. Then write a client
* program that generates random triples of pairs of floats between 0
* and 1 and computes the average area of the triangles generated.
/
#include
#include
#include
#include
using namespace std;
class Point {
public:
Point( ) { ;}
Point( float x, float y ) : x( x ), y( y ){ ;}
float getX( ) { return x; }
float getY( ) { return y; }
void setX( float X ) { x = X; }
void setY( float Y ) { y = Y; }
void print( ) { cout << "("<<x<<", "<<y<<") "<<endl; }
private:
float x;
float y;
};
class Line {
public:
Line( Point a, Point b ) : a( a ), b( b ){ ;}
void mid_point( Point &mid );
float length( );
Point getA( ) { return a; }
Point getB( ) { return b; }
void print( ) { a.print( ); b.print( ); }
private:
Point a;
Point b;
};
void Line::mid_point( Point &mid ) {
mid.setX( (a.getX( )+b.getX( ))/2.0 );
mid.setY( (a.getY( )+b.getY( ))/2.0 );
}
float Line::length( ) {
return sqrt( pow( a.getX( )-b.getX( ),2.0 ) +
pow( a.getY( )-b.getY( ),2.0 )
);
}
class Triangle {
public:
Triangle( Line A,Line B,Line C ) : A( A ), B( B ), C( C ){ ;}
floa
Normally I would put the class declarations in their own header files, and the definitions in a .cpp file, write a make file and build using make. However, this is a small program so I just place everything inside one module.
```
/****
* Define a data type for triangles in the unit square, including a
* function that computes the area of a triangle. Then write a client
* program that generates random triples of pairs of floats between 0
* and 1 and computes the average area of the triangles generated.
/
#include
#include
#include
#include
using namespace std;
class Point {
public:
Point( ) { ;}
Point( float x, float y ) : x( x ), y( y ){ ;}
float getX( ) { return x; }
float getY( ) { return y; }
void setX( float X ) { x = X; }
void setY( float Y ) { y = Y; }
void print( ) { cout << "("<<x<<", "<<y<<") "<<endl; }
private:
float x;
float y;
};
class Line {
public:
Line( Point a, Point b ) : a( a ), b( b ){ ;}
void mid_point( Point &mid );
float length( );
Point getA( ) { return a; }
Point getB( ) { return b; }
void print( ) { a.print( ); b.print( ); }
private:
Point a;
Point b;
};
void Line::mid_point( Point &mid ) {
mid.setX( (a.getX( )+b.getX( ))/2.0 );
mid.setY( (a.getY( )+b.getY( ))/2.0 );
}
float Line::length( ) {
return sqrt( pow( a.getX( )-b.getX( ),2.0 ) +
pow( a.getY( )-b.getY( ),2.0 )
);
}
class Triangle {
public:
Triangle( Line A,Line B,Line C ) : A( A ), B( B ), C( C ){ ;}
floa
Solution
using namespace std; is almost always a bad idea. Even in small programs I would avoid it.Your algorithm for
calc_area is just plain wrong. The area of a triangle is half the length of a side multiplied by the perpendicular distance from the third vertex to the base line, not the distance from the vertex to the mid-point of the base. (Think of a triangle with vertices at (-1,0), (1,0) and (1, 1). The area of this triangle should be 1, not sqrt(2).)Reviewing your
Point class, the default constructor doesn't initialized the member variables. This may be acceptable for performance reasons - for example - if you deliberately want to be able to create large arrays of uninitialized Points but it's often safer to explicitly initialize all class members in a constructor.Having both getters and setters for
x and y effectively makes them public data members. The only functionality that Point has is print but this can be provided as a non-member function. Once you've done this your class provides no functionality that a simple struct doesn't. In addition, you can use aggregate initialization for a struct which can be useful.E.g.
struct Point
{
float x;
float y;
};
// print functionality
std::ostream& operator( std::ostream& os, const Point& point )
{
os << "(" << point.x << ", " << point.y << ") " << std::endl;
}This is a lot simpler, although you would have to change initializations.
// Point p( p1, p2 );
Point p = { p1, p2 };One disadvantage is that you can't easily construct a temporary
Point with explicit initial values. If you need to do this you could consider a helper function analogous to std::make_pair.E.g.
inline Point MakePoint( int px, int py )
{
Point p = { px, py };
return p;
}
int main()
{
// FunctionTakingPoint( Point( 1, 2 ) );
FunctionTakingPoint( MakePoint( 1, 2 ) );
}Being a trivial POD-struct, most compilers will have little difficulty in eliding most of the implied copies.
There is some argument that
Line deserves to be a class as you have no setters for its members, but given that it has little behaviour and the behaviour it has can be provided by free functions I would keep it as a POD struct. Clients can choose to make a const instance should they so choose.Also, I don't see any need to make
mid_point take a reference to a struct. It can return by value for more readable code.struct Line
{
Point a;
Point b;
};
Point mid_point( const Line& line )
{
Point p = { ( line.a.x + line.b.x ) / 2.0,
( line.a.y + line.b.y ) / 2.0 };
return p;
}
double length( const Line& line )
{
double xdiff = line.b.x - line.a.x;
double ydiff = line.b.y - line.a.y;
return sqrt( xdiff * xdiff + ydiff * ydiff );
}
std::ostream& operator<<( std::ostream& os, const Line& line )
{
os << line.a << line.b;
}Code Snippets
struct Point
{
float x;
float y;
};
// print functionality
std::ostream& operator( std::ostream& os, const Point& point )
{
os << "(" << point.x << ", " << point.y << ") " << std::endl;
}// Point p( p1, p2 );
Point p = { p1, p2 };inline Point MakePoint( int px, int py )
{
Point p = { px, py };
return p;
}
int main()
{
// FunctionTakingPoint( Point( 1, 2 ) );
FunctionTakingPoint( MakePoint( 1, 2 ) );
}struct Line
{
Point a;
Point b;
};
Point mid_point( const Line& line )
{
Point p = { ( line.a.x + line.b.x ) / 2.0,
( line.a.y + line.b.y ) / 2.0 };
return p;
}
double length( const Line& line )
{
double xdiff = line.b.x - line.a.x;
double ydiff = line.b.y - line.a.y;
return sqrt( xdiff * xdiff + ydiff * ydiff );
}
std::ostream& operator<<( std::ostream& os, const Line& line )
{
os << line.a << line.b;
}Context
StackExchange Code Review Q#3757, answer score: 8
Revisions (0)
No revisions yet.