6
\$\begingroup\$

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 <iostream>
#include <math.h>
#include <time.h>
#include <stdlib.h>

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 ){ ;}
    float calc_area( );
    void print( ) { 
        cout << "Line A formed from points\n"; A.print( ); 
        cout << "Line B formed from points\n"; B.print( ); 
        cout << "Line C formed from points\n"; C.print( );
    }
    private:
    Line A;
    Line B;
    Line C;
};

float Triangle::calc_area( ) {
    Point mid;
    C.mid_point( mid );
    Line height( mid, A.getB( ) );
    return 0.5*C.length( )*height.length( );
}

int main( int argc, char *argv[ ] ) {
    srand( time( NULL ) );
    //Calculate area of 10 triangles;
    for( int i=0;i<10;i++ ) {
        Point p1( (rand( )%10)/10.0,(rand( )%10)/10.0 );
        Point p2( (rand( )%10)/10.0,(rand( )%10)/10.0 );
        Point p3( (rand( )%10)/10.0,(rand( )%10)/10.0 );
        Line l1( p1,p2 );
        Line l2( p2,p3 );
        Line l3( p3,p1 );
        Triangle t( l1,l2,l3 );
        t.print( );
        cout << "Area = " << t.calc_area( ) << endl << endl;
    }
}

Output: (in the case you don't want to copy and pasted the code)

[mehoggan@desktop triangle]$ ./tri_area 
Line A formed from points
(0.4, 0.2) 
(0.6, 0.4) 
Line B formed from points
(0.6, 0.4) 
(0.8, 0.6) 
Line C formed from points
(0.8, 0.6) 
(0.4, 0.2) 
Area = 0

Line A formed from points
(0.5, 0.2) 
(0.6, 0.3) 
Line B formed from points
(0.6, 0.3) 
(0.5, 0.9) 
Line C formed from points
(0.5, 0.9) 
(0.5, 0.2) 
Area = 0.0942404

Line A formed from points
(0.5, 0.9) 
(0.5, 0.8) 
Line B formed from points
(0.5, 0.8) 
(0.1, 0.2) 
Line C formed from points
(0.1, 0.2) 
(0.5, 0.9) 
Area = 0.129059

Line A formed from points
(0.3, 0.1) 
(0.7, 0.2) 
Line B formed from points
(0.7, 0.2) 
(0.5, 0.4) 
Line C formed from points
(0.5, 0.4) 
(0.3, 0.1) 
Area = 0.0548293

Line A formed from points
(0.8, 0.1) 
(0.2, 0.5) 
Line B formed from points
(0.2, 0.5) 
(0.8, 0.3) 
Line C formed from points
(0.8, 0.3) 
(0.8, 0.1) 
Area = 0.067082

Line A formed from points
(0.5, 0.8) 
(0.2, 0.2) 
Line B formed from points
(0.2, 0.2) 
(0.8, 0.3) 
Line C formed from points
(0.8, 0.3) 
(0.5, 0.8) 
Area = 0.166208

Line A formed from points
(0.6, 0.2) 
(0.8, 0.5) 
Line B formed from points
(0.8, 0.5) 
(0.4, 0.2) 
Line C formed from points
(0.4, 0.2) 
(0.6, 0.2) 
Area = 0.0424264

Line A formed from points
(0.1, 0.3) 
(0.1, 0) 
Line B formed from points
(0.1, 0) 
(0.4, 0.9) 
Line C formed from points
(0.4, 0.9) 
(0.1, 0.3) 
Area = 0.20744

Line A formed from points
(0.2, 0.4) 
(0.6, 0.9) 
Line B formed from points
(0.6, 0.9) 
(0.3, 0.9) 
Line C formed from points
(0.3, 0.9) 
(0.2, 0.4) 
Area = 0.109659

Line A formed from points
(0.1, 0.4) 
(0.1, 0.3) 
Line B formed from points
(0.1, 0.3) 
(0.8, 0.5) 
Line C formed from points
(0.8, 0.5) 
(0.1, 0.4) 
Area = 0.134629
\$\endgroup\$
8
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ A of triangle = 1/2*base*height, I understand my mistake about taking the midpoint because if the triangle is acute then the midpoint to the top point is not the perpendicular height. If you have a triangle that is not orthogonal to the coordinate axis how do you get the point co-linear to the base that would give you the height of the triangle? \$\endgroup\$ – Matthew Hoggan Jul 31 '11 at 21:47
  • \$\begingroup\$ It's simplest not to. The simplest formula for the area of the triangle when you have cartesian co-ordinates for its vertices is to use the half magnitude of the cross product of the vectors along two of its sides. In this case it is simply half of the absolute value of (x2 - x1)(y3 - y1) - (y2 - y1)(x3 - x1). See here: en.wikipedia.org/wiki/Triangle#Using_vectors \$\endgroup\$ – CB Bailey Jul 31 '11 at 21:57
  • \$\begingroup\$ In the first bit of code, operator( should be operator<<(. \$\endgroup\$ – Anton Golov Jan 11 '12 at 17:59
5
\$\begingroup\$

Reviewed and edited under Windows 7, compiles in VS10 SP1. Take the comments and changes with a grain of salt and use what you deem are good changes :)

/********************************************************************
 * 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.
 * *****************************************************************/
 // .h headers are deprecated, use the headers with the "c" prefix like so.
#include <iostream>
#include <cmath>
#include <ctime>
#include <ostream>

// No using namespace std. Since this is a .cpp this is acceptable, but 
// here is a rule of thumb I found useful that lets me avoid namespace std
// even in .cpp files:
// - Write "using std::xxx" locally if you are using xxx more than three times.
// - Write "using std::xxx" globally if many functions are using xxx.
// - Write "using namespace std" locally if you are using 3 or more std:: functions more than three times.
// - Write "using namespace std" globally if you are lazy :D

// Reviewing your class design, I would like to stress some things:
// You are using the space R^2 (two dimensions, real numbers).
// I do not see this constraint in your problem definition above. Consider how you would expand this class
// to multiple dimensions as well as the realm of complex numbers (for which the length and other functions is slightly
// different).

// You did not define any operator overloads for things such as addition, subtraction, etc.
// Consider if it's worth implementing these functions, especially with vectors.

class Point {
public:
    // You could default-initialize the second ctor and remove the need for the first one.
    Point( float x = 0, float y = 0 ) 
        : x_( x ), y_( y )
    { } // You do not need semicolons inside an empty function body.

    // C++ usually doesn't prefix getters with "get". Again, depends on the style guide and personal preference.
    float x( ) const 
    { return x_; } // Getters do not mutate your members, make it const!

    float y( ) const 
    { return y_; } 

    void setX( float x ) { x_ = x; } // Parameters are usually lower-case in C++.
    void setY( float y ) { y_ = y; }

    // Personal preference, but give me a space between all those symbols! Otherwise it's hard to read.
    void print( ) const 
    {  } 

private:
    float x_; // Members are usually of the form m_X, m_x, or x_ to distinguish them from local or parameter variables.
    float y_;
};

std::ostream& operator<<(std::ostream& os, Point const& point)
{
    os << "(" << point.x() << ", " << point.y() << ") " << std::endl;
    return os;
}

class Line {
public:
    // Consider a default constructor. 
    // We would then have, essentially, a line of length zero, or a point.
    Line( Point a = Point(), Point b = Point() ) 
        : a_( a ), b_( b )
    { }

    // Why is this function outside the class body whereas all others are inside? 
    // You are mutating mid, something a user of your library might not be aware of.
    // I recommend giving this function a return value of the midpoint instead.
    Point midpoint( ) const
    {
        return Point ((a_.x( ) + b_.x( )) / 2.0,
                      (a_.y( ) + b_.y( )) / 2.0);
    }

    // Again, why have this function outside the class and not the others?
    float length( ) const
    {
        // Length of a line is of the form:
        // sqrt( (P2_x - P1_x)^2 + (P2_y - P1_y)^2 + ... + (P2_n - P1_n)^2 )
        // where n is the number of coordinates of the point.
        // I mention this because you are using only two dimensions, but many more are possible and used.

        // This will not work, think about arithmetic operators for your Point class.
        // return sqrt((a - b) * (a - b));

        // (P2_x - P1_x)^2 = (P2_x - P1_x) * (P2_x - P1_x)
        // Do not use pow in this case. We are using integers and pow() uses doubles or floats.

        // sqrt uses doubles or floats. Depending on the accuracy, you want to change the cast (and the program) to a double.
        return sqrt (static_cast<float>((b_.x() - a_.x()) * (b_.x() - a_.x()) + 
                                         (b_.y() - a_.y()) * (b_.y() - a_.y())));
    }

    Point const& a( ) const
    { return a_; }

    Point const& b( ) const
    { return b_; }

private:
    Point a_;
    Point b_;
};

std::ostream& operator<<(std::ostream& os, Line const& line)
{
    os << line.a() << line.b();
    return os;
}

class Triangle {
public:
    // Triangle of size zero? :D
    Triangle( Line a = Line(), Line b = Line(), Line c = Line() ) 
        : a_( a ),  b_( b ), c_( c )
    { }

    // Since the area of a triangle is just called "area" in mathematics, 
    // why not call the function the same?
    float area( ) {
        // We are not modifying mid or height past the initialization.
        // Might as well const it.
        // I also changed the ctor call for height, this prevents the "most vexing parse" problem
        // as well as me being able to align the "=" :3
        const Point mid   = c_.midpoint();
        const Line height = Line( mid, a_.b( ) );

        return static_cast<float>(0.5 * c_.length( ) * height.length( ));
    }

    // You used accessors for Line and Point, but why not for Triangle?
    Line const& a() const { return a_; }
    Line const& b() const { return b_; }
    Line const& c() const { return c_; }

private:
    Line a_;
    Line b_;
    Line c_;
};

// Non-member non-friend for greatest encapsulation power :3
// Using an operator<< allows us to not just write to console, but moar! D:
std::ostream& operator<<(std::ostream& os, Triangle const& triangle)
{
    os << "Line A formed from points\n" << triangle.a() 
       << "Line B formed from points\n" << triangle.b()
       << "Line C formed from points\n" << triangle.c();

    return os;
}

int main( int argc, char *argv[ ] ) {
    srand( time( NULL ) );

    // Calculate area of 10 triangles;
    // For non-pod types (e.g. iterators), it is better to use post-increment (++i).
    // I like to use it even on POD types just so I'm in the habit of using post-increment.
    for( int i = 0; i < 10; ++i ) {
        // Remember that this is a non-uniform distribution that gets worse as you
        // increase the range of the rand (unless you remove the modulo division, of course).
        // With such small numbers and for the intents of your program, this is negligible.
        Point p1( (rand( ) % 10) / 10.0,(rand( ) % 10) / 10.0 );
        Point p2( (rand( ) % 10) / 10.0,(rand( ) % 10) / 10.0 );
        Point p3( (rand( ) % 10) / 10.0,(rand( ) % 10) / 10.0 );

        Line l1( p1,p2 );
        Line l2( p2,p3 );
        Line l3( p3,p1 );

        Triangle t( l1,l2,l3 );
        std::cout << t;

        std::cout << "Area = " << t.area( ) << "\n\n";
    }

    return 0;
}
\$\endgroup\$
2
\$\begingroup\$

A couple of points:

#include <math.h>
#include <time.h>
#include <stdlib.h>

There are C++ versions of these headers:

#include <cmath>
#include <ctime>
#include <cstdlib>

I know every beginners guide in the world says:

using namespace std;

Don't do it. If you want to import something specific use:

using std::cin;
using std::cout;  // or whatever you to import.

If you are going to do this then try and scope the using to the closes scope that makes sense. But there is no real reason even to do this. Just prefix stuff with std::.

std::cout << "Print a line\n";

Don't break encapsulation with get/set

float getX( ) { return x; }
float getY( ) { return y; }
void setX( float X ) { x = X; }
void setY( float Y ) { y = Y; }

This is just a bad idea. I know it looks like you are protecting encapsulation, but you are not. What you are actually doing is binding your implementation to an expectation of what it needs to provide. What you should be doing is providing methods that use the internal members (that's what encapsulation is about).

Is a triangle really 3 lines?

Triangle( Line A,Line B,Line C ) : A( A ), B( B ), C( C ){ ;}

I think a triangle is really better represented as 3 points. What if A and B are parallel!
This also makes several of the other methods easier to write. It seems like you be able to get the distance between two points by subtracting them or a distance method (thus you don't need to expose their internal representation with getX() and getY()).

It's OK to write a print method. But you should proably also provide the output stream operator.

void print( ) { cout << "("<<x<<", "<<y<<") "<<endl; }

I would do this:

class Point
{
    public:
      void print(std::ostream& str)  const  // Notice the cost
      { 
          str << "(" << x << ", " << y << ") " << std::endl;
      }
};
std::ostream& operator(std::ostream& str, Point const& data)
{
    data.print(str);
    return str;
}

PS. This is not correct:

float Triangle::calc_area( ) {
    Point mid;
    C.mid_point( mid );
    Line height( mid, A.getB( ) );
    return 0.5*C.length( )*height.length( );
}

The height line must be perpendicular to the C-Line.

\$\endgroup\$
1
\$\begingroup\$

I am not an expert on modern c++ but this is a hint I would include:

Instead of having the method print that always prints to cout, I would also include a method where it receives the stream to print to. Something like this for point (not tested):

void print(std::ostream &s){ s << "("<<x<<", "<<y<<") "<<endl;}

and for line:

void print(std::ostream &s){ a.print(s); b.print(s);}

So in this way you can print to cout, cerr or a file if you want :)

Hope this helps

\$\endgroup\$
  • \$\begingroup\$ This seems like a trick that most people are showing me. I will adopt it into my style to make things more C++ like. Thanks -matt \$\endgroup\$ – Matthew Hoggan Jul 31 '11 at 9:10
  • 1
    \$\begingroup\$ @Matthew: I would go a step further and do it the C++ way by defining the operator<< for your types. \$\endgroup\$ – Martin York Jul 31 '11 at 22:30
  • \$\begingroup\$ @Martin: Yes, I thought about that later on. \$\endgroup\$ – Oni Jul 31 '11 at 22:40

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.