Pages: 1 2 »
  Print  
Author Topic: How not to comment code.  (Read 5874 times)
Offline (Unknown gender) score_under
Posted on: February 10, 2009, 01:30:41 PM

Member
Joined: Aug 2008
Posts: 308

View Profile
Coollog is a repeat offender of this one.
Look at one of his scripts, read the code on each line, then tell me if you are at all enlightened by the comments:

Code: [Select]
var _d,_i,_j,_c;//Dims the temporary variables for the script.

_d=0;//Sets _d to 0.
for(_i=0;_i<=string_length(argument0);_i+=1)//Loops for starting with _i=0, while _i is still smaller than or equal to the length of the first inputed string, add one to _i and execute the bracketed code.
_d[_i,0]=_i;//Sets the cell at row _i, column 0 to _i.
for(_j=0;_j<=string_length(argument1);_j+=1)//Loops for starting with _j=0, while _j is still smaller than or equal to the length of the second inputed string, add one to _j and execute the bracketed code.
_d[0,_j]=_j;//Sets the cell at row 0, column _j to _j.

for(_i=1;_i<=string_length(argument0);_i+=1)//Loops for starting with _i=1, while _i is still smaller than or equal to the length of the first inputed string, add one to _i and execute the bracketed code.
for(_j=1;_j<=string_length(argument1);_j+=1)//Loops for starting with _j=1, while _j is still smaller than or equal to the length of the second inputed string, add one to _j and execute the bracketed code.
{
  if string_char_at(argument0,_i) = string_char_at(argument1,_j)//If the character in the first inputed string at position _i is equal to the same position in the second inputed string...
   _c=0;//Sets _c to 0.
  else//Otherwise...
   _c=1;//Sets _c to 1.
  _d[_i,_j]=min(_d[_i-1,_j]+1,_d[_i,_j-1]+1,_d[_i-1,_j-1]+_c);//Sets the cell at row _i, column _j to the minimum of the above value plus one, the left value plus one, or the top-left value plus _c.
}

return _d[string_length(argument0),string_length(argument1)];//Returns the difference between the first inputed string and the second inputed string.
The whole point of comments is to help people decipher the code, not to say what each individual command does, but its role in the big picture of the script.
Logged
Offline (Male) sprintf()
Reply #1 Posted on: February 10, 2009, 10:45:02 PM

"Past Contributor"
Location: S. Wales
Joined: Apr 2008
Posts: 72
MSN Messenger - dmgoron@gmail.com
View Profile Email
http://drzaius.ics.uci.edu/blogs/setbang/raiders_of_the_lost_ark_2.gif

Edit by a2h: Image URL'ed. You broke the max width and max filesize rules. No punishment... for now.
« Last Edit: February 11, 2009, 03:31:41 AM by a2h » Logged
Offline (Male) notachair
Reply #2 Posted on: February 11, 2009, 02:19:09 AM

Definitely not a chair
Contributor
Joined: Feb 2008
Posts: 299

View Profile
I once read somewhere that good commenting practice means to remove all your code and still be able to get a basic understanding of what your program does.

Though I do agree that is a bit overkill.
Logged
Offline (Male) RetroX
Reply #3 Posted on: February 11, 2009, 04:35:41 PM

