Код для критики.

proWoke · 3 Дек 2011

Посмотрите 2 моих кода. И скажите, что не то в них.

1 Код - это регистрация.

PHP:

session_start();
$server = getenv("DOCUMENT_ROOT");
include($server."/drafts/registration/system/connect_db.php"); //Подключаем файл соединения с базой данных
define("default_avatar","defavatar.gif");

$imgDir = "img";
@mkdir($imgDir, 0777); // Создаём папку для хранения аватарок

if (isset($_POST['send'])) {

// Сначало проверяем все поля на корректность введённых данных.
	if (empty($_POST['login']) or empty($_POST['email']) or empty($_POST['password']) or empty($_POST['repassword'])) {
		$_SESSION['message'] = "Пожалуйста, заполните все поля.";
		header("Location: index.php");
		exit();
	}
	if (empty($_POST['race'])) {
		$_SESSION['message'] = "Вы не выбрали рассу.";
		header("Location: index.php");
		exit();
	}
	
	if ((mb_strlen($_POST['password'],"UTF-8") < 6) or (mb_strlen($_POST['repassword'],"UTF-8") < 6)) {
		$_SESSION['message'] = "Пароль не может быть меньше 6 символов.";
		header("Location: index.php");
		exit();
	}
	
	if ((mb_strlen($_POST['password'],"UTF-8") > 21) or (mb_strlen($_POST['repassword'],"UTF-8") > 21)) {
		$_SESSION['message'] = "Пароль не может быть больше 20 символов.";
		header("Location: index.php");
		exit();
	}

	if (!preg_match("/(\S)@([a-z0-9.]+)/is",$_POST['email'])) {
		$_SESSION['message'] = "Введите корректный email.";
		header("Location: index.php");
		exit();
	}

	if (!preg_match("/^[a-z0-9а-яё_]+$/uis",$_POST['login']) or (mb_strlen($_POST['login'],"UTF-8") > 15)) {
		$_SESSION['message'] = "В имени не может быть больше 15 символов, а также различных знаков, кроме символа \"_\". ";
		header("Location: index.php");
		exit();
	}

//Рассовываем данные по переменным
$login = trim($_POST['login']);
$mail = trim($_POST['email']);
$avatar = $_FILES['avatar'];
$birthday = trim($_POST['year'])."-".trim($_POST['month'])."-".trim($_POST['day']);
$password = trim($_POST['password']);
$repassword = trim($_POST['repassword']);
$code = trim($_POST['code']);
$race = $_POST['race'];

if (preg_match("/^[0-9]+$/is",$_POST['password']) or preg_match("/^[a-zA-Zа-яА-Я]+$/is",$_POST['password']) or preg_match("/^[\W]+$/is",$_POST['password'])) { // Проверка на сложность пароля.
	$_SESSION['message'] = "Пароль не может содержать только цифры или только буквы. Пароль должен содержать и цифры и буквы.";
	header("Location: index.php");
	exit();
}

if ($password != $repassword) { // Проверяем на равенство паролей
	$_SESSION['message'] = "Пароли должны быть равными ";
	header("Location: index.php");
	exit();
}

if ($code != $_SESSION['captcha']) { // Проверяем на равенство паролей
	$_SESSION['message'] = "Капча введена неверно.";
	header("Location: index.php");
	exit();
}

//Создаём таблицу
mysql_query("CREATE TABLE IF NOT EXISTS `users` (
	id INT(8) AUTO_INCREMENT PRIMARY KEY,
	login VARCHAR(20),
	normalimg VARCHAR(255),
	miniimg VARCHAR(255),
	mail VARCHAR(20),
	birthday DATE,
	password VARCHAR(40),
	race VARCHAR(20),
	activation INT(2),
	count_message INT(10),
	time DATETIME )");

// проверка на существование пользователя с таким же логином
$result = mysql_query("SELECT id FROM `users` WHERE login='$login'");
$myrow = mysql_fetch_array($result);

if (!empty($myrow['id'])) {
	$_SESSION['message'] = "Извините, но такой пользователь уже существует.";
	header("Location: index.php");
	exit();
}

$filename = default_avatar;

if ($avatar['error'] == 4) {
	$avatar = default_avatar; // Если пользователь не выбрал файл, то ставим стандартный.
} elseif ($avatar['error'] == 0) {
 $tmp = $avatar['tmp_name'];

 if (file_exists($tmp)) {
   $info = getimagesize($tmp);
	if (preg_match("{image/(.*)}is", $info['mime'], $pocket)) {
		@mkdir($imgDir."/mini", 0777);
		@mkdir($imgDir."/normal", 0777);
		$time_for_name = time();
	
	 	$filename = $imgDir."/normal/".$time_for_name.".".$pocket[1];
		move_uploaded_file($tmp, $filename); // Большую картинку просто загружаем.

// Делаем ресайз большой картинки. Вырезаем в пропорции маленький квадратик.
$w = 90;

	if(preg_match('/[.](GIF)|(gif)$/', $filename)) {
	$im = imagecreatefromgif($filename); 
	}
	if(preg_match('/[.](PNG)|(png)$/', $filename)) {
	$im = imagecreatefrompng($filename);
	}
	
	if(preg_match('/[.](JPG)|(jpg)|(jpeg)|(JPEG)$/', $filename)) {
		$im = imagecreatefromjpeg($filename);
	}

$idimg = imagecreatetruecolor($w,$w);

$w_src = imagesx($im); //вычисляем ширину
$h_src = imagesy($im); //вычисляем высоту изображения

         // вырезаем квадратную серединку по x, если фото горизонтальное 
         if ($w_src>$h_src) 
         imagecopyresampled($idimg, $im, 0, 0,
                          round((max($w_src,$h_src)-min($w_src,$h_src))/2),
                          0, $w, $w, min($w_src,$h_src), min($w_src,$h_src)); 

         // вырезаем квадратную верхушку по y, 
         // если фото вертикальное (хотя можно тоже серединку) 
         if ($w_src<$h_src) 
         imagecopyresampled($idimg, $im, 0, 0, 0, 0, $w, $w,
                          min($w_src,$h_src), min($w_src,$h_src)); 

         // квадратная картинка масштабируется без вырезок 
         if ($w_src==$h_src) 
         imagecopyresampled($idimg, $im, 0, 0, 0, 0, $w, $w, $w_src, $w_src); 
		 
imagejpeg($idimg, $imgDir."/mini/".$time_for_name.".jpg");

$avatar = $imgDir."/mini/".$time_for_name.".jpg"; //заносим в переменную путь до аватара.

	} else {
		$_SESSION['message'] = "Файл может быть только изображением.";
		header("Location: index.php");
		exit();
	}	
 }

} // Закрывает условие на проверку наличия фотографии

// Подготавливаем и сохраняем данные в базе
$login = mysql_escape_string($login);
$mail = mysql_escape_string($mail);
$birthday = mysql_escape_string($birthday);
$password = mysql_escape_string($password);
$race = mysql_escape_string($race);

$login = htmlspecialchars($login);

$password = md5($password);  //шифруем пароль
$password = strrev($password);  
$password = $password."afr54";

$path_to_mini = "/drafts/guestbook/registration/".$avatar;
$path_to_normal = "/drafts/guestbook/registration/".$filename;

$insert_query = mysql_query("INSERT INTO users (login,normalimg,miniimg,mail,birthday,password,race,activation,count_message,time) VALUES('$login','$path_to_normal','$path_to_mini','$mail','$birthday','$password','$race','0','0',NOW())");

if ($insert_query) {
	$result3 = mysql_query ("SELECT id FROM users WHERE login='$login'",$db);
	$myrow3 = mysql_fetch_array($result3);
	$activation = md5($myrow3['id']).md5($login);

	$subject = "Подтверждение регистрации";
	$message = "Здравствуйте! Спасибо за регистрацию на sitename.ru\nВаш логин: ".$login."\n
	Перейдите по ссылке, чтобы активировать ваш аккаунт:\nhttp://localhost/drafts/guestbook/registration/activation.php?login=".$login."&code=".$activation."\nС уважением,\n
	Администрация sitename.ru";
	mail($email, $subject, $message, "Content-type:text/plane; Charset=utf-8\r\n");


	$_SESSION['message'] = "Спасибо за регистрацию. На вашу почту выслана ссылка для подтверждения регистрации.";
	header("Location: index.php");
	exit();
	}
}

