My first script

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
draco2317
Forum Newbie
Posts: 19
Joined: Mon Sep 18, 2006 9:17 pm

My first script

Post by draco2317 »

This is the first (and so far only) php script i have written. I was wanting to know how badly I did :)

Code: Select all

<?php
include("include/dbconnect.php");
include("include/settings.php");

$recover=mysql_query("SELECT name, user, pass FROM models WHERE email = '$email'");
$subject='Log In Info';

$row = mysql_fetch_assoc($recover);
if ($row == "")
{
echo "The email address you submitted was not found in our database.  Check to make sure you entered in the address in correctly.";
}
else 
{ 
$mesaj = "From time to time people forget many things, dont feel bad. Your information is listed below.\n\nUsername: $row[user]\nPassword: $row[pass]";
mail($email, $subject , $mesaj ,"From: webmaster@reddragonmodels.net\nReply-To: webmaster@reddragonmodels.net\n");
}
?>

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>

<body>
<?php include("header.php"); ?>
<?php print "$row[name],\n"; ?><br><br>
   <font size="+1"><b>Your Username and password has been sent<br> to the email account your registered with.</b></font><br>
</body>
</html>
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

Looks ok to me. A couple of things:

- Why is settings included before dbconnect? It seem like the DB connection info could be in settings to centralize configuration data.

- You might want to add error checking in case the query fails.

- If no row is found you echo an error message. Better would be to assign the value to a string and display it below in the <body>. The same goes for the message if a row is found -- assign the text that is currently at the bottom within the if. Then the HTML section at the bottom just displays one string or another.
(#10850)
draco2317
Forum Newbie
Posts: 19
Joined: Mon Sep 18, 2006 9:17 pm

Post by draco2317 »

- Why is settings included before dbconnect? It seem like the DB connection info could be in settings to centralize configuration data.
Honestly I dont know. The script i wrote is based off one i bought. The designer forgot to write that page, and said it would be a wihle before we could do so, so i wrote it myself. The includes were always laid out that way
- You might want to add error checking in case the query fails.
I completely missed that, after reading that i tested it by submitting empty variables and it acted as if everything was okay. Thanks for pointing that out.
wyrmmage
Forum Commoner
Posts: 56
Joined: Sat Oct 28, 2006 12:43 pm
Location: boise, ID

Post by wyrmmage »

looks pretty good, although I would point out a couple of things.
First: why are you switching back and forth between \n and <BR>?
Secondly: most people use echo"" instead of print"". print"" returns a value saying whether whatever it was priting to the screen actually got there and, since most people don't normally check to see if a string got put on the screen or not, it isn't used much. Most people use echo"" or, in my case, I use echo(''); If you use echo, you must do something like this:

Code: Select all

<?
$x = 1;
echo('$x is this: ' . $x . '<BR>');
?>
instead of going:

Code: Select all

<?
$x = 1;
echo"the variable x is this: $x <Br>";
?>
Using echo('') may seem awkward and not terribly useful at first, but I prefer it, mostly because then I don't have to escape double quotes, which is what I use in HTML tags ;)

-wyrmmage
Post Reply