1

Тема: Будь ласка , оцініть якість коду.

Привіт усім. Попрошу сьогодні розглянути декілька моїх випадкових файлів з кодом та оцінити його.

http://pastebin.com/85nsgFE2
http://pastebin.com/qaW1GmVa
http://pastebin.com/J4sTisjq

Подякували: 221VOLT, Monolith, ostap34PHP, funivan4

2

Re: Будь ласка , оцініть якість коду.

$user_password_hash = md5(md5($user_password));     //Хешуємо пароль

а смисл? де сіль?
також ймовірно краще використувати якісь інші алгоритми (на випадок зливу бази чи й коду -- трохи краще захистить паролі користувачів)

$check_verification_code = mysql_query("SELECT id, user_id, used FROM verification WHERE verification_code='$verify_code'");

2017й рік надворі - який mysql_* , люди?
юзайте mysqli_* !!!
тут http://replace.org.ua/post/93254/#p93254 я прикладів відсипав трохи, повторятись лінь

$user_password = trim($_POST["password"]);

тут крім трім-а я ще зробив би екранування html-символів чи перетворення їх в їхні коди
проте у вас далі йдуть регулярки, отже глянемо далі

if(!preg_match("/^([A-Za-z0-9_\.-]+)@([A-Za-z0-9_\.-]+)\.([A-Za-z\.]{2,6})/", $user_email) and !strlen($user_email) <= 50) {
                        $preg_match_errors[] = "Неправильний E-Mail!";
                    }

оце все є жахливим -- регулярки повільні, враховує далеко не всі валідні існуючі емейли і ...
простіше і правильніше робити провірку лише на @ (якщо виключаємо локалхост -- тоді ще можна додати провірку на крапочку)

if(!preg_match("/[A-Za-z0-9_]{5,50}$/", $user_password)) {
                        $preg_match_errors[] = "Неправильний пароль!";
                    }

знову ж таки, регулярки... + неправильний пароль -- це як? може бути невалідним по довжині -- мінімальний 8 символів наприклад, а максимальний -- 150 (все-одно які там символи і яка плюс мінус довжина -- ми ж його закодуємо)
відповідно можна провіряти лише на довжину(для любителів алгоритмів і повідомлень для користувача -- ще на складність, щоб вивести користувачу "чувак, твій пароль - лайно! його відгадає 2х річна дитина за хвилину!")

$user_nickname_check = mysql_query("SELECT id FROM users WHERE LOWER(nickname) LIKE '%".$user_nickname."%'");

пошук з LIKE працює повільно, можна вважати що це та ж регулярка (якщо ми не говоримо про індекси, та навіть і з ними - також)
не впевнений, що там варто приводити до нижнього регістру(здається в mysql пошук регістронезалежний? хоча про який тип поля ми говоримо зараз?) і пошук можна/потрібно зробити без викоритання LIKE

$user_create_query = mysql_query("INSERT INTO users SET nickname='$user_nickname', password='$password_hash', email='$user_email', rdate='$date'");

інколи все ж варто почитати стандарти, щоб у безхмарному майбутньому раптом нежданчиком хмари над головою не зібрались і не закапав дощ на голову у самому невідповідному місці у самий невідповідний час...
тобто не забувайте про зворотні лапки -- це колись вам збереже багато нервів, часу/грошей etc

строка запиту повинна виглядати так

"INSERT INTO `users` SET `nickname`='$user_nickname', `password`='$password_hash', `email`='$user_email', `rdate`='$date'"

або так

'INSERT INTO `users` SET `nickname`=\''.$user_nickname.'\', `password`=\''.$password_hash.'\', `email`=\''.$user_email.'\', `rdate`=\''.$date.'\''

загалом - непогано, трошки практики - і у вас буде красивіше і класніше з кожним днем!
успіхів!

p.s. деякі речі критикувати не став - то справа стилю, не відноситься до швидкодії чи безпеки (стосовно останнього - міг щось і пропустити, не претендую на істині в останній інстанції :) )

Подякували: Monolith, Betterthanyou, ostap34PHP, Cannoneer4

3 Востаннє редагувалося Cannoneer (28.01.2017 15:18:55)

Re: Будь ласка , оцініть якість коду.

СУПЕР! Дякую за таку обширну відповідь!
Постараюсь зробити усе в найкращому виді.

Також я маю до тебе питання: як ти розділяєш код і шаблон?

4 Востаннє редагувалося 221VOLT (28.01.2017 16:26:46)

Re: Будь ласка , оцініть якість коду.

кнопочка "дякую" знаходиться справа знизу повідомлення(форумні бали репутації -- фантики звісно, проте показують що повідомлення комусь здалось корисним - отже мотивує щось писати на цьому чудовому форумі)

