Critique my script

[corbin]

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] )

[fenway]

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

[corbin]

[!--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?

[fenway]

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.

[corbin]

[!--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.

[corbin]

[!--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 :).

[fenway]

[!--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.