Очистка PHP-кода

Question

Я заметил, что я очень неряшливый кодер и делаю что-то необычное.

Можете ли вы взглянуть на мой код и дать мне несколько советов о том, как сделать код более эффективным? Что я могу сделать, чтобы улучшить?

session_start(); 



/*check if the token is correct*/ 
if ($_SESSION['token'] == $_GET['custom1']){ 

    /*connect to db*/ 
    mysql_connect('localhost','x','x') or die(mysql_error()); 
    mysql_select_db('x'); 



    /*get data*/ 

    $orderid = mysql_real_escape_string($_GET['order_id']); 
    $amount = mysql_real_escape_string($_GET['amount']); 
    $product = mysql_real_escape_string($_GET['product1Name']); 
    $cc = mysql_real_escape_string($_GET['Credit_Card_Number']); 
    $length = strlen($cc); 
    $last = 4; 
    $start = $length - $last; 
    $last4 = substr($cc, $start, $last); 
    $ipaddress = mysql_real_escape_string($_GET['ipAddress']); 
    $accountid = $_SESSION['user_id']; 
    $credits = mysql_real_escape_string($_GET['custom3']); 




    /*insert history into db*/ 
    mysql_query("INSERT into billinghistory (orderid, price, description, credits, last4, orderip, accountid) VALUES ('$orderid', '$amount', '$product', '$credits', '$last4', '$ipaddress', '$accountid')"); 
    /*add the credits to the users account*/ 
    mysql_query("UPDATE accounts SET credits = credits + $credits WHERE user_id = '$accountid'"); 

    /*redirect is successful*/ 
    header("location: index.php?x=1"); 
}else{ 

    /*something messed up*/ 
    header("location: error.php"); 
} 

Answer 1

Возможно, вы захотите изучить использование фреймворка или, по крайней мере, объектно-реляционного картографа.

Это действительно упрощает операции SQL, если вы используете объектно-ориентированный подход.

Например, с моей картой для создания нового пользователя требуется только несколько строк кода, после того как они заполнили регистрационную форму. Конечно, за объектом скрывается множество кода, чтобы предотвратить любые забавные дела или попытки взлома, но с ним очень просто работать.

$account = new Account(); 
$account->insert(array(
    'userid' => $id, 
    'accountpass' => $passhash, 
    'accountemail' => $emailhash 
)); 

Я не использовал никаких известных структуры или объектно-реляционные картограф, но я слышал, что CodeIgniter, Kohana, и доктрина несколько хороших. Однако у них есть кривая обучения, поэтому вы должны знать заранее, если ваше приложение достаточно велико, чтобы гарантировать изучение другого API.

Answer 2

Ваш код выглядит довольно стандартным. Вероятно, вы должны использовать отступы какого-то рода, а для запросов MySQL используйте prepared statements вместо того, чтобы просто включать переменные в середине запроса. С точки зрения стиля это в основном отлично, но

Answer 3

Зависит от того, что вы подразумеваете под «чистым» здесь. Все это принесет огромную пользу от базового отступа - в значительной степени, перемещая существующие пробелы туда, где это более продуктивно. (Ваши разрывы строк кажутся довольно непоследовательно распределенными для одного.) Отступы и согласованность строк значительно повышают читаемость кода.

На следующем уровне стиля, я вижу, почему вам нужно назначить тонну переменных там, учитывая, что вы работаете с необработанными функциями MySQL. Если вы планируете придерживаться этой системы, по крайней мере, назначьте переменные в разумном порядке, не говоря уже о том, что я сделал несколько других изменений и примечаний ниже.

// Let's just strip all non-numerics from the card number so that we can validate it. 
$cc = preg_replace('/[^0-9]/', '', $_GET['Credit_Card_Number']); 
if(strlen($cc) != 12) { 
    // The card number is invalid; go back to form and try again. 
} 
$last4 = substr($cc, -4); // A negative start value is cleaner. 

$accountid = $_SESSION['user_id']; 

$amount = mysql_real_escape_string($_GET['amount']); 
$credits = mysql_real_escape_string($_GET['custom3']); 
// note: don't trust that users won't change the ipAddress parameter before submitting 
$ipaddress = mysql_real_escape_string($_GET['ipAddress']); 
$orderid = (int) $_GET['order_id']; 
$product = mysql_real_escape_string($_GET['product1Name']); 

Я разбил назначения переменных на три блока. Первый - для преобразования данных, второй - для извлечения данных из источников, отличных от GET, а третий - базовое назначение из $_GET переменных, отсортированных в алфавитном порядке. Назначения теперь гораздо более структурированы, и легко найти любую данную строку. (Это, конечно, ни в коем случае не является стандартным способом взлома, это то, что имело смысл здесь.)

Это все, что имеет значение для хорошего кода, что вам легко поддерживать. Сделайте все, что лучше всего подходит для вас, но я подозреваю, что в конечном итоге читаемость кода и упорядоченные назначения переменных хорошо работают.

Кроме того, как уже упоминалось, рассмотрите возможность использования чего-то типа PDO для операций с базой данных, что позволяет использовать подготовленные инструкции и избегать всех этих функций экранирования. Вы также можете посмотреть в ORM, например Doctrine, или, возможно, полноценный framework. Использование более мощных инструментов имеет более высокую кривую обучения, но также приводит к повышению производительности после овладения ООП.

Answer 4

Передача номера кредитной карты и IP-адреса с помощью GET не очень безопасна, URL-адрес, содержащий строку GET, может быть сохранен в истории браузера. Как вы назначаете $_GET['ipAddress']?Вместо этого вы должны использовать $_SERVER['REMOTE_ADDR'].

Чтобы получить последние 4 цифры кредитной карты, вы можете сэкономить несколько строк кода, выполнив:

$last4 = substr($cc,-4);

Answer 5

Первое, что выскакивает у меня есть, что есть довольно большой в вашем коде. Переставляя переменные $ _GET непосредственно в SQL-запросы, вы оставляете себя открытым для широкого спектра атак. Кроме того, поскольку, как представляется, использование данных для биллинга, можно манипулировать собственным балансом своих счетов, играя с переменными get.

Я бы порекомендовал некоторые проверки данных о входящих данных GET перед тем, как отправить их в базу данных.

Answer 6

Я бы добавил, что каждый раз, когда вы видите, что повторяете одно и то же действие, вы можете создать функцию.

Если завтра вы хотите изменить способ выхода из своих данных, вам придется редактировать все строки, в которых вы используете, например, mysql_real_escape_string.

почему бы не добавить функции, такие как

function escape_data($data) { 
    $escaped_data = mysql_real_escape_string($data); 
    // room for more actions ... 
    return ($escaped_data); 
} 

также, если вы применяете ту же функцию для всех членов массива (здесь $ _GET), вы могли бы найти array_map() полезно:

$_GET = array_map('escape_data', $_GET);