2. Загрузка фотографий в альбоме

PHP:

<?php
session_start();
$server = getenv("DOCUMENT_ROOT");
include($server."/drafts/album/system/connect_db.php");
include($server."/drafts/album/system/lib.php");

$query_category = mysql_query("SELECT `id`,`name` FROM `category`");
for ($categories = array(); $row = mysql_fetch_assoc($query_category); $categories[] = $row);

if (isset($_POST['submit'])) {

list($category_id, $category_name) = explode(".", $_POST['category']);
$flagempty = false; // Флаг, которому будет присваиваться true, если хотя бы один файл был выбран пользователем.
if (empty($category_name)) {
	$_SESSION['message'] = "Не выбрана галерея.";
	header("Location: index.php");
	exit();
}


foreach ($_FILES as $key=>$value)
{
	if ($value['error'] != 4 ) 
	{
mysql_query("CREATE TABLE IF NOT EXISTS `category` (
	id INT(8) AUTO_INCREMENT PRIMARY KEY,
	miniimg VARCHAR(40),
	bigiimg VARCHAR(40),
	category VARCHAR(40),
	description VARCHAR(40))");

$flagempty = true;

$uploadedfile = $value['tmp_name'];

$filename = getRandomName().".jpg"; // Имя файла.
$number = substr($key,-1,1);
$description = $_POST["desc".$number]; // Переменная с текстовым описанием файла.

if (empty($description)) {
	$description = "без описания"; 
}

$description = mysql_escape_string($description);
$description = htmlspecialchars($description);

$absolute_to_mini = "$server/drafts/album/photos/$category_name/mini/$filename";
$absolute_to_big = "$server/drafts/album/photos/$category_name/big/$filename";

$relative_to_mini = "/drafts/album/photos/$category_name/mini/$filename";
$relative_to_big =  "/drafts/album/photos/$category_name/big/$filename";

// Загружаем мини фотографию
		$thumb = new Imagick($uploadedfile);

		$height=$thumb->getImageHeight(); 
		
		$width=$thumb->getImageWidth();
 
		if ($height < $width)
		{ 
		 $thumb->resizeImage(150,199,Imagick::FILTER_LANCZOS,1);
 		}
		else
		{ 
		 $thumb->resizeImage(150,199,Imagick::FILTER_LANCZOS,1);
 		}

		$thumb->writeImage($absolute_to_mini);
		$thumb->destroy(); 

move_uploaded_file($uploadedfile, "$absolute_to_big"); // Загрузка оригинала

mysql_query("INSERT INTO `photo` (miniimg,bigimg,category,description) VALUES ('$relative_to_mini','$relative_to_big','$category_id','$description')");
	$_SESSION['current_category'] = $category_id; // Переменная с id каталога, в который мы загружаем фотографии, для создания select в option.
	}
}

if (!$flagempty) {
	$_SESSION['message'] = "Невозможно загрузить файл.";
	header("Location: index.php");
	exit();
}
if ($flagempty) {
	
	$_SESSION['message'] = "Файл удачно загружен.";
	header("Location: index.php");
	exit();
}

}

