Page 2 of 3

Posted: Tue May 20, 2003 10:49 am
by jason
[]InTeR[]: No, that should be handled by your versioning software (CVS). In fact, that's part of the reason to use it, and it's better then relying on programmers to remember to mark what they changed, and when they changed it, especially if you are using freelancers.

Posted: Tue May 20, 2003 10:03 pm
by zoki
InTeR, yep, you can do like that, but CVS is better.

Taking some time to learn how to use CVS is highly recommended.

OK ...

Posted: Tue May 20, 2003 10:28 pm
by netcrash
I'm sorry but also the kind of comments that you have shown are not in anyway good.
Yes I know that you know that .

But i'll give you a good example or i'll die trying.

When I started programing in PHP i made a program for a company this program
was a mess when i finished it . I didn't know mutch about classes, i consider class's
also a way of organization and sometimes simplification of code.
(Yes even in web pages I believe that it can be very handy ).

Well I programmed using scripts They started at a point and went on no functions whatsoever
in the code, it was just a big happy program that worked when i finished it.

But geting to the point :
This Program was very , very extense , big two thounsand and more lines of code .
( Yes this for me is a big script ).

Well i had if conditions ones inside of others i didn't know where it started and ended .
So just to know where i was in the code i used comments .
And I believe that even for an advanced programmer this can be very handy.

Does examples given before were poor. This is not better because it shows someone,
trying to make it has a programmer, but i have learned a few things since then.

And I still like to give comments on the code it's like Documenting what you are doing.

I just remenbered a example of when i coded in Visual Basic .... Since then untill i forgot
a few things so, in order to do a job to someone a few months ago i went to get old code
that i made and guided by only comments i made it out .

You can't expect me to remember what each function that i made time ago does.
So read the header and i know it takes a control object , an array , and it returns a integer.

When you have extencible code it HELPS

:D
And you can't change my mind about that :P
Of course that i agree with your examples ...
That kind of comments is unnecessary .

And I agree on using CVS.

Posted: Tue May 20, 2003 11:21 pm
by jason
What is it with people thinking I don't like comments?

I never say comments are bad.

Commening Rule #1: Comments that require maintenance are bad comments.

Commening Rule #2: More comments means your code is less readable.

Commening Rule #3: Comments are written in another language from the code. When reading through code littered with comments, it's like reading through a book in two languages.

Those are my rules for comments. All these things are true. It's as simple as that. This doesn't mean you shouldn't use comments. Comment each function, by all means! I do it too.

Include a header in your file, it's like an introduction to a piece of written work. It's an introduction, meant to prepare you for whats coming next. That's fine!

What I am against is the "Comments == Very Good" opinions. They are wrong. Yes, opinions can be wrong, and this is an example.

Comments are good, but like anything else, in excess, they are bad. Writing good comments is just as important a skill as anything else in programming. Unfortunately, people don't spend the time necessary to learn how to write good comments.

netcrash: Your example about writing the poor code and the comments helping you out is a perfect example of where comments can be good. However, had you written good code in the first place, those comments probably wouldnt' have been necessary. Of course, without seeing the comments or the code (not that I want to), I can't be sure.

Final point: My three rules I stand by. They are rules which make sense, and will to anyone who has ever had to deal with bad comments. Comments can be good, comment can be bad. Do you need comments to write easy to read, easy to understand code? Absolutely not. Can good comments help? Absolutely.

Posted: Wed May 21, 2003 6:45 pm
by McGruff
jason wrote:Commening Rule #3: Comments are written in another language from the code.
... but at least it's a language that I understand 8O

I also tend to have a lot of long scripts. Don't see the point of creating lots of function definitions unless there's some sharable code, so lots of comments help when you come back to it later. In particular, I find it helps to explain the logic behind each block of code - it's often far from trivial to work that out from code which might in turn depend on assumptions in other scripts or in the db design.

And, I like a lot of whitespace:

Code: Select all

