someone please critique my code

hkarre

This is my first post here and I was hoping to get some people to critique my code. I am a PHP newbie and am ready, willing, and able to learn. The attached code is for a simple shoutbox and am looking for ways to improve it, make it more efficient. I have tested this code on my own server and it seems to work correctly. Well, that is all and thanks for helping me out!

LordShryku

A few comments...

if ($_POST[submit]=="Shout"){

1) Validate first that it's set with [man]isset[/man] or [man]empty[/man]. Throws a notice of "undefined index" if you don't.
2) Quote literals in array keys. It will throw another notice of "undefined constant" without. This applies to all of your arrays.

if(isset($_POST['submit']) && $_POST['submit'] == "Shout") {

I'd also suggest adopting some sort of standard naming convention for your variables. You seem to have some upper case, some lower case, some both.

BuzzLY

<sigh>

[monotone voice][FONT=courier new]This forum is for asking for critiques of your code only not...[/FONT][/monotone voice]

Oh. It is. My bad. Sorry. Carry on 😃

Shrike

mysql_db_query()

Note: This function has been deprecated since PHP 4.0.6. Do not use this function. Use mysql_select_db() and mysql_query() instead.

C'mon we're on PHP 4.3.7 now 😉

Check to see if this is a real submission or if the user just hit the refresh button

You can avoid this by doing a header redirect after the form data is submitted to the database.

There doesn't seem to be any validation whatsoever done on the form data. I'd recommend at least checking to see if gpc_magic_quotes is enabled, and also consider using [MAN]html entities[/MAN]

Why bother only storing 10 entries? A simpler approach would be to only select 10 entries during the query. You can use the LIMIT and RAND() MySQL stuff to ensure your getting what you want.

Other than that, good stuff 🙂

hkarre

First off thanks for helping me out, the comments and suggestions made by LordShryku and Shrike have been extremely helpful and I really appreciate it. I am re-attaching a new and improved shoutbox script with some of the easier changes already implemented. I still have a couple of questions though.

You can avoid this by doing a header redirect after the form data is submitted to the database.

I am assuming you mean to use the header() function. Do you mean to re-direct the user to a simple thank you page or to re-direct the user to same page. I'm not sure I am understanding you fully and not sure how this would help. Basically what I want to do is to make it so that a user cannot repeatedly post identical data and therby filling up the screen with a single post.

As far as validating the form I am taking the suggestion and have begun working on it. I will post it once I get it working.

Thanks.😃

Looks like file didn't upload for some reason, will try again.

Shrike

Redirect wherever you want, doing that will renew the HTTP headers, and so lose the form POST 🙂

hkarre

can i use the following?

header("Location: $_PHP_SELF"); // to refresh the page and lose the form post (as you say)

hkarre

I inserted the following in my code right after the database insertion but it doesn't seem to clear the POST data when re-directing back to the same page and when the user hits the refresh button, they still get the message about re-sending form data and the message is posted again, any suggestions?

header("Location: $_SERVER[PHP_SELF]");