роділення коду і шаблону?
запитання про розділення php та html ?
тут потрібно відповісти собі на запитання -- для чого це (мені) (взагалі) потрібно?

imho хороші статті -
http://www.phpinfo.su/articles/practice … v_php.html
http://phpfaq.ru/tech/tpl

процитую

Фантазий же главных две:
1. Шаблоны нужны "дизайнеру", чтобы он мог их править, не разбираясь в PHP.
2. Следовательно, шаблоны служат для отделения PHP от HTML.

Давайте попробуем задуматься над первым утверждением. Кто такой дизайнер? Это человек, который работает в фотошопе. HTML он чаще всего не знает вообще. А над шаблоном работает либо специальный верстальщик или - чаще всего... сам программист! Смешно, правда?
Теперь следствие, про отделение PHP от HTML. Отлично. Перед нами стоит святая цель отделить. Поэтому мы придумываем Смарти и пишем:
{foreach key=cid item=con from=$contacts}
    <a href="contact.php?contact_id={$cid}">{$con.name} - {$con.nick}</a><br />
{/foreach}
Ещё смешнее.
"Дизайнер", ради которого все затевалось, падает в обморок от счастья.

напевно тут варто застосувати "бритву Окками"

тобто у випадку фанатичного "все-все повністю розділити php від html" -- ми будемо мати зайві аналогічні конструкції -- ще одну мову

скоріше, хорошою практикою є відділення отримання-обробки даних від їх виводу -- MVC
немає нічого страшного, якщо десь в шаблоні ми пишемо вставку php <?=$message;?> для виводу

проте не варто змішувати в купу запити до бази даних з html...

можливо, я не найдосвідченіший php-програміст, проте у мене проект виглядає так (враховуючи що я не користуюсь движками і фреймворками -- самопис лише) --

папка conf -- туди кладу файл-підключення до бд, ще якісь файли з налаштуваннями
папка include -- туди складаю php файли з функціями
папка parts -- сюди -- шаблони, частини сторінок та щось таке інше
у корені - index.php та інші сторінки (у випадку якщо у нас односторінковий сайт-додаток, чи сайт з однієї точкою входу -- у корені лежить лише індекс, а у окремій папці - назовемо її pages -- деякі частини сторінок - тобто php-файли з кодом)

сама сторінка-файл php виглядає якось так --

<?php

/* маршрутизація -- визначаємо куди користувач "постукав" */


/* визначаємо-перевіряємо(частково це пункт нижче) чи має він права/привілегії бачити ту сторінку куди він постукав --
повертаємо сторінку(про це нижче) чи повідомлення/форму входу/переадресацію */


/* перевірка вхідних даних -- куки, сесії, гет-пост-параметри(частково це пункт вище),
валідації(ймовірно тут потрібні моментами запити до бд, про це нижче) */


/* запити до бази даних -- валідації-провірки вхідних даних, отримання контенту */


/* всі отримані дані(після необхідної обробки чи відсутності такої) ми передаємо в шаблон */

/* в шаблоні ловимо дані, показуємо (і так, в шаблоні можна використовувати цикли php -- у цьому нічого погано немає, як інколи можна і зробити необхідну перевірку) */

всі пункти можуть бути розбиті на один чи більше інклудів, різниця між деякими пунктами є досить розпливчасто-умовна і вони(пункти) тісно переплітаються

загалом якось отак сайт на php з tcp виглядає -- не залежно від того - чи самопис, чи движок, чи фреймворк...

про вебсокети на php нічого сказати не можу -- з ними я працюю з допомогою erlang/elixir зараз, на php - не працював з вебсокетами :)

Подякували: Cannoneer1

5

Re: Будь ласка , оцініть якість коду.

Cannoneer написав:

Також я маю до тебе питання: як ти розділяєш код і шаблон?

тут допоможуть чарівні букви  - MVC

Подякували: flatliner1

6

Re: Будь ласка , оцініть якість коду.

221VOLT написав:
$user_password_hash = md5(md5($user_password));     //Хешуємо пароль

а смисл? де сіль?
також ймовірно краще використувати якісь інші алгоритми (на випадок зливу бази чи й коду -- трохи краще захистить паролі користувачів)

$check_verification_code = mysql_query("SELECT id, user_id, used FROM verification WHERE verification_code='$verify_code'");

2017й рік надворі - який mysql_* , люди?
юзайте mysqli_* !!!
тут http://replace.org.ua/post/93254/#p93254 я прикладів відсипав трохи, повторятись лінь

$user_password = trim($_POST["password"]);

тут крім трім-а я ще зробив би екранування html-символів чи перетворення їх в їхні коди
проте у вас далі йдуть регулярки, отже глянемо далі

