Added the source in the first post. Go easy on me, lol.
1. I don't comment anything... sorry in advance.
2. I realize now I could have done stuff a whole lot differently, but I didn't want to go back and re-do it all.
Printable View
Added the source in the first post. Go easy on me, lol.
1. I don't comment anything... sorry in advance.
2. I realize now I could have done stuff a whole lot differently, but I didn't want to go back and re-do it all.
What is this and why are you doing it?PHP Code:$tid = $_POST['tid'];
$tid = preg_replace("/[^0-9]/","", $tid);
removing any character that is not numeric.Quote:
Originally Posted by AngraMainyu
I can use a for-loop in those $_POST to shrink the code, also can't you do this:
Why add it to a variable o.O?PHP Code:$tid = preg_replace("/[^0-9]/", "", $_POST['tid']);
Because I'm still not a pro lol, this code is actually more condensed than it originally was. I'll fix all that tho.Quote:
Originally Posted by iAkira
I tried.. o.O? anyways, I'll recheck this tomorrow for more ways to shrink this. Goodnight.
thanks for the inputQuote:
Originally Posted by iAkira
So I turned this:
into this:PHP Code:$tid = $_POST['tid'];
$tid = preg_replace("/[^0-9]/","", $tid);
$brand = $_POST['brand'];
$brand = preg_replace("/[^A-Za-z0-9]/", "", $brand);
$serial = $_POST['serial'];
$serial = preg_replace("/[^A-Za-z0-9]/", "", $serial);
$fname = $_POST['fname'];
$fname = preg_replace("/[^A-Za-z0-9]/", "", $fname);
$lname = $_POST['lname'];
$lname = preg_replace("/[^A-Za-z0-9]/", "", $lname);
$phone = $_POST['phone'];
$phone1 = preg_replace("/[^0-9]/","", $phone);
$serial = $_POST['serial'];
$serial = preg_replace("/[^A-Za-z0-9]/", "", $serial);
$comments = $_POST['comments'];
$comments = mysql_real_escape_string($comments);
$backup = $_POST['backup'];
$backup = preg_replace("/[^0-9]/","", $backup);
$virus = $_POST['virus'];
$virus = preg_replace("/[^0-9]/","", $virus);
$install = $_POST['install'];
$install = preg_replace("/[^0-9]/","", $install);
$email = $_POST['email'];
if (filter_var($email, FILTER_VALIDATE_EMAIL) == TRUE)
{
$email = $email;
}
else
{
$email = preg_replace("/[^A-Za-z0-9]/", "", $email);
}
PHP Code:$tid = preg_replace("/[^0-9]/","", $_POST['tid']);
$brand = preg_replace("/[^A-Za-z0-9]/","", $_POST['brand']);
$serial = preg_replace("/[^A-Za-z0-9]/","", $_POST['serial']);
$fname = preg_replace("/[^A-Za-z0-9]/","", $_POST['fname']);
$lname = preg_replace("/[^A-Za-z0-9]/","", $_POST['lname']);
$phone1 = preg_replace("/[^0-9]/","", $_POST['phone']);
$serial = preg_replace("/[^A-Za-z0-9]/","", $_POST['serial']);
$backup = preg_replace("/[^0-9]/","", $_POST['backup']);
$virus = preg_replace("/[^0-9]/","", $_POST['virus']);
$install = preg_replace("/[^0-9]/","", $_POST['install']);
$comments = mysql_real_escape_string($_POST['comments']);
$email = $_POST['email'];
if (filter_var($email, FILTER_VALIDATE_EMAIL) != TRUE)
{
$email = preg_replace("/[^A-Za-z0-9]/", "", $email);
}
Looks good, they codes are pretty clean enough
You could use switch statement on the index page.
Something like:
Haven't coded PHP so I don't know if there is something else wrong with the code but using switch statements instead of piles and piles of elseifs makes the code more readable.PHP Code:switch($option)
{
case "create":
{
if(isset($_POST['createticket']))
{
tickets::createticket();
}
else
{
tickets::ticketform();
}
break;
}
case "check":
{
if($option2 == "selected")
{
tickets::checkticket($tid, "tickets");
}
else
{
tickets::checkticket(0, "tickets");
}
break;
}
...and so on.
@Splitter
It wouldn't do much of anything for him IMO, and if/elseif/else is quicker than switch statements.
@zkemppel
I made it much more readable for you.
PHP Code:<?php
function makeNumerical($input)
{
return preg_replace("/[^0-9]/","", $input);
}
function makeAlphanumerical($input)
{
return preg_replace("/[^A-Za-z0-9]/","", $input);
}
$tid = makeNumerical($_POST['tid']);
$phone1 = makeNumerical($_POST['phone']);
$backup = makeNumerical($_POST['backup']);
$virus = makeNumerical($_POST['virus']);
$install = makeNumerical($_POST['install'])
$brand = makeAlphanumerical($_POST['brand']);
$serial = makeAlphanumerical($_POST['serial']);
$fname = makeAlphanumerical($_POST['fname']);
$lname = makeAlphanumerical($_POST['lname']);
$serial = makeAlphanumerical($_POST['serial']);;
$comments = mysql_real_escape_string(htmlentities($_POST['comments'])); // NOTE: I added htmlentities here to avoid HTML/XSS injection
$email = $_POST['email'];
if (! filter_var($email, FILTER_VALIDATE_EMAIL)) {
$email = makeAlphanumerical($email);
}
?>
You need to learn to separate the PHP from HTML better. Even if you can't wrap your mind around a simple template class, then at least take them out of the echo. Echo's shouldn't be big HTML blocks like that. Just keep it out of the PHP tags.
I recommend you learn better code management and organization. Looking into design patterns could prove very beneficial for you.
Also, I would recommend trying to start conforming to PEAR Standards.
s-p-n brought it to my attention recently, and, although my coding style was similar to PEAR standards, reading up on it and doing my best to practice it at all times really has made my code more readable for me in the now, as well as when I go back to look at it a few days later, and especially for other developers.
You obviously don't have to follow all the rules and standards, but some of the basics are well thought out and just plain look nice and make writing PHP less confusing at times.
Just by taking using the PEAR Control Standards(i.e. if/elseif/else statement, switch statement, etc.. standards), you can lower the amount of lines in your code, and make it more readable.
This is the type of feedback I've been looking for, thank you. I have no formal training in PHP which is probably why it's roughly put together.Quote:
Originally Posted by timebomb
Ok, so it seems that timebomb has covered most of my points already, but here is what else I have:
-----------
The first thing I noticed after logging in is that the code to check is only 4 digits long and all numeric - this is a problem!
After entering 0069 (I think) I was shown the following:
This should never be accessible, except by the person who the details belong to. You do not need to include any of this data in the external page, where anyone who can guess/brute force the input box can "farm" personal details.Code:Name: john smith
Phone #: (123) 456-7890
Email: test@test.com
-----------
I feel you are not seperating the admin functionality from the user functionality enough.
I mean, you have all these IF statements to validate staff, when really, you could just make a user.php and a staff.php and lock down the staff page, preferably by .htaccess
-----------
I also think you have too many classes in seperate files, but maybe that is because I have my own framework that I work within now that only requires me to have a database class and an authorization class. You seem to have classes in numerous pages. I would definitely merge some of those, based on their logic and downsize the file count.
-----------
You should also consider having a single included file, with all other includes put into that.
Right now, this is no problem for you due to the site size, but it is good practice to get into because one day, should you run a big site and include files on individual pages, you only have to remove an include from one file instead of multiple.
Sure, you could just empty data from the file and leave the file there, but in a commercial environment, that is not acceptable IMO.
-----------
Hope my input helps you, if only a little.
Regards,
Luke