<?php
$location = htmlspecialchars($_SERVER['HTTP_REFERER']);
$email = strtolower(mysql_real_escape_string($_REQUEST['email']));
$hashed = substr(md5($email), 0, 4);
$invites = 0;
$uri = $_REQUEST['uri'];
$ip = $_SERVER['REMOTE_ADDR']; //get IP
if (!$email){
echo "<p>Please enter a valid e-mail address<br><ul><span><a href='$location'>go back</span></li></ul></p>";
}
elseif (filter_var($email, FILTER_VALIDATE_EMAIL)){
$insertinto = mysql_query("INSERT IGNORE INTO table (email, hash, invites, ip, signup_time, signup_date, signup_jointime) VALUES ('$email', '$hashed', '$invites', '$ip', CURTIME(), NOW(), NOW())") or die('error');
if (mysql_affected_rows() == 0){
$getlink = mysql_query("SELECT hash,invites from table WHERE hash='$hashed'");
while($row = mysql_fetch_assoc($getlink)){
$existinghash = $row['hash'];
$totalinvites = $row['invites'];
}
echo "<p><span>Your back!<br>";
if ($totalinvites > 0){
echo "You have a total of ".$totalinvites." invites.</span></p>";
}
else{
echo "No invites, yet.</p>";
}
echo "<p><span>The more friends you invite, the sooner you'll get access!<br>paste this URL below into Twitter, Facebook, or an email:</p><p>Share Link <input class='referral' type=text size=27 readonly onClick=select(); value=\"".$sitedomain."".$existinghash."\"></span></p>";
echo "<p>Or use the buttons:<br></p>";
}
elseif ($uri){
$updateinvites = mysql_query("UPDATE table SET invites=invites+1 WHERE hash='$uri'") or die('error!');
echo "<p>The more friends you invite, the sooner you'll get access!<br>Copy/paste this URL below into Twitter, Facebook, or an email:</p><p>Share Link <input class='referral' type=text size=27 readonly onClick=select(); value=\"".$sitedomain."".$hashed."\"></p>";
echo "<p>Or use the buttons:<br></p>";
}
else{
echo "<p><input class='referral' type=text size=27 readonly onClick=select(); value=\"".$sitedomain."".$hashed."\"></p>";
echo "<p>Or use the buttons:<br></p>";
}
}
else {
echo "<p class='invaild_email'>Please enter a valid e-mail address<br><ul><span><a href='$location'>go back</span></li></ul></p>";
}
?>
Refactorings
No refactoring yet !
Ants
January 16, 2012, January 16, 2012 01:53, permalink
I'm sorry if this sounds harsh, but I'm at a loss to understand how can you say that "everything works"?
Issues:
1) You are outputting incorrect HTML. You don't have opening or closing <html>, or <body> elements. You have mismatched <li> elements. You are mixing XHTML and HTML.
2) On your initial insert, you are inserting the hash of the email address in the hash column, yet later you are attempting to bump up the invite count by matching the hash column against the URI. How does the invite count ever get bumped up?
3) Your code is susceptible to SQL injection. For example you pick up $_REQUEST['uri'] without any escaping. I recommend using SQL prepare and bind operations to help insulate yourself from SQL injection attacks.
4) Your while loop to comptue totalinvites doesn't sum up the values from the rows with a matching hash. Even if it did, it would have been more efficient to use the SQL SUM() operator rather than doing the looping yourself.
5) It doesn't look like you are opening the SQL database connection.
openid.aol.com/zaknaji
January 16, 2012, January 16, 2012 18:50, permalink
Your right on all your points but in my defense this is just part of the code and wanted to see if and how this part only would be refactored.
openid.aol.com/zaknaji
January 16, 2012, January 16, 2012 18:50, permalink
Your right on all your points but in my defense this is just part of the code and wanted to see if and how this part only would be refactored.
openid.aol.com/zaknaji
January 16, 2012, January 16, 2012 18:50, permalink
Your right on all your points but in my defense this is just part of the code and wanted to see if and how this part only would be refactored.
Ants
January 17, 2012, January 17, 2012 01:38, permalink
This isn't a true refactoring since refactoring shouldn't change behavior including any existing bugs. This is how I envision the code working. I'm making a big assumption that behavior is that the invite count gets bumped up based on the email address being seen again.
BTW, your use of $hashed picking up only the first 4 characters of the MD5 hash is very susceptible to collisions because MD5 computes 128 bits, but you are only using the first 16 bits.
Code below is untested but hopefully it gets you going:
<?php
$email = strtolower(mysql_real_escape_string($_REQUEST['email']));
if ($email && filter_var($email, FILTER_VALIDATE_EMAIL)):
$getlink = mysql_query("SELECT SUM(invites) from table WHERE email='$email'");
if($row = mysql_fetch_assoc($getlink)):
$totalinvites = $row['SUM(invites)'] + 1;
mysql_query("UPDATE table SET invites=invites+1 WHERE email='$email'") or die('error!');
echo "<p><span>Your back!<br>You have a total of ".$totalinvites." invites.</span></p>";
else:
$hashed = md5($email);
$ip = $_SERVER['REMOTE_ADDR'];
mysql_query("INSERT IGNORE INTO table " .
"(email, hash, invites, ip, signup_time, signup_date, signup_jointime) VALUES " .
"('$email', '$hashed', '0', '$ip', CURTIME(), NOW(), NOW())") or die('error');
echo "<p>No invites, yet.</p>";
endif;
?>
<p>
<span>
The more friends you invite, the sooner you'll get access!<br />
Copy/paste this URL below into Twitter, Facebook, or an email:
</span>
</p>
<p>
Share Link
<input class="referral"
type="text"
size="27"
readonly="1"
onClick="select();"
value="<?php echo $sitedomain.$hashed; ?>" />
</p>
<p>
Or use the buttons:<br />
</p>
<?php else: ?>
<p class="invalid_email">
Please enter a valid e-mail address</p>
<ul>
<li>
<span><a href="<?php echo htmlspecialchars($_SERVER['HTTP_REFERER']) ?>">go back</span>
</li>
</ul>
<?php endif; ?>
Ants
January 17, 2012, January 17, 2012 11:50, permalink
I had some mismatched tags, duplicate parallel code, and some view logic still intertwined with the behavior. This should fix the mismatched tags, get rid of the parallel duplication, do a bit more separation of concerns by blocks.
<?php
$email = strtolower(mysql_real_escape_string($_REQUEST['email']));
if ($email && filter_var($email, FILTER_VALIDATE_EMAIL)):
$getlink = mysql_query("SELECT SUM(invites) from table WHERE email='$email'");
if($row = mysql_fetch_assoc($getlink)):
$totalinvites = $row['SUM(invites)'] + 1;
$query = "UPDATE table SET invites=invites+1 WHERE email='$email'";
$message = "You’re back!<br />You have a total of $totalinvites invites.";
else:
$hashed = md5($email);
$ip = $_SERVER['REMOTE_ADDR'];
$query = "INSERT INTO table " .
"(email, hash, invites, ip, signup_time, signup_date, signup_jointime) VALUES " .
"('$email', '$hashed', 0, '$ip', CURTIME(), NOW(), NOW())";
$message = "No invites, yet.";
endif;
mysql_query($query) or die('error');
?>
<p>
<span>
<?php echo $message; ?>
</span>
</p>
<p>
<span>
The more friends you invite, the sooner you'll get access!<br />
Copy/paste this URL below into Twitter, Facebook, or an email:
</span>
</p>
<p>
Share Link
<input class="referral"
type="text"
size="27"
readonly="1"
onClick="select();"
value="<?php echo $sitedomain . $hashed; ?>" />
</p>
<p>
Or use the buttons:<br />
</p>
<?php else: ?>
<p class="invalid_email">
Please enter a valid e-mail address
</p>
<ul>
<li>
<span>
<a href="<?php echo htmlspecialchars($_SERVER['HTTP_REFERER']) ?>">go back</a>
</span>
</li>
</ul>
<?php endif; ?>
openid.aol.com/zaknaji
January 17, 2012, January 17, 2012 14:53, permalink
@Ants Thanks for showing me a few things I should have done or should be doing. Unfortunately the code above is not working properly but gave me ideas for much improvements.
Just completed this a week ago and everything works. Now I just want to optimize the code. Mind you I am just a beginner examples would be great thanks for any help.