The attached program performs a QuickSort in C++ which sorts an array of structs based on their data members. Basically, it sorts an array of coordinates based on their distance from the origin.

Nice Inline Commenting! Bravo on Code Format as well.

:-)

Thank you ... I have a very definitive coding style.

I would argue that your code suffers from excessive commenting, which is as bad as under-commenting. I also have a personal preference for avoiding end-of-line comments in all but the simplest of cases, but that's personal preference.

Let's look at some examples:

> int n; // number of points in the array

OK, so here you've declared an integer variable and given it the identifier 'n'. Because 'n' is devoid of meaning you have a comment alongside the declaration so that the reader knows what 'n' is for. So now what do you do? Do you comment the use of 'n' every time it's used in your program? Or do you expect the reader to remember, or to search the code for the declaration and comment? ...

> ...
> cin >> n;
> ...
> Q = new Point[n]; // allocate space for Q of n Points

Hmmm, how about calling it something other than 'n', for instance, let's call it 'numberOfPoints'. Now lets' look at the resulting changes to the code:

int numberOfPoints;
...
cin >> numberOfPoints;
...
Q = new Point[numberOfPoints];

Notice how the identifier conveys meaning, and you no longer need to comment the declaration or use of the variable.

Single character identifiers should generally be reduced to use only for loop indexes and the like, and even then there is often value in using a meaningful name.

There are various other examples where you need to let the code do the talking, and cases where the code already does but you use a comment in addition to the code, which is quite unecessary.


Another point, there are several ways to handle accessors (getters and setters) and this is one:

> int x(); // returns the x-coordinate
> void x(int); // sets the x-coordinate to the input value

but using the same overloaded function name for getting and setting can lead to confusion. Probably simpler and clearer is:

int getx();
void setx(int);

The comments are no longer necessary, and reading the code where the functions are called is easier on the reader.

>return 0; // successful completion

Why comment it? Returning zero means successful termination. If you really want to be clear you can always return EXIT_SUCCESS from <cstdlib> instead but return 0 should be enough.

You're still using the outdated C++ headers, which is a shame.

Also, C++ comes with a swap function so you don't need to roll your own.

> if (again == 'Y') main();

This is a real no-no. In C++ it's illegal to call main(). Don't do it. There are other ways to re-run the code inside main.


> int n;
> cin >> n;
> P = new Point[n];

Sooner or later someone using your program will enter a letter or something else other than a number, and then it will probably crash the program, because cin goes into a failed state. You should code against this.

There are other issues, but that's probably enough
HTH

Ahh! You sure do know how to break a gal down, eh? (just kidding)

Okay okay where shall I start. First, this program was written my third semester at Hofstra for a data structures and algorithms course. (It was a theoretical course; the C++ was simply an exercise). It was a homework assignment meant to demonstrate my knowledge of the QuickSort. It was basically a "sort coordinates via a quicksort, exercise for a weekend" given Friday due Monday type deal. Now that I actually think back to it, I think we had a week for it.

As for over-commenting, I know I did Basically I was taught right off the bat that over-commenting is great and deserves extra commendation for effort. In retrospect, my freshman professors might have told me so simply because it's easier for them to grade algorithms instead of teaching the best coding style. Of course, commenting is also a great way to learn computer programming. My current commenting style includes a detailed (probably too detailed) pre and a post above all functions/methods and very little else. (I'll use a pre and post even for one line functions that are plainly obvious, if for nothing else than consistancy for all functions). Alternatively, I'll comment function prototypes in my header files.

As for using better variable/function names ... I definitely see the point of this. I try to do it as much as I can (now) but I'm often guilty, unfortunately. I especially appreciate setx and getx as I have recently dealt with set and get blocks in C#.

To quote you ... "that's probably enough" ... my hand is getting tired of typing

- Dani

P.S. Thanks for the post! No, really, thanks

Its good to over-comment when explaining to noobs.
But, in a firm, when you work, the people there won't pay/reward you more for over-commenting. All they are interested in is work (the code) and has comments just enough for a programmer to be able to understand what it does.
Personally I avoid commenting as until essential (may use variable/function names upto 20 characters).
Writing the code with those small variables only, then do a replace all
my var names also tend to be long since I don't use Hungarian notation (M$ one), I use the old one.
I use this_is_my_string instead of sThisIsMyString

Its been over one year since I touched C++. Just took a 3 months gap from it after 6 years, but got extended to 1 yr. Will only be able to resume in Jan04.

All meant as constructive comment of course - I'm sure you know me well enough by now to know that

Yes, definitely That's why I threw the j/k in there!

Some of Bob's points are valid, but I'd like to sugest alternate comments.....

First, I like end-of-line comments that talk about THAT line. And I like separate-line comments that talk about the strategy of the next several lines. Like:

// Get the two numbers and sort them into order
-vs-
if (n % 2) // is n odd?

I agree with Bob on the variable names, depending on the SCOPE of the name. If I'm in a small loop, who cares if I use 'i' rather than 'loopIndex'?

for (int i = 0; i < 10; i++)

(note the time-honored tradition of FORTRAN use of i,j,k as integers!)

But, overall, if I could offer one piece of advice from 25 years of making a living writing code, and I know this is generally the OPPOSITE what we are taught in CS....

You need to understand your own code. The person who comes along a year from now doesn't matter. You have no control over that person, only over your own work. The quality of your work is based on your understanding of it, not on someone else's.

You will be judged based on the finished product, not on the variable names or brace style or the editor you used or whether you use overloaded operators or not.

Further, in all companies I know of, the SHIP DATE is the master. Not quality, not features. If you miss the ship date, don't tell your boss "But, we were delayed because we are staying away from multiple inheritence, just like the guidelines said!" Nor, "rather than coding an array we spent lots of time coding circular lists."

ok, so the bottom line for me, then, is:

CSCGal, were you happy with this work at the time? Were the consumers of this code happy?