To register for an Internet.com membership to receive newsletters and white papers, use the Register button ABOVE.
To participate in the message forums BELOW, click here
PHPBuilder.com  
 

 

Go Back   PHPBuilder.com > PHP Help > Code Critique

Code Critique Having someone critique your code is always a great way to hone the skills. Stop in and post your code to see what your peers may have done differently.

Reply
 
Thread Tools Rate Thread Display Modes
Old 10-19-2003, 02:55 AM   #1
Ed Koren
Junior Member
 
Join Date: Oct 2003
Posts: 5
IMAP - keeping mailing list up-to-date

Hi there,

I was wondering if anyone could have a look at a small script I wrote that I want to use to keep a mailing list up-to-date. The aim is to periodically execute it as a cron job, picking out the returned mail messages and deleting the original recipient from the mailing list. I am certainly not an advanced PHP programmer and therefore I am not sure if the script is reliable - testing it has been successful though.

// Retrieve headers from mailbox
$headers = @imap_headers($m_connect)
or die("Couldn't retrieve headers");
// Get number of messages
$num_emails = sizeof($headers);
// Loop through headers to locate Returned Mail messages
for($i = 1; $i < $num_emails+1; $i++) {
$mailHeader = @imap_headerinfo($m_connect, $i);
$from = $mailHeader->from[0];
if ($from->mailbox == "MAILER-DAEMON") {
// If returned mail message, get body of message
$b = imap_body($m_connect, $i);
// Remove occurrences of own domain from body
$b = ereg_replace("@mydomain.com","",$b);
// Check if it's a permanent error - not sure if this is a reliable way
if (ereg("permanent", $b) | ereg("fatal", $b)) {
// Fish for failed address
ereg('[a-zA-Z0-9_\.\-]+@[a-zA-Z0-9\-]+\.[a-zA-Z0-9\-\.]*', $b, $regs);
$addr = $regs[0];
// Delete address from db
$sql = "DELETE * FROM mailinglist WHERE ml_addr = '$addr'";
$result = mysql_query($sql, $db);
// Mark for deletion
imap_delete ($m_connect, $i);
}
}
// Delete all messages marked for deletion
imap_expunge ($m_connect);

}
imap_close($m_connect);

Any suggestions to improve it would be very much appreciated.

Ed
Ed Koren is offline   Reply With Quote
Old 10-19-2003, 03:20 AM   #2
Moonglobe
Better fan than rebelo!
 
Moonglobe's Avatar
 
Join Date: Apr 2003
Location: brain://localhost:left-side
Posts: 2,381
try wrapping it in [php] tags, then we'll have a look
__________________
there's no place i can be, since i found serenity.
Moonglobe is offline   Reply With Quote
Old 10-19-2003, 06:35 PM   #3
Ed Koren
Junior Member
 
Join Date: Oct 2003
Posts: 5
script incl. php tags

Here it is including php tags...first time posting here, never had a look at the posting guidelines. My apologies for that.

PHP Code:
// Retrieve headers from mailbox
$headers = @imap_headers($m_connect);

// Get number of messages
$num_emails = sizeof($headers);

// Loop through headers to locate Returned mail messages
for($i = 1; $i < $num_emails+1; $i++) {
$mailHeader = @imap_headerinfo($m_connect, $i);
$from = $mailHeader->from[0];
if (
$from->mailbox == "MAILER-DAEMON") {

// If returned mail message, get body of message
$b = imap_body($m_connect, $i);

// Remove occurrences of own domain from body
$b = ereg_replace("@mydomain.com","",$b);

// Check if it's a permanent error
if (ereg("permanent", $b) | ereg("fatal", $b)) {

// Fish for failed address
ereg('[a-zA-Z0-9_\.\-]+@[a-zA-Z0-9\-]+\.[a-zA-Z0-9\-\.]*', $b, $regs);
$addr = $regs[0];

// Delete address from db
$sql = "DELETE * FROM mailinglist WHERE ml_addr = '$addr'";
$result = mysql_query($sql, $db);

// Mark for deletion    
imap_delete ($m_connect, $i);
}
}

// Delete all messages marked for deletion
imap_expunge ($m_connect);

}
imap_close($m_connect);