<?php
    // article editors can start forum topics in restricted articles; others have to be a member of the group
    $member = articleMember(stripslashes($result['groups']), $result['restricted']);  

    IF ($member == 0) {
        
        die("You do not have sufficient privileges.");

    }
    
    // check if user has the post privilege
    IF (substr(USER_REACH, 3, 1) >= POST_PRIVILEGE) {
    
        $result['title'] = stripslashes($result['title']);
        include('html/article_add_forum_post.htm');

    } ELSE {

        echo '<p class="body">You do not have sufficient privileges to start discussion.</p>';
        echo '<p class="body"><a href="index.hp?page=articles_view_' . intval($_POST['aid']) . '_' . intval($_POST['page_number']) . '">Back-></a></p>';
    
    }

?>
.. rather than:

Code: Select all

<?php
    // article editors can start forum topics in restricted articles; others have to be a member of the group
    $member = articleMember(stripslashes($result['groups']), $result['restricted']);  
    IF ($member == 0) {        
        die("You do not have sufficient privileges.");
    }    
    // check if user has the post privilege
    IF (substr(USER_REACH, 3, 1) >= POST_PRIVILEGE) {    
        $result['title'] = stripslashes($result['title']);
        include('html/article_add_forum_post.htm');
    } ELSE {
        echo '<p class="body">You do not have sufficient privileges to start discussion.</p>';
        echo '<p class="body"><a href="index.hp?page=articles_view_' . intval($_POST['aid']) . '_' . intval($_POST['page_number']) . '">Back-></a></p>';   
    }

?>

Posted: Wed May 21, 2003 7:05 pm
by jason
Well, to keep it simple, in my opinion, the following is not an example of good code:

Code: Select all

<?php

