Jump to content

Archived

This topic is now archived and is closed to further replies.

corbin

Critique my script

Recommended Posts

Well i got bored the other day so i made a little chat box thing... It simply stores the comments in a table along with some other stuff. Anyways if someone could tell me anything i should have coded different to make the coding shorter or more efficient i would appreciate it. :)

Thanks -Corbin

( get [a href=\"http://corbin.no-ip.org/chat.zip\" target=\"_blank\"]http://corbin.no-ip.org/chat.zip[/a] )
( working copy [a href=\"http://corbin.no-ip.org/dev/chat/index.php\" target=\"_blank\"]http://corbin.no-ip.org/dev/chat/index.php[/a] )

Share this post


Link to post
Share on other sites
I can't speak to the PHP side... on the MySQL side, I have a few comments:

-mysql_result() is inefficient -- you should be using mysql_fetch_array(), and then retrieiving the desired piece by column name

-if you're already using VARCHARs, there's no reason to have them limited to 20 -- go all the way to 255

-storing IPs as strings is a poor choice, and limits the way you can query this information; use INET_ATON() and store it as an integer.

-you should always explicitly include the column names when you issue INSERT statements... don't rely on the order of the columns never changing

Share this post


Link to post
Share on other sites
[!--quoteo(post=369958:date=Apr 29 2006, 03:39 PM:name=fenway)--][div class=\'quotetop\']QUOTE(fenway @ Apr 29 2006, 03:39 PM) [snapback]369958[/snapback][/div][div class=\'quotemain\'][!--quotec--]
I can't speak to the PHP side... on the MySQL side, I have a few comments:

-mysql_result() is inefficient -- you should be using mysql_fetch_array(), and then retrieiving the desired piece by column name

-if you're already using VARCHARs, there's no reason to have them limited to 20 -- go all the way to 255

-storing IPs as strings is a poor choice, and limits the way you can query this information; use INET_ATON() and store it as an integer.

-you should always explicitly include the column names when you issue INSERT statements... don't rely on the order of the columns never changing
[/quote]


Fixed the mysql_result use for the loop to display the posts... On the detail screen I dont think it will be afftected much since its only querying 1 row and then printing one result. It builds the tables where all the VARCHARs are 255 length now. With the IP thing... I couldnt get INET_ATON to work correctly... I think it's becuase I'm using MySQL 4.1... not sure though. Also, I went through and fixed all the INSERT statements... Anything else you can think of?

Share this post


Link to post
Share on other sites
I'm sure that the INET functions were supported as far back at 3.23 -- there's nothing "wrong" with storing them as strings, it's just that it becomes useless for anything but logging, and wastes space. INT UNSIGNED is a better choice... simply wrap the dotted-quad in an INET_ATON() call in your INSERT statements. Otherwise, the MySQL component looks just fine... you'll have to get someone else's opinion on the PHP stuff, since I do everything in Perl.

Share this post


Link to post
Share on other sites
[!--quoteo(post=370005:date=Apr 29 2006, 08:40 PM:name=fenway)--][div class=\'quotetop\']QUOTE(fenway @ Apr 29 2006, 08:40 PM) [snapback]370005[/snapback][/div][div class=\'quotemain\'][!--quotec--]
I'm sure that the INET functions were supported as far back at 3.23 -- there's nothing "wrong" with storing them as strings, it's just that it becomes useless for anything but logging, and wastes space. INT UNSIGNED is a better choice... simply wrap the dotted-quad in an INET_ATON() call in your INSERT statements. Otherwise, the MySQL component looks just fine... you'll have to get someone else's opinion on the PHP stuff, since I do everything in Perl.
[/quote]

I tried

INSERT VALUES ('INET_ATON('192.168.2.1')') into IP; earlier but it kept putting 0... I also tried other formats and couldnt get it to work... I had it as int though not int unsigned so ill try that right now.

Share this post


Link to post
Share on other sites
[!--quoteo(post=370015:date=Apr 29 2006, 09:28 PM:name=Corbin)--][div class=\'quotetop\']QUOTE(Corbin @ Apr 29 2006, 09:28 PM) [snapback]370015[/snapback][/div][div class=\'quotemain\'][!--quotec--]
I tried

INSERT VALUES ('INET_ATON('192.168.2.1')') into IP; earlier but it kept putting 0... I also tried other formats and couldnt get it to work... I had it as int though not int unsigned so ill try that right now.
[/quote]

Hehe not sure what i was doing wrong earlier but i got it working now... Thanks for the advice :).

Share this post


Link to post
Share on other sites
[!--quoteo(post=370019:date=Apr 29 2006, 10:09 PM:name=Corbin)--][div class=\'quotetop\']QUOTE(Corbin @ Apr 29 2006, 10:09 PM) [snapback]370019[/snapback][/div][div class=\'quotemain\'][!--quotec--]
Hehe not sure what i was doing wrong earlier but i got it working now... Thanks for the advice :).
[/quote]
The "unsigned" is important, otherwise IPs with the first octet > 127 won't be stored properly becuase their numeric equivalent would be too large for the column to store.

Share this post


Link to post
Share on other sites

×

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.