Ragazzo · 3 Дек 2011

<sarcasm>Это еще и работает?

</sarcasm>

proWoke · 3 Дек 2011

В чём косяк?

Фанат · 3 Дек 2011

огромное количество дублированного кода. от локейшенов с екзитами в глазах рябит.
"рассовывание" по переменным висит почему-то посередине валидации, непонятно к чему.
зачем проверять на длину повторный пароль?
зачем ограничивать длину пароля?
SQLинъекция
расширение mysql объявлено устаревшим и более не поддерживается. надо переползать на mysqli
две строчки после MD5 - какой-то адовый треш.
mysql_insert_id()
дальше устал

proWoke · 3 Дек 2011

Эти 2 строчки я увидел в примере на сайте russeler. А что надо было делать с location and exit? В функцию выносить их отдельную? Да и где мне взять примеры хорошего кода на php и Javascript?

Фанат · 3 Дек 2011

например, после всех проверок можно ппосмотреть, есть ли что-то в $_SESSION['message'], и если есть - то делать редирект. один

proWoke · 3 Дек 2011

Ух, и вправду. Так где можно посмотреть пример хороших кодов на php. Желательно не ООП, ООП я пока не воспринимаю, я только книгу читаю ща по нему.

KorP · 3 Дек 2011

всё читать было лень, но что бросилось в глаза в начале:
что в скрипте регистрации делает создание папки для аватара и таблицы под юзеров? 0_0
edit: fail fix