IF (substr(USER_REACH, 3, 1) >= POST_PRIVILEGE) { 

?>
Not to say it's broken, nor is it not readable. You can make out what it does, even without the comment. Indeed, I must thank you for using constants, an underused element in PHP However, what would be better would be something like this:

Code: Select all

<?php

IF ( userIsAllowed(POST_PRIVILEGE) ) { 

?>
...or...

Code: Select all

<?php

IF ( $user->isAllowed(POST_PRIVELEGE) ) { 

?>
...or you can turn it around and not base it on the action, but on the user, such as...

Code: Select all

<?php

IF ( checkPostPriv($user_object_or_a_user_based_variable) ) { 

?>
Note: Never use variable names that long =p

Anyways, each of these examples, I feel, would only make the comment become a problem, a hinderance. A pimple on a face, if you will.

Code: Select all

<?php

    // article editors can start forum topics in restricted articles; others have to be a member of the group (names stored space separated in groups field)
    $member = articleMember(stripslashes($result['groups']), $result['restricted']); 

    IF ($member = 0) {
       
        die("You do not have sufficient privileges.");

    } 

?>
That comment is begging for elimination. It should NOT be in the code. Rather, is should be located in the documentation overview or the software. However, here is a question, does that comment tell me what that code does?

Nope!!!

Okay, so this is probably sample code, however, here is the problem with the above code and the comment. The comment is misleading. Now, can anyone tell me why?

And if it takes you a few minutes to figure out, admit it. =) I know this is just an example, but it's proof of the danger in trusting in comments.

Posted: Wed May 21, 2003 7:20 pm
by McGruff
Rgr - the second comment isn't really necessary. But if I created a one-line function to do the substr() check, aren't I just burdening the parser with an unecessary function definition (not much of a burden admittedly)?

The code was posted just to show how much easier it is to read with genrous use of blank lines such as between blocks of code or before and after braces - the POST_PRIVILEGE check would actually be redundant since I've already checked if the user is a member and there's not much point in denying members of an item the ability to comment on it!

However, I do find the first comment (article editors etc..) helpful since, to me at least, this explains what the following code is about to do. If I come back later to check if my privilege system is watertight, this lets me find the block of code quickly and reminds me that the logic is (a) give editors access to everything and (b) only give others access if they are members of an item.

Why is this misleading? You've got me!

Yeah constants are good: immune to value substitution (well, they are once they're defined..) so particularly good for something like a user access level.

Posted: Wed May 21, 2003 7:40 pm
by jason
It's misleading because of this line:

Code: Select all

<?php

IF ($member = 0) { 

?>
The single =. I know you used this just as an example, but, even when I first looked at the code, and read the comment, I just assumed that the code did what the comment said! This single '=' would completely mess up what the comment said. It wasn't until I went back over it that it caught my eye.

The point isn't that you write broken code (again, I understand this wasn't meant to be production quality), however, it illustrates a comments most deadly feature: It isn't run by the computer.

Just because a comment says it does something, doesn't mean the code does it. That's why I am a big supporter of self-commenting code.
Rgr - the second comment isn't really necessary. But if I created a one-line function to do the substr() check, aren't I just burdening the parser with an unecessary function definition (not much of a burden admittedly)?
Yes and no. First, if you were just checking permissions once in one place, then yes. However, I am sure this script would be checking permissions in different areas, and would probably checking different types of permissions. So the danger here is you are doing substr's all over the place. And when something needs to be changed...well, you know the rest of the story.

Hardware is cheaper then time. Throwing in a few good functions to enhance maintenance was never a bad thing.
The code was posted just to show how much easier it is to read with genrous use of blank lines such as between blocks of code or before and after braces - the POST_PRIVILEGE check would actually be redundant since I've already checked if the user is a member and there's not much point in denying members of an item the ability to comment on it!
Yup, it's all good. Like I said, I know the code wasn't meant to be production. I agree with the whitespace contention. Whitespace is good...to a point. If I have to scroll through 2 screens to read through a 20 LOC script, their is a problem. =)
However, I do find the first comment (article editors etc..) helpful since, to me at least, this explains what the following code is about to do. If I come back later to check if my privilege system is watertight, this lets me find the block of code quickly and reminds me that the logic is (a) give editors access to everything and (b) only give others access if they are members of an item.
This is debateable. I would prefer to see the privledge system more modular, rather than ingrained into the system as it seems to be. While the comment isn't bad (I didn't particular say it was), I would rather see it in the overview documentation.

However, if the first comment was left in, so be it. However, you still run the risk of the comment becoming out of date if the permissions system changes. And then, the not-so-bad comment becomes deadly.

Attack of the Killer Comments!!!

Posted: Wed May 21, 2003 7:53 pm
by McGruff
Thanks Jason. I actually edited the == in after posting. If you ever see a post by myself, check the time and wait for ten minutes until I've finished editing all the typos :)

I'm self-taught with php so it's interesting to hear ideas about improving the way you write code.

Posted: Wed May 21, 2003 7:59 pm
by jason
Hah, didn't notice you had changed it. :D

Posted: Thu May 22, 2003 3:24 pm
by llimllib
A rant about why you should seperate even seemingly simple things into functions:

Seperating even simple things out into functions really does help. I see what you're saying, mcgruff, that you don't want to burden the interpereter more than is necessary. However, a) functions calls are very cheap (take very few CPU cycles). Because there are so many of them, interpereters and compilers are very good at making them fast. The speed you lose from a function call is negligible. b) The gains you make in code extensibility and readability are substantial.

For example, imagine a function is_member($name) which takes a username and checks to see if that username has been registered. Now imagine that you need to change this function. If you're using a good editor, it should be very easy to find this function - it'll be in the function list on the left side of an ide, or "/function is_mem" will do it in VIM. If it's a piece of code in a long block, it will be difficult to find.

Furthermore, when you change the function, you can be sure (barring pass-by-reference, with which you must take extra care) that you are avoiding harmful side effects for your new code. As long as it still accepts the same data and outputs the same data, the rest of the code will work as advertised. If, however, it is a piece of code in a long block, you must check to make sure that none of the variables used by the function are changed, which can lead to problems that are very difficult to debug.