if(!preg_match("/^([A-Za-z0-9_\.-]+)@([A-Za-z0-9_\.-]+)\.([A-Za-z\.]{2,6})/", $user_email) and !strlen($user_email) <= 50) {
                        $preg_match_errors[] = "Неправильний E-Mail!";
                    }

оце все є жахливим -- регулярки повільні, враховує далеко не всі валідні існуючі емейли і ...
простіше і правильніше робити провірку лише на @ (якщо виключаємо локалхост -- тоді ще можна додати провірку на крапочку)

if(!preg_match("/[A-Za-z0-9_]{5,50}$/", $user_password)) {
                        $preg_match_errors[] = "Неправильний пароль!";
                    }

знову ж таки, регулярки... + неправильний пароль -- це як? може бути невалідним по довжині -- мінімальний 8 символів наприклад, а максимальний -- 150 (все-одно які там символи і яка плюс мінус довжина -- ми ж його закодуємо)
відповідно можна провіряти лише на довжину(для любителів алгоритмів і повідомлень для користувача -- ще на складність, щоб вивести користувачу "чувак, твій пароль - лайно! його відгадає 2х річна дитина за хвилину!")

$user_nickname_check = mysql_query("SELECT id FROM users WHERE LOWER(nickname) LIKE '%".$user_nickname."%'");

пошук з LIKE працює повільно, можна вважати що це та ж регулярка (якщо ми не говоримо про індекси, та навіть і з ними - також)
не впевнений, що там варто приводити до нижнього регістру(здається в mysql пошук регістронезалежний? хоча про який тип поля ми говоримо зараз?) і пошук можна/потрібно зробити без викоритання LIKE

$user_create_query = mysql_query("INSERT INTO users SET nickname='$user_nickname', password='$password_hash', email='$user_email', rdate='$date'");

інколи все ж варто почитати стандарти, щоб у безхмарному майбутньому раптом нежданчиком хмари над головою не зібрались і не закапав дощ на голову у самому невідповідному місці у самий невідповідний час...
тобто не забувайте про зворотні лапки -- це колись вам збереже багато нервів, часу/грошей etc

строка запиту повинна виглядати так

"INSERT INTO `users` SET `nickname`='$user_nickname', `password`='$password_hash', `email`='$user_email', `rdate`='$date'"

або так

'INSERT INTO `users` SET `nickname`=\''.$user_nickname.'\', `password`=\''.$password_hash.'\', `email`=\''.$user_email.'\', `rdate`=\''.$date.'\''

загалом - непогано, трошки практики - і у вас буде красивіше і класніше з кожним днем!
успіхів!

p.s. деякі речі критикувати не став - то справа стилю, не відноситься до швидкодії чи безпеки (стосовно останнього - міг щось і пропустити, не претендую на істині в останній інстанції :) )

2017й рік надворі - який mysql_* , люди?
юзайте mysqli_* !!!

навіть я перейшов цього року :D

Подякували: varkon1

7

Re: Будь ласка , оцініть якість коду.

2017й рік надворі - який mysql_* , люди?
юзайте mysqli_* !!!
навіть я перейшов цього року :D

Юзайте PDO навіть я перейшов )))

Тепер по якості коду:
Я буду оцінювати відносно свого рівня.
1.  Немає обєкту контроллера. Як ти будеш перевіряти свій код?
2. Використовуються глобальні змінні типу POST & SESSION. Як ти будеш тестувати свій код?
3. defined('_INCLUDE_') or die('Shit happens!');  Щось не так у твоїй архітектурі.
4. Sql injection. Якщо твій код десь уже працює - його можна поламати на раз два. $user_data = mysql_query("SELECT id, nickname, password, user_group, verified FROM users WHERE email='$user_email'")

В загальному на початок прийде. До речі якщо ти користуєшся phpstorm постав собі плагін Php Inspection EA https://plugins.jetbrains.com/plugin/76 … -extended-
Прожени інспецікї по проекту і він тобі підскаже що не так у твоєму коді)

Подякували: 221VOLT1

8

Re: Будь ласка , оцініть якість коду.

funivan написав:

1.  Немає обєкту контроллера. Як ти будеш перевіряти свій код?

??

9

Re: Будь ласка , оцініть якість коду.

@iovchynnikov можете пояснити запитання?

10

Re: Будь ласка , оцініть якість коду.

funivan написав:

@iovchynnikov можете пояснити запитання?

Не зрозумів як перевірка (що б це не було) пов'язана з існуванням контролера

11

Re: Будь ласка , оцініть якість коду.

