writing style critique

[fabricetoussain]

/* Fabrice Toussaint
   section N938
   Feburary 1, 2011
   Purspose of program is to locate centroid and find distance between all three points and centroid
   */
#include <iostream>// for cout and cin
#include <cmath> // for sqrt () and pow (x,y)
 

using namespace std;

int main () 
{
	float distance1; //  assign number of distance 1
	float distance2; // assign number of distance 2
	float distance3; // assign number of distance 3
	float centroidX; // x point of centroid
	float centroidY; // y point of centroid
	float xa; // point xa
	float xb; // point xb
	float xc; // point xc
	float ya; // point ya
	float yb; // point yb
	float yc; // point yc
	
	cout << " Triangle Centroid Calculator!" << endl; // welcome user
	cout << endl;
	
	cout << "Enter point one on your triangle: " << endl; // prompt user to enter two numbers
	cin >> xa >> ya;                                      // store numbers into variable xa and ya
	cout << "Enter point two on your triangle: " << endl; // prompt user to enter two numbers for second point
	cin >> xb >> yb;                                      // store numbers into variable xb and yb
	cout << "Enter point three on your triangle: " << endl; // prompt user to enter two numbers for third point
	cin >> xc >> yc;                                      // store number into variable xb and yb
	cout << endl;
	
	centroidX = (xa + xb + xc)/3;  // centroid equation for point X
	centroidY = (ya + yb + yc)/3;  // centroid equation for point Y
	cout << "The centroid is located at (" << centroidX << ',' << centroidY << ") " << endl; // distplay centroid coordinate to user
	cout << endl;
	
	
	
	distance1 = sqrt((pow((centroidX - xa), 2) + pow((centroidY - ya), 2))); // distance 1 from point 1 to centroid
	distance2 = sqrt((pow((centroidX - xb), 2) + pow((centroidY - yb), 2))); // distance 2 from point 2 to centroid
	distance3 = sqrt((pow((centroidX - xc), 2) + pow((centroidY - yc), 2))); // distance 3 from point 3 to centroid
	
	cout << "Distance from centroid to point 1: " << distance1 << endl; // prompt user of the distance1
	cout << "Distance from centroid to point 2: " << distance2 << endl; // prompt user of the distance2
	cout << "Distance from centroid to point 3: " << distance3 << endl; // prompt user of the distance3
	cout << endl;
	cout<< "Thank you for your business." << endl;
	
    return 0;
}

im fairly new to coding, and i just finished writing up this code for a centroid calculator, and i was just curious to have more advance coders have a look at this and critique me on my style or suggest any better ways to getting to the end.

what could i do to shorten it??
what could i do to maybe make it more readable?( i had to leave comments on everysingle line for teacher)

im open so just let me know your opinions on one of my first programs.

thank you
-fabrice toussaint

[pseudorandom21]

to make more readable: Functional decomposition, which involves breaking the program into segments and using functions for them. For instance, getting your input could be in it's own function.

Personally I would probably use an array of floats and enum values for indexing it.
ex:

enum //<-- anonymous enum
{
  DISTANCE_ONE = 0,
  DISTANCE_TWO,
  DISTANCE_THREE,
  BUFFER_SIZE
};
int main()
{
  float buffer[BUFFER_SIZE];
  buffer[DISTANCE_ONE] = 0.0f;
  buffer[DISTANCE_TWO] = 0.1f;
  //etc.
}

You certainly don't have to do it that way, it was just a random thought of mine.

[mike_2000_17]

Overall it is quite ok. I mean, you use indentation correctly, which is important. Your variable names are clear, which is also very important. You did put a lot of comments, which is important for the professor (but not really for any programmer that I know of).

It is a very small and simple piece of code, so there isn't much place for bad style or bad programming practices. But since you are asking for critique, I could have a few things to point out:

1)
In general, it's often better style to collect variable declarations near to where they are first defined (this isn't always possible though). You are using what some people would call C-style variable declarations, i.e., declaring all variables at the start of the function (it comes from an old standard in C that forced all variables to be declared at the start of the function, it's not the case anymore and hasn't been for 20 years or so). This is ok, even some experienced people do it this way, but most people (including myself) would say it is better to declare variables as they are needed, as so:

float centroidX = (xa + xb + xc)/3;
float centroidY = (ya + yb + yc)/3;

//instead of:

float centroidX;
float centroidY;
//... a whole bunch of code here ...
centroidX = (xa + xb + xc)/3;
centroidY = (ya + yb + yc)/3;

First, it has the advantage that you don't have to either plan ahead for all the local variables you are going to need in a function, or have to jump back and forth in the code to add variable declarations at the start as you discover that you need them, i.e. it is more effective. Second, since you declare variables just before you assign a value to them (or _as_ you are assigning a value to them), you reduce the "time" during which the variable exists but is not yet initialized, e.g. if you declare a variable at the start, you could mistakenly use it before you initialized it and have a silent bug, while declaring it as you are giving it a value, the compiler will give an error if you mistakenly try to use it before that. Finally, in theory, it will be more efficient (although in practice, it's a trivial matter for the compiler to optimize this).

2)
You asked about how to shorten the code. I would first warn you that shortening code is not always synonymous to improving readability (in fact, it often isn't), and it rarely gives much of an improvement in performance (remember that it is the compiler's job to translate C++ code to machine code, don't try to do too much of that yourself). But, otherwise, shortening code is often nice.

In this case, I'm guessing you might not have learned about arrays yet? If you did, then there is a simple way to make the code significantly smaller and also more practical. Consider this piece of the code (the input part):

float x[3], y[3];            //declare two static arrays for the coordinates of the points.
for(int i = 0; i < 3; ++i) { //loop through the three points to enter coordinates
  cout << "Enter point " << i << " on your triangle: " << endl; // prompt user to enter coordinates i.
  cin >> x[i] >> y[i];       // store numbers into variable x[i] and y[i].. i.e. the ith coordinate.
};
cout << endl;                //now that's it!

You could do the same for distances too.

3)
I'll take you early on this one. Don't use the pow() function lightly. For simple, small integer powers (like 2 or 3) there will be a significant performance difference between simply doing "x * x" versus "pow(x,2)". It is a simple thing to change, one is not less obvious than the other in terms of readability, and if it happens that you put this in the middle of a CPU intensive program or algorithm, multiplying the terms out will give a noticeable performance improvement.

....
there are many more things that can be improved.. but these are things you will learn later in your course / curriculum.

Bienvenu à Daniweb, et reviens nous voir!

[pseudorandom21]

It's also good to avoid magic numbers.

In mike_2000_17's code snippet magic numbers are used for the array size, and in the for loop.

magic number = unnamed numerical constant.

I've been turned down a job because I used a couple o' magic numbers.

Also in your code exist magic numbers.

[fabricetoussain]

thanks guys so far, taking up some good tips, any more critiques more than welcome.

and no i have not learned arrays yet.

[ravenous]

no i have not learned arrays yet.

If you haven't learned about arrays yet, then you probably haven't learned about structures either. But, "in for a penny, in for a pound" and all that.

So, I'd recommend using structures (or classes, but I wanted to at least try and keep it simple) to group your data in an obvious fashion. So, my version of your code is:

#include <iostream>
#include <cmath>

/* structure to store the (2D) coordinates of a point */
typedef struct{
    double x;
    double y;
}Point_2d;

/* Structure to store the vertices of a triangle */
typedef struct{
    Point_2d vertex[3];
}triangle;

/* Function prototypes */
void findCentroid(const triangle &inputTriangle, Point_2d &outputPoint);
double distance(const Point_2d &point1, const Point_2d &point2);

/*************** Main starts here **************/
int main (){
    /* Print a welcome message */
    std::cout << " Triangle Centroid Calculator!\n" << std::endl;

    /* Make a triangle structure and get the points of the vertices from the user */
    unsigned numberOfVertices = 3;
    triangle myTriangle;
    for(unsigned v = 0; v < numberOfVertices; ++v){
        std::cout << "Enter point " << v + 1 << " on your triangle: " << std::endl;
        std::cin >> myTriangle.vertex[v].x >> myTriangle.vertex[v].y;
    }

    /* Calculate & display the centroid */
    Point_2d centroid;
    findCentroid(myTriangle, centroid);
    std::cout << "The centroid is located at (" << centroid.x << ',' << centroid.y << ")\n" << std::endl;

    /* Calculate & display the distance from the centroid to the vertices */
    for(unsigned v = 0; v < numberOfVertices; ++v)
        std::cout << "Distance from centroid to point " << v << ": " << distance(centroid,myTriangle.vertex[v]) << std::endl;

    std::cout<< "\nThank you for your business." << std::endl;

    return 0;
}
/*********** end of main **************/