In short, remember two things. One, "premature optimization is the root of all programming errors". Said by a very famous computer scientist (Donald Knuth, I believe), it means that, when you first write a program, you should not worry about making it fast. Design it well, then optimize it later. If you worry about losing 2 or 3 cycles, you're missing the big picture when you're programming in a very high-level language such as PHP. Two, programmer time is more valuable than CPU time in very high-level languages. Your time is worth more than the cpu's. Program in a well-designed, organized way, then find the bottlenecks, then optimize if you have to.

Posted: Thu May 22, 2003 9:36 pm
by McGruff
Some very interesting points. I think I maybe do worry about efficiency too much and your comments and others are starting to change my way of thinking. I think I'm going to be taking a long hard look at my work to date..

Posted: Fri May 23, 2003 9:19 am
by BDKR
jason wrote:
Commening Rule #1: Comments that require maintenance are bad comments.

Commening Rule #2: More comments means your code is less readable.

Commening Rule #3: Comments are written in another language from the code. When reading through code littered with comments, it's like reading through a book in two languages.

Those are my rules for comments. All these things are true. It's as simple as that. This doesn't mean you shouldn't use comments. Comment each function, by all means! I do it too.

What I am against is the "Comments == Very Good" opinions. They are wrong. Yes, opinions can be wrong, and this is an example.

Comments are good, but like anything else, in excess, they are bad. Writing good comments is just as important a skill as anything else in programming. Unfortunately, people don't spend the time necessary to learn how to write good comments.
I have to say that some of this is just downright arbitrary! But here goes....
Commening Rule #1: Comments that require maintenance are bad comments.
No, comments require maintenance because at times while creating and finding a better way during the coding process, the code changes. Therefore the comments should be changed. What if the function needs a rewrite in response to a boneheaded client that doesn't
seem to know what he wants?
Commening Rule #2: More comments means your code is less readable.
This is simply a matter of personal taste. It's also an echo of old time dinosaurs from the days when memory was extremely expensize and hard to come by. Back then, code had to be as tight and small as possible. For the most part, it's not true in general. What if the code is doing something interesting. That's the kind of stuff that should be commented. Besides, I don't want to have to read the code to see what it's doing if a comment will tell me. Even better, it may help put me in the mindset of the original coder to help better understand his logical process.

One of the single most difficult things any progger ever has to do is read someone else's code!.
Commening Rule #3: Comments are written in another language from the code. When reading through code littered with comments, it's like reading through a book in two languages.
As an extreme example, I removed all the comments from a tool that I wrote to help with emergency situations should they arise (and have done so by the way). Can anyone tell me what it does with a casual glance?

Code: Select all

#!/usr/local/bin/php -q
<?php
set_time_limit(0);
$args=&check_args();
$file_array=array();

if($args['db']!='all')
	{
	if(file_exists($args['file']))
		{
		echo "About to open file\n";
		$f1=fopen($args['db'], 'a');
		if($f1)
			{	
			$file_array[$args['db']]['fp']=$f1; 
			$file_array[$args['db']]['name']=$args['db'];
			$file_array[$args['db']]['count']=0;
			}
		else
			{ echo "Failed to open a file for the db ".$args['db']."\n"; exit; }
		}
	else
		{ echo $args['file']." doesn't exist!\n"; exit; }	
	}

$queries=file($args['file']);

$current='';
while(list( , $v)=each($queries))
	{
	if( (stristr($v, 'use')) )
		{
		if( (stristr($v, 'INSERT')) || (stristr($v, 'UPDATE')) || (stristr($v, 'DELETE')) || (stristr($v, 'ALTER')) )
			{
			if(isset($file_array[$current]['fp']))
				{ 
				fwrite($file_array[$current]['fp'], $v); 
				++$file_array[$current]['count']; 
				}
			}
		else
			{	$db=parse_use(&$v); }

		if(isset($file_array[$db]['fp']))
			{	$current=$db; }
		elseif($args['db']=='all')
			{ 
			add_fp(&$file_array, &$db); 
			$current=$db;
			}	
		else
			{ $current=''; }		
		}
		
	elseif($current!='')
		{
		if((stristr($v, 'SET TIMESTAMP')) || (stristr($v, 'SET INSERT_ID')))
			{ }
		else
			{ 
			fwrite($file_array[$current]['fp'], $v); 
			++$file_array[$current]['count'];
			} 
		}
	else
		{ }
	}	