Last edited by Ed Koren; 10-19-2003 at 09:29 PM.
Ed Koren is offline   Reply With Quote
Old 10-19-2003, 07:48 PM   #4
LordShryku
kung foo code monkey
 
LordShryku's Avatar
 
Join Date: Aug 2002
Location: Occupational Hypnotherapy
Posts: 7,473
I think he meant the PHP VB Code tags....
Check this page
LordShryku is offline   Reply With Quote
Old 10-19-2003, 09:33 PM   #5
Ed Koren
Junior Member
 
Join Date: Oct 2003
Posts: 5
Thanks for that, I must appear brainless ;-) Makes the code much more readable indeed. I fixed it in the above post so it would be great if someone could actually comment on the script.

Ed
Ed Koren is offline   Reply With Quote
Old 10-20-2003, 07:11 AM   #6
Weedpacket
Custom User Title™
 
Weedpacket's Avatar
 
Join Date: Aug 2002
Location: Rapid Offensive Unit "Foreign Object Damage"
Posts: 19,122
I guess I should brush up on what IMAP actually does one of these millennia... Too many FLEAs...

I can suggest one improvement:
PHP Code:
$b = ereg_replace("@mydomain.com","",$b);
is in a sense incorrect: since . will match any character, the above expression will happily turn "@mydomainacom.com" into ".com". Okay, a rather contrived example, but still, the principle's there.

Fortunately, while the . could just be escaped (i.e., use \.), the fact that there isn't anything particular regexy about that expression and is actually just a string means that
PHP Code:
$b = str_replace("@mydomain.com","",$b);
would see a marked performance improvement.

Similarly, using strpos instead of ereg to detect plain substrings in longer strings would also be an efficiency gain.

The only other tiddly little change I can see would be turning
PHP Code:
for($i = 1; $i < $num_emails+1; $i++)
into
PHP Code:
for($i = 1; $i <= $num_emails; $i++)
....saves one addition per iteration.

(And is it "DELETE * FROM" or just "DELETE FROM"?)

All pretty minor, but hey - I get paid for being pedantic.

Phew, and I managed to get through all that without saying "use preg_match() instead!" ..... damn.
__________________
On two occasions I have been asked [by Members of Parliament], "Pray, Mr. Babbage, if you put into the machine wrong figures, will the right answers come out?" I am not able rightly to apprehend the kind of confusion of ideas that could provoke such a question.
Weedpacket is offline   Reply With Quote
Old 10-22-2003, 03:47 AM   #7
Ed Koren
Junior Member
 
Join Date: Oct 2003
Posts: 5
Quote:
Originally posted by Weedpacket

(And is it "DELETE * FROM" or just "DELETE FROM"?)
Yes, it is "DELETE FROM" indeed ;-) Thanks for the input, I shall take it into account when rewriting the script. I expected a comment on
PHP Code:
if (ereg("permanent", $b) | ereg("fatal", $b))
because it seems too simple, although it seems to work just fine.

Anyway, thanks.

Ed
Ed Koren is offline   Reply With Quote
Old 10-22-2003, 10:48 AM   #8
Moonglobe
Better fan than rebelo!
 
Moonglobe's Avatar
 
Join Date: Apr 2003
Location: brain://localhost:left-side
Posts: 2,381
ya that works, but as Weed said, you should replace tem with strpos() calls for a pretty hefty performance gain.
__________________
there's no place i can be, since i found serenity.
Moonglobe is offline   Reply With Quote
Reply

Bookmarks


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes Rate This Thread
Rate This Thread:

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is Off
HTML code is Off
Forum Jump


All times are GMT -4. The time now is 02:23 PM.






Acceptable Use Policy

internet.comMediabistrojusttechjobs.comGraphics.com

WebMediaBrands Corporate Info


Advertise | Newsletters | Feedback | Submit News

Legal Notices | Licensing | Permissions | Privacy Policy


Powered by vBulletin® Version 3.7.2
Copyright ©2000 - 2009, Jelsoft Enterprises Ltd.