I’ve always been in the “More comments are better” camp… but in my new employment, on a large team of coders working together, I have discovered that this point of view is considered (at least here) to be old-school.
It seems that the new style is “as few comments as possible” “Especially dont leave commented-out old code in your files when you check them in/commit them to the build”
for example:
// see ISSUE00105797 .. calculation changed to centre the cardinal direction
// out->gpsData.data_XDataFormat1.xDataFormat1_HDG =
// in->gpsData.course.value / X_DATA_FORMAT1_HEADING_CONV_FACTOR;
// new calculation:
out->gpsData.data_XDataFormat1.xDataFormat1_HDG =
((in->gpsData.course.value + (X_DATA_FORMAT1_HEADING_CONV_FACTOR/2))%360) / X_DATA_FORMAT1_HEADING_CONV_FACTOR;
When I first heard this, I was very surprised, as I’ve always left small bug fixes with old code commented out (emphasis on small) in the code, and generally included as many comments as possible.
I have been told “Dude, comments are bad. People forget to update them. You have to refactor everything to make sure the code is completely understandable without comments”. When I pushed back, saying “Comments are helpful” I sparked a tangible anger from several colleagues who insisted comments were the source of terrible problems and only used by terrible coders.
This seems a little unrealistic to me. Am I just old fashioned now?
I understand the use of version history tools to see previous versions (we use Mercurial) but still… Are extensive comments now a “no-no” in large team continuous-integration environments? Are they now seen as flaws/weaknesses?
Note that Im not talking about leaving in test-code or temporary debug code commented out, as in this thread:
I totally agree with your colleagues. “Green code” and out-of-date comments are worse than no documentation at all. Refactor your code right and you won’t need either of them…
I would never leave the old, buggy code in place. There are a few places where I leave significant amounts of commented-out code (like testing projects) but it’s probably not the best way to handle reference code.
Also, referencing issues in your comments is a smell, I’d avoid it. Reference issues in your commit messages instead.
If you have no other reference location available, comments in code and leaving block-commented-out old code in-situ is OK.
If you have other reference location available, the comments and content is best stored in those, not the source code.
Yup. Lots of comments were for a time when IDEs and languages were not as verbose and intelligent as they are now. Less was considered more. The modern mantra is that code should be verbose and the IDE should be able to fill in most of the gaps that comments would have traditionally been used for.
I guess that I am old school as well, since I include comments where I think they are necessary. However, I was also taught that code should be as self-documenting as possible. Meaning things like:
[ul]Organize your solution into sub-projects, and only expose what’s necessary from each project.
Organize complex methods into layers using subroutines.
Use variable and method names that are meaningful (don’t be afraid to use long names).
Don’t be sloppy, or try to make your code too compact.
Use white space and blank lines where appropriate (white space is not your enemy).
Use indentation appropriately, and consistently.
Use language features appropriately: Access modifiers (public, internal, protected, private), Other modifiers (readonly, const).[/ul]
If you have a source code management tool like TFS, Git, … old commented out code does not help anyone.
If you want to see the old code, you run a compare/history op on the file.
Also:
If you have code that explains itself, then this is better than the best comment you can write.
In contrary, if you had to make some more strange code, like to work around a bug in 3rd party code, or to avoid some side effects hard to see on the code, a short comment is really helpful.
I’m also in the comment-minimalism camp. I do believe in using intellisense comments for public APIs; comments that document pre/post-conditions and invariants; and comments that reference an algorithmic approach that may not be evident from the code itself (e.g., “Use a sieve of Eratosthenes to find the next prime number”. Basically, anything that you cannot easily glean from the source. But comments that explain syntax are wasted effort and can lead to “debugging the comments”. And source management obviates the need for commented out code, though I have to admit that I do still do this myself during development, and then sometimes don’t clean it up before checkin.
Worst offender : the automated comments that VS adds in via it’s templates, which are universally awful (many have been removed in the VS2013 and VS2015).
Leaving old code commented out is a cardinal sin! Code should be readable without the comments however: comments are very necessary for explaining what the code does.
Take for example, the code snippet below. The functions GetAngle and CalculateSweep. These are implementations of geometric mathematical functions that may not be common knowledge to the developers. The only way to explain this is to write it in the comments.
/// <summary>
/// Generates the points along an arc including the start and end points.
/// </summary>
/// <param name="start">absolute start coordinate</param>
/// <param name="end">absolute end coordinate</param>
/// <param name="center">absolute center coordinate</param>
/// <param name="isClockwise">true to plot clockwise, false to plot counter clockwise</param>
/// <param name="radius">radius of the circle</param>
/// <param name="minArcLength">if this arc doesn't meet the minimum threshold, don't expand.</param>
/// <param name="arcSegmentLength">length of each arc segment</param>
/// <returns>List of plottable points on the arc</returns>
private List<Point> PlotArc(Point start, Point end, Point center, bool isClockwise, double radius, double minArcLength, double arcSegmentLength) {
// Calculate radius if necessary.
if (radius == 0) {
radius = Math.Sqrt(Math.Pow(start.X - center.X, 2.0) + Math.Pow(end.Y - center.Y, 2.0));
}
double startAngle = GetAngle(center, start);
double endAngle = GetAngle(center, end);
double sweep = CalculateSweep(startAngle, endAngle, isClockwise);
// Convert units.
double arcLength = sweep * radius;
// If this arc doesn't meet the minimum threshold, don't expand.
if (minArcLength > 0 && arcLength < minArcLength) {
return null;
}
int numPoints = 20;
if (arcSegmentLength <= 0 && minArcLength > 0) {
arcSegmentLength = (sweep * radius) / minArcLength;
}
if (arcSegmentLength > 0) {
numPoints = (int)Math.Ceiling(arcLength / arcSegmentLength);
}
List<Point> segments = new List<Point>();
segments.Add(start);
double angle;
// Calculate radius if necessary.
if (radius == 0) {
radius = Math.Sqrt(Math.Pow(start.X - center.X, 2.0) + Math.Pow(start.Y - center.Y, 2.0));
}
for (int i = 0; i < numPoints; i++) {
if (isClockwise) {
angle = (startAngle - i * sweep / numPoints);
}
else {
angle = (startAngle + i * sweep / numPoints);
}
if (angle >= Math.PI * 2) {
angle = angle - Math.PI * 2;
}
segments.Add(new Point(Math.Cos(angle) * radius + center.X,
Math.Sin(angle) * radius + center.Y));
}
segments.Add(end);
return segments;
}
/// <summary>
/// Return the angle in radians when going from start to end.
/// </summary>
/// <param name="start"></param>
/// <param name="end"></param>
/// <returns></returns>
private double GetAngle(Point start, Point end) {
double deltaX = end.X - start.X;
double deltaY = end.Y - start.Y;
double angle = 0.0;
if (deltaX != 0) { // prevent div by 0
// it helps to know what quadrant you are in
if (deltaX > 0 && deltaY >= 0) { // 0 - 90
angle = Math.Atan(deltaY / deltaX);
}
else if (deltaX < 0 && deltaY >= 0) { // 90 to 180
angle = Math.PI - Math.Abs(Math.Atan(deltaY / deltaX));
}
else if (deltaX < 0 && deltaY < 0) { // 180 - 270
angle = Math.PI + Math.Abs(Math.Atan(deltaY / deltaX));
}
else if (deltaX > 0 && deltaY < 0) { // 270 - 360
angle = Math.PI * 2 - Math.Abs(Math.Atan(deltaY / deltaX));
}
}
else {
// 90 deg
if (deltaY > 0) {
angle = Math.PI / 2.0;
}
// 270 deg
else {
angle = Math.PI * 3.0 / 2.0;
}
}
return angle;
}
private double CalculateSweep(double startAngle, double endAngle, bool isCw) {
double sweep;
// Full circle
if (startAngle == endAngle) {
sweep = (Math.PI * 2);
// Arcs
}
else {
// Account for full circles and end angles of 0/360
if (endAngle == 0) {
endAngle = Math.PI * 2;
}
// Calculate distance along arc.
if (!isCw && endAngle < startAngle) {
sweep = ((Math.PI * 2 - startAngle) + endAngle);
}
else if (isCw && endAngle > startAngle) {
sweep = ((Math.PI * 2 - endAngle) + startAngle);
}
else {
sweep = Math.Abs(endAngle - startAngle);
}
}
return sweep;
}
@ Mr. John Smith - [em] comments are very necessary for explaining what the code does.[/em]
From a poll of colleagues and former colleagues, it seems that that is an old, outdated habit of poor programmers.
Apparently, the new ideal is “the code needs to be self explanatory; if it isn’t, then refactor it so that it is. Comments at best introduce risk that they will become outdated, and at worst can end up misleading, wasting time, and introducing more work to maintain the comments - so they are to be avoided everywhere, except extremely rare cases, such as documenting API usage, and very obscure high level information that cannot be shown by refactored code (like the “Use a sieve of Eratosthenes to find the next prime number” example)”
In your example, the only valid comments would be the API description to be picked up by an external documenting tool.
Comments like “// Convert units.” and “// Calculate radius if necessary.” are exactly the kind of comments I would normally like, but are specifically the type of comments that everyone here says 'No no no NO! Change the variable names to be clearer… that is BAD and it SMELLS and you are a bad programmer if you use those!"
It really is a paradigm shift that caught me off guard. But I taught myself to program at 13 in 1980 - I didnt take any college courses. It’s all bootstrappy stuff. So I missed some of these things. I even came to the idea of standard “patterns” late, after a few decades of programming already. I knew most of the patterns instinctively, but they were just common sense things I recognized by experience, without unique names. So, I don’t speak the same language or have the same programming culture as my colleagues. It takes a little longer for them to realize how awesome I am. I really suck at interviews. But once I get past one, and past the “this guy doesnt talk like us”, I do well.
But stuff like this still pops up and surprises me.
BTW, this only applies to shared code worked on by many people and intended to be around for along time and continually worked on. If its just for one person, go to town and fill up everything with comments/notes to your heart’s content.
Comments should explain WHY you’re doing something, not what you’re doing. If you have to explain that you’re calculating the radius, then you probably could use better variable names, etc. If you’re working on a complex or obscure algorithm, then it makes sense to explain WHY you’re calculating the radius, so the next guy doesn’t come along and remove it, because he/she doesn’t understand the algorithm.
Another thing that I learned is that properly documenting software costs as much as writing the software itself. So if developers are taking time to write and maintain comments everywhere, its actually costing more to develop the software.
// Convert radius value to proper units.
if (gc_block.modal.units == UNITS_MODE_INCHES) { gc_block.values.r *= MM_PER_INCH; }
/* We need to calculate the center of the circle that has the designated radius and passes
through both the current position and the target position. This method calculates the following
set of equations where [x,y] is the vector from current to target position, d == magnitude of
that vector, h == hypotenuse of the triangle formed by the radius of the circle, the distance to
the center of the travel vector. A vector perpendicular to the travel vector [-y,x] is scaled to the
length of h [-y/d*h, x/d*h] and added to the center of the travel vector [x/2,y/2] to form the new point
[i,j] at [x/2-y/d*h, y/2+x/d*h] which will be the center of our arc.
d^2 == x^2 + y^2
h^2 == r^2 - (d/2)^2
i == x/2 - y/d*h
j == y/2 + x/d*h
O <- [i,j]
- |
r - |
- |
- | h
- |
[0,0] -> C -----------------+--------------- T <- [x,y]
| <------ d/2 ---->|
C - Current position
T - Target position
O - center of circle that pass through both C and T
d - distance from C to T
r - designated radius
h - distance from center of CT to O
Expanding the equations:
d -> sqrt(x^2 + y^2)
h -> sqrt(4 * r^2 - x^2 - y^2)/2
i -> (x - (y * sqrt(4 * r^2 - x^2 - y^2)) / sqrt(x^2 + y^2)) / 2
j -> (y + (x * sqrt(4 * r^2 - x^2 - y^2)) / sqrt(x^2 + y^2)) / 2
Which can be written:
i -> (x - (y * sqrt(4 * r^2 - x^2 - y^2))/sqrt(x^2 + y^2))/2
j -> (y + (x * sqrt(4 * r^2 - x^2 - y^2))/sqrt(x^2 + y^2))/2
Which we for size and speed reasons optimize to:
h_x2_div_d = sqrt(4 * r^2 - x^2 - y^2)/sqrt(x^2 + y^2)
i = (x - (y * h_x2_div_d))/2
j = (y + (x * h_x2_div_d))/2
*/
// First, use h_x2_div_d to compute 4*h^2 to check if it is negative or r is smaller
// than d. If so, the sqrt of a negative number is complex and error out.
float h_x2_div_d = 4.0 * gc_block.values.r*gc_block.values.r - x*x - y*y;
if (h_x2_div_d < 0) { FAIL(STATUS_GCODE_ARC_RADIUS_ERROR); } // [Arc radius error]