Master of all things Linux
Contributor
Location: US
Joined: Apr 2008
Posts: 1055
MSN Messenger - classixretrox@gmail.com
View Profile Email
Better than squishcode.
Logged
My Box: Phenom II 3.4GHz X4 | ASUS ATI RadeonHD 5770, 1GB GDDR5 RAM | 1x4GB DDR3 SRAM | Arch Linux, x86_64 (Cube) / Windows 7 x64 (Blob)
Quote from: Fede-lasse
Why do all the pro-Microsoft people have troll avatars? :(
Offline (Male) Rusky
Reply #4 Posted on: February 11, 2009, 06:59:43 PM

Resident Troll
Joined: Feb 2008
Posts: 960
MSN Messenger - rpjohnst@gmail.com
View Profile WWW Email
yeesh, those kinds of comments are why people are supposed to use descriptive names. real comments describe blocks of code. the only time you should comment every single line is assembly.
« Last Edit: February 12, 2009, 06:46:33 PM by Rusky » Logged
Offline (Male) notachair
Reply #5 Posted on: February 12, 2009, 12:51:19 AM

Definitely not a chair
Contributor
Joined: Feb 2008
Posts: 299

View Profile
yeesh, those kinds of comments are why people are supposed to use descriptive names. real comments describe blocks of code. they only time you should comment every single line is assembly.
Oh, so that's how you keep your brain from exploding when looking at ASM!
Logged
Offline (Unknown gender) score_under
Reply #6 Posted on: February 12, 2009, 03:09:25 PM

Member
Joined: Aug 2008
Posts: 308

View Profile
yeesh, those kinds of comments are why people are supposed to use descriptive names. real comments describe blocks of code. they only time you should comment every single line is assembly.
No, Assembly has such subtle commands that it's best to comment every tenth line, or less.
It can get pretty useless, especially in badly optimized code, for example:
Code: [Select]
xor eax,eax
dec eax
movzx eax,al
xchg al,ah
dec eax
push ax
call SomeFunction ; SomeFunction(-256)

A2h, I see you do not share my passion for debugging.
Logged
Offline (Male) Josh @ Dreamland
Reply #7 Posted on: February 12, 2009, 06:02:22 PM

Prince of all Goldfish
Developer
Location: Pittsburgh, PA, USA
Joined: Feb 2008
Posts: 2958

View Profile Email
'Tis a rarity, methinks.
Logged
"That is the single most cryptic piece of code I have ever seen." -Master PobbleWobble
"I disapprove of what you say, but I will defend to the death your right to say it." -Evelyn Beatrice Hall, Friends of Voltaire
Offline (Male) Rusky
Reply #8 Posted on: February 12, 2009, 06:55:09 PM

Resident Troll
Joined: Feb 2008
Posts: 960
MSN Messenger - rpjohnst@gmail.com
View Profile WWW Email
I wouldn't tell someone to comment every line of assembly with something like "move this number into this register" or "call this function," that'd be stupid I agree. It's more to keep a running commentary next to everything you're doing, something like:
Code: [Select]
; Function to add the first two parameters
add: push ebp ; create stack frame
mov ebp, esp
mov eax, [ebp + 8] ; get the first argument
mov ecx, [ebp + 12] ; second argument
add eax, ecx ; sum in eax to return
pop ebp ; restore stack frame
ret
Brains are not CPUs. They need help, no matter how good at debugging you think you are.
Logged
Post made February 13, 2009, 03:37:28 AM was deleted at the author's request.
Offline (Unknown gender) score_under
Reply #10 Posted on: February 13, 2009, 01:47:22 PM

Member
Joined: Aug 2008
Posts: 308

View Profile
I wouldn't tell someone to comment every line of assembly with something like "move this number into this register" or "call this function," that'd be stupid I agree. It's more to keep a running commentary next to everything you're doing, something like:
Code: [Select]
; Function to add the first two parameters
add: push ebp ; create stack frame
mov ebp, esp
mov eax, [ebp + 8] ; get the first argument
mov ecx, [ebp + 12] ; second argument
add eax, ecx ; sum in eax to return
pop ebp ; restore stack frame
ret
Brains are not CPUs. They need help, no matter how good at debugging you think you are.
When I'm reverse-engineering stuff, I usually comment like this:
(Using same code)
Code: [Select]
add: push ebp ; add(arg1,arg2)
mov ebp, esp ; {
mov eax, [ebp + 8] ;  
mov ecx, [ebp + 12] ;   return arg1+arg2;
add eax, ecx ;  
pop ebp ; }
ret
The comments aren't well placed (the "ret" is on the last line, yet I've put the "return" in the middle of the function, in the middle of the 3 calculation instructions - it'll still execute code similar to that comment). I only try to make sure that the code is readable for me at 04:00 AM.

Also, Josh, for support of large numbers in your modulus function, this might work (instead of a-=(int)a;)...
Code: [Select]
double temp;
asm("FLD %%st\n\
     FISTPQ %2\n\
     FILDQ %2\n\
     FSUBRP %%st,%%st(1)\n"
     :"=&t"(x)
     :"f"(x),"m"(temp));
For the commenting bit:
Code: [Select]
fld st
fistp qword [temp] ; temp = (QWORD)floor(x);
fild qword [temp]
fsubp st, st1      ; x -= temp;

If you want to be more standards-compliant, you could always do "x-=(long long signed int)x;", but that will produce around 3x more useless opcodes.
« Last Edit: February 13, 2009, 03:16:37 PM by score_under » Logged
Offline (Unknown gender) ludamad
Reply #11 Posted on: February 19, 2009, 06:26:30 PM
Developer
Joined: May 2008
Posts: 1256

View Profile Email
Code: [Select]
variable = 3/*sets a variable with name "variable" equal to three*/ //comments the code to explain to the user that "variable = 3" sets a variable with name "variable" equal to three
Logged
Post made February 20, 2009, 09:37:23 AM was deleted at the author's request.
Offline (Unknown gender) score_under
Reply #13 Posted on: February 20, 2009, 12:20:13 PM

Member
Joined: Aug 2008
Posts: 308

View Profile
Well, you were half-right...
Code: [Select]
variable = 3; //Set maximum iterations for short pathfinding algorithm to 3.
Logged
Offline (Male) sprintf()
Reply #14 Posted on: February 20, 2009, 03:30:38 PM

"Past Contributor"
Location: S. Wales
Joined: Apr 2008
Posts: 72
MSN Messenger - dmgoron@gmail.com
View Profile Email
Code: [Select]
maxIterations = 3
Logged
Pages: 1 2 »
  Print