comment on my php code

[wwhassupp]

Hi, I'm new in php and MySQL.
I've made a php file to update user password. Below is the code, and I would like to have some comment on it. Thanks!

chgpw.php

<?php
session_start();
header("Cache-control: private");
@error_reporting('E_ALL');
@ini_set('display_errors', '1');

if(!isset($_SESSION['logged']) or $_SESSION['logged'] != TRUE)
{
$_SESSION['caller'] = $_SERVER['PHP_SELF'];
header("Location: http://www.xxxxxxxxxxxxxxxx.com/login.php");
exit;
}
?>
<?php
if (isset($_SESSION['logged']))
{
require("connecttodatabase.php");

if (($_POST['oldpassword'] !=='') && ($_POST['newpassword'] !=='') && ($_POST['newpassword2'] !==''))
{
if (isset($_POST['oldpassword']) && isset($_POST['newpassword']) && isset($_POST['newpassword2']))
{
$oldpassword = mysql_real_escape_string($_POST['oldpassword']);
$md5_oldpw = md5($oldpassword);

$sql = "SELECT password
FROM members
WHERE username = '$userId'
AND password = '$md5_oldpw'";

$result = mysql_query($sql)
or die('Query failed. ' . mysql_error());

if (($result and mysql_num_rows($result) == 1) && ($_POST['newpassword'] == $_POST['newpassword2']))
{
$newpassword = mysql_real_escape_string($_POST['newpassword']);
$md5_newpw = md5($newpassword);
$updatepw = "UPDATE members SET password='$md5_newpw' WHERE username ='$userId'";
$result = mysql_query($updatepw) or die('Query failed. ' . mysql_error());
$msg = "<p id='msg'>Password changed.。</p>";
}
else
{
$msg = "<p id='msg'>Error: old password and/or the two new password not match</p>";
}

}
}
else
{
$msg = "<p id='msg'>Error: Please enter old password and/or new password</p>";
}

}
?>
...
...
...
<form action='<?php echo $_SESSION['PHP_SELF'] ?>' method='post'>
<table width="300" border="0" align="center" cellpadding="0" cellspacing="0" class="tableborder02">
<tr>
<td width="120" height="25" valign="middle"><p class="header01">CHANGE PASSWORD</p></td>

<td width="180" valign="middle" class="txtincell">&nbsp;</td>
</tr>
<tr>
<td height="25" valign="middle" bgcolor="#66CCFF" class="txtincell">Old password: </td>

<td valign="middle" bgcolor="#FFFFCC" class="txtincell"><input name="oldpassword" type="password" size="16" maxlength="16"></td>
</tr>
<tr>
<td height="25" valign="middle" bgcolor="#66CCFF" class="txtincell">New password: </td>

<td valign="middle" bgcolor="#FFFFCC" class="txtincell"><input name="newpassword" type="password" size="16" maxlength="16"></td>
</tr>
<tr>
<td height="25" valign="middle" bgcolor="#66CCFF" class="txtincell">Confirm new password: </td>

<td valign="middle" bgcolor="#FFFFCC" class="txtincell"><input name="newpassword2" type="password" size="16" maxlength="16"></td>
</tr>
<tr>
<td height="25" valign="middle" class="txtincell"></td>

<td valign="middle" class="txtincell"><input type="submit" name="submit" value="Submit"></td>
</tr>
<tr>
<td height="25" colspan="2" valign="middle" align="center" class="txtincell"><font color="#FF0000"><?php
if(!empty($msg))
{
echo $msg;
}
?></font></td>
</tr>
</table>
</form>
....
....

[chazzy]

You don't need the select statement. Having the user logged in and getting their old password would be enough to verify both who they are and what the password is. You can just encrypt the old password after you receive it in the form and pass that in the update. Then check if the update worked or not.

Also, your code is procedural. The best code is written in an object oriented style.

[bokeh]

check if the update worked or not.What method would you use to do that?

[ShrineDesigns]

try this, some of you if statements are redundant<?php
session_start();
header("Cache-Control: private");

if(!isset($_SESSION['logged']))
{
$_SESSION['caller'] = $_SERVER['PHP_SELF'];
header("Location: http://www.xxxxxxxxxxxxxxxx.com/login.php");
exit;
}
else
{
if(isset($_POST['oldpassword'], $_POST['newpassword'], $_POST['newpassword2']) && $_POST['newpassword'] == $_POST['newpassword2'])
{
$result = mysql_query("SELECT 1 FROM `members` WHERE `username` = '$userid' AND `password` = '" .md5($_POST['oldpassword']). "'");

if(!$result || !mysql_num_rows($result))
{
// error
}
else
{
// ok
}
}
else
{
// error
}
}
?>

[wwhassupp]

Also, your code is procedural. The best code is written in an object oriented style.

What's that mean? Can you explain it a more little bit? Thanks!

[wwhassupp]

try this, some of you if statements are redundant<?php
session_start();
header("Cache-Control: private");

if(!isset($_SESSION['logged']))
{
$_SESSION['caller'] = $_SERVER['PHP_SELF'];
header("Location: http://www.xxxxxxxxxxxxxxxx.com/login.php");
exit;
}
else
{
if(isset($_POST['oldpassword'], $_POST['newpassword'], $_POST['newpassword2']) && $_POST['newpassword'] == $_POST['newpassword2'])
{
$result = mysql_query("SELECT 1 FROM `members` WHERE `username` = '$userid' AND `password` = '" .md5($_POST['oldpassword']). "'");

if(!$result || !mysql_num_rows($result))
{
// error
}
else
{
// ok
}
}
else
{
// error
}
}
?>
I've tried it, but, if only one of the three input fields (oldpassword, newpassword, confirm new password (i.e. newpassword2)) is filled, the form still process. I want the users to fill in ALL three fields in order to change their password.

[NogDog]

Also, your code is procedural. The best code is written in an object oriented style.
That's debatable, depending on what you mean by "best." If re-usability and maintainability are your criteria, then OO is best. If performance is your criteria, then procedural is probably best. (As most [all?] of what I do with PHP is not "rocket science", I tend toward OO techniques now that I've learned more about them and am starting to get comfortable with them.)

For more on OO in PHP, see http://us3.php.net/manual/en/language.oop.php (PHP4) and http://us3.php.net/manual/en/language.oop5.php (PHP5).