$temp_count=0;
foreach($file_array as $x)
	{
	echo $x['name']." had a total of ".$x['count']." lines.\n";
	$temp_count=($temp_count+$x['count']);
	fclose($x['fp']);
	}


function check_args()
	{
	$data_arr=array("file"=>'', "db"=>'');
	if($_SERVER['argc']!=3)
		{ 
		echo "Incorrect argument count! Use -h for usage.\n";
		exit;
		}
	
	$data_arr['file']=&$_SERVER['argv'][1];
	$data_arr['db']=&$_SERVER['argv'][2];

	return $data_arr;
	}


function parse_use($str)
	{
	$new=str_replace("use", "", $str);
	$new=str_replace(";", "", $new);
	$new=str_replace(" ", "", $new);
	$new=trim($new);
	return $new;
	}

function add_fp($files, $db)
	{
	$fp=fopen($db, 'a');
	if($fp)
		{	
		$files[$db]['fp']=$fp;
		$files[$db]['name']=$db;		
		$files[$db]['count']=0;
		}
	else
		{ echo "Failed to open a file for the db ".$db."\n"; exit; }
	}

?>
I'll take the comments!

For the most part, this topic has led to holy wars and flame wars for as long as proggers have been progging! I really should have a talk with limlib about this. He should know better! He's prolly giggling his tail off over this one.

Anyways, like crack and tailgating, I usually don't indulge, but on this topic we have to remember that far fewer things then we like to think are concrete. Whatever an individual does that works for him or her isn't bad because it's gibberish to another. Yes, on one level, we must be concerned when we are part of a team or project, but if one is in 'lone-coder' mode, and it's his or her project, more power to 'em! These are more often than not the nuts and weirdos coding or tinkering away in a basement somewhere, clothes dirty and face unwashed, that comes up with the next big thing! We should be careful that we don't stifle or hinder with standards.

Cheers,
BDKR

p.s. ternary operators rule!

Posted: Fri May 23, 2003 10:17 am
by volka
you have to consider those rules all together not one by one.
Also note the conditions Jason mentioned. Since your code does not fulfill them you cannot prove they are wrong ;)

Posted: Fri May 23, 2003 2:08 pm
by llimllib
Sorry, BD, I really wasn't trying to troll at all :) However, I believe we have gotten a couple things out in this thread that are important. First, the importance of seperating things out into functions even when they seem simple. It appears that mcgruff has seen the light.

Second, I think this comments discussion is an important one, and perfect for the advanced forum. Perhaps it should be seperated into a seperate thread, because it's a little different from what it started out to be.

Anyway, my personal opinion on your code is that you misunderstood what Jason was trying to say about comments. I think a comment like:

Code: Select all

#initialize file_array if the values are valid
if($args['db']!='all')
   {
   if(file_exists($args['file']))
      {
      echo "About to open file\n";
      $f1=fopen($args['db'], 'a');
      if($f1)
         {   
         $file_array[$args['db']]['fp']=$f1;
         $file_array[$args['db']]['name']=$args['db'];
         $file_array[$args['db']]['count']=0;
is OK. However, what you don't want is:

Code: Select all

$file_array[$args['db']]['fp']=$f1;  #set fp to point to the file
         $file_array[$args['db']]['name']=$args['db'];    #set name to point to the db name
         $file_array[$args['db']]['count']=0;       #initialize the count to 0
These comments are obviated by the correct choice of variable names, and simply get in the way of my reading the code. Further, the first comment requires no maintenance; this block should always do what that says, while it's more likely that the second set of comments would require maintenance.

I think all Jason is trying to say is to comment wisely.