proWoke · 3 Дек 2011

KorP написал(а):
всё читать было лень, но что бросилось в глаза в начале:
что в скрипте регистрации делает создание папки для аватара и таблицы под юзеров? 0_0
ну и ты наличие $_FILES не проверяешь, что выдаст $avatar = $_FILES['avatar']; если аватар не загружен?

Там потом идёт проверка файла. А где должно быть создание папки и таблицы?

KorP · 4 Дек 2011

proWoke написал(а):
Там потом идёт проверка файла. А где должно быть создание папки и таблицы?

твоё "потом" мало волнует php и скрипт
таблица и папка должна создавать ДО работы скрипта (ну во время установки-развёртывания), а не пытаться создаться во время его выполнения при каждом обращении

proWoke · 4 Дек 2011

if ($avatar['error'] == 4) {
$avatar = default_avatar; // Если пользователь не выбрал файл, то ставим стандартный.
} elseif ($avatar['error'] == 0) {
$tmp = $avatar['tmp_name'];

if (file_exists($tmp)) {
$info = getimagesize($tmp);
if (preg_match("{image/(.*)}is", $info['mime'], $pocket)) {
@mkdir($imgDir."/mini", 0777);
@mkdir($imgDir."/normal", 0777);
$time_for_name = time();

$filename = $imgDir."/normal/".$time_for_name.".".$pocket[1];
move_uploaded_file($tmp, $filename); // Большую картинку просто загружаем.

Вот идёт проверка. Если файла нет, ставится стандартная ава.

proWoke · 4 Дек 2011

avatar['error'] == 4

Если он не загружен, то в массив будет элемент error со значением 4.

Ragazzo · 4 Дек 2011

proWoke
Вообще неплохо бы выкинуть этот код, прочитать стоящую книгу по PHP5, в которой обсуждены уже многие вопросы, которые здесь тебе указали, а потом сделать тестовое какое-либо задание, например, вот, иначе дальше все будет только печальнее.

proWoke · 4 Дек 2011

Наоборот надо много писать и писать и читать чужой код. Я уже прочитал котерова и фленагана, толку 0. А вот опыта написания у меня небольшой. Посоветуйте лучше где можно найти примеры грамотного php кода. Как ООП, так и не ООП.

berkut · 4 Дек 2011

убил-бы всех, кто юзает собаки!

proWoke · 4 Дек 2011

KorP написал(а):
с какой такой радости?
error 4 будет если файл по каким то причинам не загружен, а у тебя будет андефинед вэриэбл. я вродь ещё не до такой степени пьян

СЛушай. Вот проверь сам. Что будет в переменной, если никакой файл не будет загружен.

KorP · 4 Дек 2011

ещё тут углядел

PHP:
PHP:

if(preg_match('/[.](GIF)|(gif)$/', $filename))

а если у меня пнг с расширением гиф? или виндоюзер туда в имя зафигачил .jpg, а у него просто скрыто отображение расширения файла - что будет? у тебя есть более надёжная вещь - тип
про остальное вродь сказали

Redjik · 5 Дек 2011

Фанат написал(а):
расширение mysql объявлено устаревшим и более не поддерживается. надо переползать на mysqli

не нагуглил, можно ссылку, похоже я что-то очень неправильно понял, и не хочу задавать глупый вопрос =)

Зы - шопскрипт весь так написан, что уж теперь - пусть живет, но на ООП явно стоит взгялнуть =)
пхп в подлиннике хорош кстати

KorP · 5 Дек 2011

Иван Redjik Матвеев написал(а):
не нагуглил, можно ссылку

http://php.net/manual/en/function.mysql-db-query.php

Redjik · 5 Дек 2011

ну вот, опять пострадал от невнимательности своей, я подумал вообще любое обращение с синтаксисом начинающимся с mysql - depricated, в принципе не очень расстроился, ибо все через PDO делаю, но как то озадачило! =)

Код для критики.

Новичок

TDD interested

Новичок

oncle terrible

Новичок

oncle terrible

Новичок

Новичок

Новичок

Новичок

Новичок

Новичок

TDD interested

Новичок

Новичок

Новичок

Новичок

Джедай-мастер

Новичок

Джедай-мастер