void findCentroid(const triangle &inputTriangle, Point_2d &outputPoint){
    outputPoint.x = (inputTriangle.vertex[0].x + inputTriangle.vertex[1].x + inputTriangle.vertex[2].x)/3;
    outputPoint.y = (inputTriangle.vertex[0].y + inputTriangle.vertex[1].y + inputTriangle.vertex[2].y)/3;
}

double distance(const Point_2d &point1, const Point_2d &point2){
    return sqrt((pow((point1.x - point2.x), 2) + pow((point1.y - point2.y), 2)));
}

So this code is no shorter than yours (well, by two or three lines but that's not really the point), but I hope it's a little clearer.

The code defines the concept of a point in 2D space using a structure to group two numbers that represent the two coordinates of the point. Then the concept of a triangle is defined as a collection of three points. So now the code knows what the properties of a triangle are (i.e. it is a collection of three points, each of which is two numbers). Obviously, you could just use an array of six numbers to do the same thing, but the structure has readability advantages. So:

triange myTriangle;
myTriange.vertex[1].y = 5.1;

versus

double myTriangle[6];
myTriangle[3] = 5.1;

As recommended above, I've also used functions to do certain tasks, like calculate the distance and find the centroid. These functions contain the exact same code that you had originally, but now it's hidden away in a function. This means that when you're reading the code you see a line like findCentroid(myTriangle, centroid); you can just think "OK, that bit calculates the centroid of myTriangle " and then move on. If, at some point, you care about how the centroid is calculated, you can go and look in that function. But, if you don't care, it doesn't get in the way.

You'll also notice that there are far fewer comments. This is because the extra organising power of structures (or classes) and functions means that much of the code is self-documenting. So you just need a kind of "section title" when you change from doing one thing (reading in the coordinates, say) to another (calculating the centroid). It's kind of obvious what's happening in each section, because of the names of things.

Obviously, if you haven't got to any of this stuff yet, then you can't have done it that way, but it's something to think about for the future.

Also, on a bigger project it might be more useful to describe very general concepts and have specialisations for particular things. For example, a triangle is a kind of shape, but so is a square. C++ has ways to describe these things using classes, but that's probably something for another day :)

[pseudorandom21]

This was an experiment for me, using enums to index arrays, and for the size:

#include <iostream>// for cout and cin
#include <cmath> // for sqrt () and pow (x,y)
 

using namespace std;

enum //<-- anonymous enum
{
  DISTANCE_ONE = 0,
  DISTANCE_TWO,
  DISTANCE_THREE,
  DISTANCE_BUFFER_SIZE
};

struct Triangle
{
	Triangle() : x(0.0f), y(0.0f) {}
	float x;
	float y;
};

enum
{
	TRIANGLE_ONE = 0,
	TRIANGLE_TWO,
	TRIANGLE_THREE,
	TRIANGLE_BUFFER_SIZE
};

//Prototypes
float ComputeDistance( const Triangle &centroid, const Triangle &point );
Triangle ComputeCentroid( const Triangle points[], const int size );
bool GetInput( Triangle points[], const int size );

int main () 
{
	cout << " Triangle Centroid Calculator!" << endl; // welcome user
	float distanceBuffer[DISTANCE_BUFFER_SIZE] = {0.0f};//  assign number of distance 1-3
	Triangle triangleBuffer[TRIANGLE_BUFFER_SIZE];// Points.
	Triangle centroid;

	while( !GetInput(triangleBuffer,TRIANGLE_BUFFER_SIZE) )
	{
		cin.sync();
		cin.clear();
	}
	
	centroid = ComputeCentroid(triangleBuffer, TRIANGLE_BUFFER_SIZE);

	cout << "The centroid is located at (" << centroid.x << ',' << centroid.y << ") " << endl; // distplay centroid coordinate to user
	cout << endl;
	
	distanceBuffer[DISTANCE_ONE] = ComputeDistance(centroid,triangleBuffer[TRIANGLE_ONE]);
	distanceBuffer[DISTANCE_TWO] = ComputeDistance(centroid,triangleBuffer[TRIANGLE_TWO]);
	distanceBuffer[DISTANCE_THREE] = ComputeDistance(centroid,triangleBuffer[TRIANGLE_THREE]);

	
	//Print results
	for(int i=0; i < DISTANCE_BUFFER_SIZE; i++)
	{
		cout << "Distance from centroid to point " << i << ": " << distanceBuffer[i] << endl; // prompt user of the distance1
	}
	cout << endl;
	cout<< "Thank you for your business." << endl;
	
	cout << "Press ENTER to exit." << endl;
	cin.sync();
	cin.clear();
	cin.get();
    return 0;
}

bool GetInput( Triangle points[], const int size )
{
	cout << endl;
	for(int i = 0; i < size; i++)
	{
		cout << "Enter point " << i << " on your triangle: " << endl; // prompt user to enter two numbers
		cin >> (points[i].x) >> (points[i].y); // store numbers into variable x and y
	}
	return cin.good();
}

Triangle ComputeCentroid( const Triangle points[], const int size )
{
	Triangle xy;
	for(int i = 0; i < size; i++)
	{
		xy.x += points[i].x;
		xy.y += points[i].y;
	}
	xy.x /= size;
	xy.y /= size;
	return xy;
}

float ComputeDistance( const Triangle &centroid, const Triangle &point )
{
	return sqrt((pow((centroid.x - point.x), 2) + pow((centroid.y - point.y), 2))); // distance from point to centroid
}

It's certainly not the prettiest out there, but with a namespace or a class it wouldn't pollute the global namespace with my enums at least.

[mike_2000_17]

@pseudorandom:
I don't want to be mean, but I think your implementation is probably worse then the OP's or ravenous' implementation.

You correctly pointed out that using magic numbers is not nice, but you are using them. Replacing magic numbers, like 1 2 3, by anonymous enums with names like ONE TWO THREE, is merely a change from typing numbers to typing letters, but they are magic numbers nonetheless. And it is even worse in terms of additional typing (not to mention that they look disgusting in the code... at first glance, any programmer would look at this code and say: "oh, another piece of s**t code with #defines everywhere"). There is no difference between anonymous enums and #defines when used that way. And there is no difference between #defines and literal magic numbers when the values of them cannot be changed (a define with name ONE has to always be one, otherwise it would be confusing, and thus, mind as well use the number 1 instead). In this case, a number like 3 for the number of points in a triangle isn't really a magic number in the typical evil sense. A triangle has three points, that's the definition of a triangle, there is nothing wrong with using a magic numbers like 3 or calculating stuff without a loop, when there is no other possible number of points in a triangle (it's not like you would have to come back later and change that number!).

I just wanted to make sure that the OP knew that your "experiment" was not the kind of code you ought to consider "good".

"Good" code would be more along the lines of:

#include <iostream>
#include <cmath>

using namespace std;

struct Point2D {
  double x,y;
  Point2D(float aX = 0.0, float aY = 0.0) : x(aX), y(aY) { };
  //Creates a 2D point and prompts the user for the point with name aPromptName.
  explicit Point2D(const string& aPromptName) {
    cout << "Enter coordinates of " << aPromptName << ": ";
    cin >> x >> y;
  };
  //Vector addition operation.
  Point2D operator+(const Point2D& p) const { return Point2D(x + p.x,y + p.y); };
  //Vector subtraction operation.
  Point2D operator-(const Point2D& p) const { return Point2D(x - p.x,y - p.y); };
  //Scalar multiplication.
  Point2D operator*(const double& s) const { return Point2D(s*x,s*y); };
  //Scalar division.
  Point2D operator/(const double& s) const { return Point2D(x/s,y/s); };
};

//Computes the magnitude of a vector.
double magnitude(const Point2D& p) { return sqrt(p.x*p.x + p.y*p.y); };

//Prints out a point's coordinates to an ostream.
ostream& operator <<(ostream& out, const Point2D& p) {
  out << "(" << p.x << ", " << p.y << ")";
  return out;
};

//Defines a triangle, i.e. three points.
struct Triangle {
  Point2D pts[3];   //holds the points.
  Point2D centroid; //holds the centroid value.
  //Create a triangle, prompting the user for three points and store the centroid.
  Triangle() {
    pts[0] = Point2D("Point One");
    pts[1] = Point2D("Point Two"); 
    pts[2] = Point2D("Point Three"); 
    centroid = (pts[0] + pts[1] + pts[2]) / 3;
    cout << "The centroid was found at: " << centroid << endl;
  };
};

int main() {
  cout << " Triangle Centroid Calculator!" << endl;
  Triangle triangle;
  for(int i=0;i<3;++i)
    cout << "The distance of point " << i+1 << " to the centroid is " 
         << magnitude(triangle.pts[i] - triangle.centroid) << endl;
  return 0;
};

Fabrice, don't let this scare you, the above is just a taste of what typical C++ implementations for this problem would look like if a "real" programmer did it. You are just starting your journey, take it one step at a time. You will also notice, as pointed out by ravenous, that there isn't really any use for the comments in the above code (except for the constructor that prompts the user), because the functions are clear (an addition is an addition, a multiplication is a multiplication, etc.). This is why I originally mentioned that comments are good for the professor correcting assignments from inexperienced students, but actual programmers rarely use them much, that's because programmers usually put a great deal of effort making sure the code is self-explanatory. It's still a good thing to put comments, if you don't abuse it, but clear code is even better.

[pseudorandom21]

I do agree it turned out to be a piece of crap. I suppose I should give it another whirl, after all I'm here to learn.

So, I forgot OP doesn't even know how to use arrays yet, but here's a pretty one (in my opinion):

#include <iostream>
#include <cmath>

struct Point
{
	Point() : x(0.0f), y(0.0f) {}
	Point(float tx, float ty) : x(tx), y(ty) {}
	Point& operator+=(const Point &rhs)
	{
		this->x+=rhs.x;
		this->y+=rhs.y;
		return *this;
	}
	Point& operator/=(const int &rhs)
	{
		this->x /= rhs;
		this->y /= rhs;
		return *this;
	}
	const Point operator-(const Point &rhs) const
	{
		return Point(rhs.x-this->x, rhs.y-this->y);
	}
	friend std::ostream& operator<<(std::ostream &out, const Point &rhs)
	{
		out << "(" << rhs.x << "," << rhs.y << ")";
		return out;
	}
	//data members
	float x;
	float y;
};

struct Triangle
{
	static const int NUM_TRIANGLE_POINTS = 3;
	Point vertices[NUM_TRIANGLE_POINTS];
};

Point CalculateCentroid(/* IN */ Triangle &triangle_in)
{
	Point r;
	for(int i = 0; i < triangle_in.NUM_TRIANGLE_POINTS; i++)
	{
		r+=triangle_in.vertices[i];
	}
	return (r /= triangle_in.NUM_TRIANGLE_POINTS);
}

float CalculateDistance(const Point &centroid, const Point &fromPoint)
{
	Point t = fromPoint - centroid;
	return t.x * t.x + t.y * t.y;
}

bool GetInput(/* IN */ Triangle &triangle_in)
{
	for(int i = 0; i < triangle_in.NUM_TRIANGLE_POINTS; i++)
	{
		std::cout << "Enter triangle point " << i+1 << " as a float" << std::endl;
		std::cout << "In the form: x y" << std::endl;
		std::cin >> triangle_in.vertices[i].x >> triangle_in.vertices[i].y;
		if( !std::cin.good() )
		{
			std::cout << "Invalid entry; retry." << std::endl;
			std::cin.sync();
			std::cin.clear();
			i--;
			continue;
		}
		else if( std::cin.bad() )
		{
			std::cout << "Stream is totally unusable." << std::endl;
			return false;
		}
	}
	return true;
}

int main()
{
	std::cout << "Triangle Centroid Calc" << std::endl;
	Triangle data;
	if( !GetInput(data) )
		return 1;//failbit
	Point centroid(CalculateCentroid(data));
	std::cout << std::endl << "Centroid = " << centroid << std::endl;
	for(int i = 0; i < data.NUM_TRIANGLE_POINTS; i++)
	{
		std::cout << "Distance from point " << data.vertices[i] << " to centroid at " << centroid << 
			" is: " << CalculateDistance(centroid,data.vertices[i]) << std::endl;
	}
	std::cin.ignore();
	std::cin.get();
}

[pseudorandom21]

Oh, and for mike_2000_17's implementation, the values returned by your operator-/+ overload would usually be constant, to avoid weird crap like this:

((obj - obj2) = obj3);

and my Triangle needs a copy constructor to do a deep copy because of the array.

[mike_2000_17]

>>and my Triangle needs a copy constructor to do a deep copy because of the array.
The triangle class does not need a copy-constructor, the default one is fine. The array contained in Triangle is a static array, it will be copied correctly by the compiler-generated default copy-constructor.

[pseudorandom21]

Nice to know, see I learned something. :D

Typically I will exploit every useful part of the standard library to make the application more robust. Note being restricted to <iostream> and <cmath>.