Не зрозумів як перевірка (що б це не було) пов'язана з існуванням контролера

 
Тема низвається: Будь ласка , оцініть якість коду.
Для мене якість коду це можливість перевірити його на помилки. Можливість протестувати свій код. Відповідно для того що б його було зручно тестувати треба мати обєкт (в даному випадку це контроллер) Можете називати це як хочете,: контроллер, екшин або будь що інше. Я не побачив як можна написати unit test для цієї штуки. Відповідно і написав що я код не гуд. ;)

Подякували: 221VOLT1

12

Re: Будь ласка , оцініть якість коду.

funivan написав:

Не зрозумів як перевірка (що б це не було) пов'язана з існуванням контролера

 
Тема низвається: Будь ласка , оцініть якість коду.
Для мене якість коду це можливість перевірити його на помилки. Можливість протестувати свій код. Відповідно для того що б його було зручно тестувати треба мати обєкт (в даному випадку це контроллер) Можете називати це як хочете,: контроллер, екшин або будь що інше. Я не побачив як можна написати unit test для цієї штуки. Відповідно і написав що я код не гуд. ;)

Код дійсно далекий від ідеального, але будь ласка, використовуйте професійну термінологію більш коректно. Ви ж не на форум домогосподарок пишете. Чому б вам тоді не зробити зауважень про відсутність моделі? ;) Це теж до речі досить ускладнює написання ваших міфічних "unit tests". ;) Там багато чого не вистачає, окрім того, щоб взагалі піднімати питання про UT

Подякували: 221VOLT1

13

Re: Будь ласка , оцініть якість коду.

funivan написав:

Не зрозумів як перевірка (що б це не було) пов'язана з існуванням контролера

 
Тема низвається: Будь ласка , оцініть якість коду.
Для мене якість коду це можливість перевірити його на помилки. Можливість протестувати свій код. Відповідно для того що б його було зручно тестувати треба мати обєкт (в даному випадку це контроллер) Можете називати це як хочете,: контроллер, екшин або будь що інше. Я не побачив як можна написати unit test для цієї штуки. Відповідно і написав що я код не гуд. ;)

Отже, за Вашою логікою, функціональний код взагалі не тестується? А отже кожен, хто такий підхід використовує, апріорі має поганий код [такий, що не можливо оцінити?], чи що?

Подякували: 221VOLT, varkon2

14

Re: Будь ласка , оцініть якість коду.

Код дійсно далекий від ідеального, але будь ласка, використовуйте професійну термінологію більш коректно. Ви ж не на форум домогосподарок пишете.

Буду дуже вдячний якщо ви скажете мені які не професійні терміни я використовую, і в професійному світі як правильно написати ;)

Чому б вам тоді не зробити зауважень про відсутність моделі?

Вирішив все не описувати. Згідний, можна вказати.

Це теж до речі досить ускладнює написання ваших міфічних "unit tests". Там багато чого не вистачає, окрім того, щоб взагалі піднімати питання про UT

А чому міфічних? І чому моїх? Unit tests не є моїми і тим більше не є міфічними :)
Так багато чого не вистачає. Але якщо розглядати код із сторони "тестування" можна буде простіше розробити кращий код (Моя думка)

Отже, за Вашою логікою, функціональний код взагалі не тестується? А отже кожен, хто такий підхід використовує, апріорі має поганий код [такий, що не можливо оцінити?], чи що?

Ні) Я старався донести наступну думку - необхідно тестувати код. Я б зробив об'єкт і його перевіряв. Ви б зробили по іншому.
Висловив свою думку: якщо будуть об'єкти контроллера - буде простіше розробляти. Це моя думка.

15

Re: Будь ласка , оцініть якість коду.

funivan написав:

Код дійсно далекий від ідеального, але будь ласка, використовуйте професійну термінологію більш коректно. Ви ж не на форум домогосподарок пишете.

Буду дуже вдячний якщо ви скажете мені які не професійні терміни я використовую, і в професійному світі як правильно написати ;)

Ну написати можно було щось приблизно так:
"Так як автор не дотримується парадігми MVC (яка вимагає розділяти отримання, обробку та показ данних), його код важко сприймати, він є яскравим прикладом "вермешелевого коду" з усіма пов'язаними проблемами. Окрім того, код не покритий UT (або вони не показані) - що може свідчити про його низьку якість та велику кількість помилок". Це відповідає дійсності.
Порівняйте з вашим - " Немає обєкту контроллера. Як ти будеш перевіряти свій код? "

Подякували: 221VOLT1

16

Re: Будь ласка , оцініть якість коду.

@varkon ок) але у моїх реченнях немає нічого не технічного :)

17

Re: Будь ласка , оцініть якість коду.

funivan написав:

@varkon ок) але у моїх реченнях немає нічого не технічного :)

Так. Немає. Як і будь-якого сенсу :)