https://github.com/nsdvw/file-sharing

Я успел проверить не 100% кода, а где-то 70%, но думаю, найденных ошибок и замечаний тебе вполне хватит для начала. Код определенно надо улучшать.

> https://github.com/nsdvw/file-sharing/blob/master/db.sql#L25
> CREATE TABLE comment (
Если ты делаешь древовидные комменты, то в таблице стоит сделать ссылку на родительский комментарий как минимум. Она поможет например при необходимости восстановить или перепроверить значения в materialized path. Да и позволит легко определять кто родитель. Ссылка должна быть сделана с внешним ключом разумеется.

LoginManager надо сделать нормальным, не статическим классом и синглетоном в Слиме. Нужные ему зависимости (маппер) передавать через конструктор. Копипасту, где ты получаешь отдельный объект PDO прямо в классе, надо убрать.

Форму логина, встроенную в шапку, надо переделать так, чтобы с любой страницы она отправляла на отдельную страницу логина, где выводятся ошибки, если они есть (а если нет то после логина делается редирект на исходную страницу).

Тогда нам не надо в каждом действии передавать переменные  $loginError = '', $loginEmail = '', $loginPassword = '' и не надо их указывать в шаблоне, так как они всегда пустые.

У тебя очень много копипасты в index.php, программист должен не копировать код, а думать как этого избежать.

> if (LoginManager::login()) {
>        $login = true;
Вместо того чтобы это писать в каждом действии, лучше в самом начале передать объект LoginManager в смарти.

> $title = 'FileSharing &mdash; upload file';
Опять же, название по умолчанию можно передать один раз в начале index.php

>  $bookmark = 'Upload';
Чтобы это не копипастить, можно их передать через view->appendData один раз в начаел index.php.

> $app->post('/login',
> $app->get('/login',
Не вижу смысла делать тут 2 функции ради одной страницы. Лучше объединить их в одну, которая обрабатвает и POST и GET.

> $app->get('/logout',
так как логаут изменяет состояние системы, его надо делать через POST.Также, защити-ка его CSRF токеном, для работы с токенами стоит сделать конечно отдельный универсальный класс, который можно подключить к любой форме, с методами сгенерировать токен если надо, получить токен, проверить токен:  https://github.com/codedokode/pasta/blob/master/security/xsrf.md

> https://github.com/nsdvw/file-sharing/blob/master/public_html/index.php#L138
>   $uploadError = (isset($_GET['error']))
При ошибке ты не должен редиректить. Ведь в этом случае пользователь не сможет повторно отправлить файл на загрузку нажатием кнопки F5. Соответственно эта проверка тоже не нужна. При ошибке надо просто снова выводить форум загрузки. По этой причине удобно объединить вместе функции / и /upload_file

>   $file->size = ViewHelper::formatSize($file->size);
Это неправльно. Почему одно и то же поле хранит значение в разных форматах? Как в таком коде ориентироваться, когда непонятно в каком формате там лежит значение? Так быть не должно.

>  echo json_encode($file);
При отдаче JSON надо выставлять заголовок Content-Type: application/json. Также, ты не должен применять json_encode к объекту, ну сам подумай, это же не массив а объект, как его можно преобразовывать в JSON? Это чревато раскрытием каких-то данных которые  пользователь знать не должен, ведь твой код никак не проверяет какие поля можно отдать, а какие нельзя.

Этого быть не должно. Лучше сделать из класса массив только с перечисленными полями и его уже преобразовывать в JSON. Метод получения массива можно поместить в класс.

>   echo 'error';
Это неправильно, ты в одном случае отдаешь JSON, а в другом текстовую строку, как ты отличишь одно от другого? Как ты передаешь в чем именно ошибка? Логично при ошибке тоже отдавать JSON.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/index.php#L178
> if ($user->hash !== sha1($user->salt . $loginForm->password)) {
не надо копипастить по всему коду метод вычисления хеша, надо сделать отдельную функцию или метод.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/index.php#L181
> LoginManager::setSession(array('id'=>$user->id, 'hash'=>$user->hash,));
Метод SetSession неправильный, у LoginManager должен быть метод «залогинить пользователя», принимающий на вход например объект User, а не метод принимающий массив неизвестного вида. Не стоит заменять объект на массив. Метод може выглядеть так: $loginManager->autorizeUser($user);

> https://github.com/nsdvw/file-sharing/blob/master/public_html/index.php#L215
>  if (!LoginManager::isLoggedIn()) {
>            $author_id = null;
>        } else {
>            $author_id = $_COOKIE['id'];
>        }
Вся работа с авторизационными куками должна быть в классе LoginManager, а не размазана повсему коду. В данном случае логично у класса сделать метод getLoggedUser() который возвращает либо объект User либо null.

>  $request_method = (isset($_GET['ajax'])) ? 'ajax' : 'direct';
Тут лучше использовать переменную $isAjax с значениями true/false. А то возникает вопрос например почему тут не используются константы вместо строк.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/index.php#L224
>  $app->response->redirect("/?error=$error");
При ошибке не надо никуда редиректить, чтобы пользователь мог отправить форму повторно.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/index.php#L242
>     $app->response->redirect('/view/?upload=ok');
Ты при успешной загрузке редиректишь на страницу в пути к которой не указан id файла. А как пользователь тогда получит ссылку на файл? Логичнее редиректить его на страницу файла.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/index.php#L294
>   $user->fromForm($registerForm);
Это как-то не очень правильно, получается у нас модель пользователя должна знать о форме его редактирования. Это неправильно так как модель ничего не должна знать о внешнем мире и как она редактируется. Вместо этого в модели формы должен быть метод, возвращающий или заполняющий модель пользователя.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/index.php#L303
> $registerError = $registerForm->errorMessage;
>        $registerLogin = $registerForm->login;
>        $registerEmail = $registerForm->email;
Вместо того чтобы передавать переменные по отдельности, логичнее передать в смари саму форму.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/index.php#L415
>   $comment->level = Comment::getLevelFromPath($comment->materialized_path);
Вместо этого надо сделать метод getLevel()

> https://github.com/nsdvw/file-sharing/blob/master/public_html/index.php#L416
>   $comment->author_id = $app->userMapper->findById($comment->author_id);
запрашивать записи по одной неэффективно. Надо собрать id авторов, выбрать их одним запросом, присоединить к комментариям. И делать это надо не в index.php, а в маппере.

> $app->get('/view/:id',
> $app->post('/view/:id',
думаю, это надо объединить вместе в одну функцию.  В любом случае, копипастить огромный кусок кода как у тебя, неправильно.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/index.php#L471
>   if ($file->isImage()) {
>   } elseif ($file->isVideo()) {
тут же всего лишь выбор шаблона для отображения? Тогда это логичнее прямо в шаблоне и делать.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/js/lib.js#L76
> var previewEl = document.createElement('div');
Вот это тоже не очень удачный код. Фактически у тебя тут по частям создается кусок HTML кода. Но раз нам нужен HTMl код, почему мы его маскируем под JS и размазываем на 30 строк? Сделай в HTML файле шаблон окна превью:

<script id="tpl-preview" type="text/x-template">
<div class="a">{name}</div>
<a href="{href}"...
...
</script>

Тогда ты можешь в JS получить текст шаблона, сделать там подстановки функцией replace() и вставить в DOM. Что-то похожее описано например тут: https://learn.javascript.ru/templates

В общем HTML-код должен быть в HTML файле. При подстановке не забудь про экранирование спецсимволов а то получим этакий XSS.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/js/list.js#L3
> for (var i=0; i < files.length; i++) {
> files[i].addEventListener('click', function(){
не надо ставить 100 обработчиков. События всплывают, и достаточно поставить один обработчик на родительский элемент (это кстати разбирается в наших задачах на JS, советую порешать). Описано например тут: https://learn.javascript.ru/event-bubbling

> removeClass(files[i], 'file-selected');
>            addClass(files[i], 'file-nonselected');
Нет смысла делать 2 класса, достаточно один, для выделенного файла.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/js/list.js#L15
> removeClass(icons[k], 'file-icon-revert');
Зачем отдельный класс на иконку? Средствами CSS наверно можно ее выделить или что там надо сделать.

> currentChild.getAttribute('class').indexOf('file-icon') != -1)
Эта же проверка сработает на класс «some-file-iconic». Она неправильная. Также, ее надо вынести в отдельную функцию hasClass()

> https://github.com/nsdvw/file-sharing/blob/master/public_html/js/list.js#L28
> var xhr = getXmlHttp();
Отправку аякс-запроса не надо вписывать прямо в код (у тебя там и так гигансткая простыня вышла), надо сделать функцию такого типа:

getWithAjax({
    url: ...,
    dataType: 'ajax',
    success: function(result) {
        // получает уже разобранный ответ в случае успеха
    },
    error: function (xhr, errorText) {
        // вызывается при ошибке
    }
});

При работе с аяксом надо проверять HTTP-статус ответа, что он равен 200. Почитай например тут: https://ru.wikipedia.org/wiki/%D0%A1%D0%BF%D0%B8%D1%81%D0%BE%D0%BA_%D0%BA%D0%BE%D0%B4%D0%BE%D0%B2_%D1%81%D0%BE%D1%81%D1%82%D0%BE%D1%8F%D0%BD%D0%B8%D1%8F_HTTP

При работе с аяксом надо обрабатывать ошибки и давать пользователю возможность отправить запрос повторно.

Также, я не уверен, а нужен ли тут аякс и не проще ли JSON с информацией о файлах внедрить прямо в страницу?

> https://github.com/nsdvw/file-sharing/blob/master/public_html/js/main.js#L1
> window.onload = function () {
Использовать onload плохая идея так как она ждет загрузки всех ресурсов и картинок и обычно поздно срабатывает. А достаточно хоть одной картинке тормозить при загрузке (например сторонний счетчик какой-нибудь) и она сработает с большой задержкой. Пользователь может уже файл успеет выбрать.

В данном случае лучше явно вызвать прямо из шаблона нужную фунцкию:

<script>
doSomething();
</script>

Или к кнопке приписать в коде <button onclick="doSomething(event)">. По моему это и проще и удобнее и работает с момента появления кнопки на странице.

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

Отправку файла методом POST через аякс стоит сделать отдельной функцией. Вывыод ошиьки отдельной функцией. Проверку поддержки нужных функций в браузере отдельной функцией. Увеличение прогресса отдельной функцией.

Вообще, лучше отучайся писать простыни — их тяжело читать и неудобно править.

> https://github.com/nsdvw/file-sharing/blob/master/public_html/js/main.js#L45
>   var fullWidth = +(progressBox.offsetWidth);
Это ненадежно так как верстальщик может например добавить паддинг и алгоримт сломается. Вместо этого лучше воспользоваться возможностью задавать ширину в процентах.

Также, в HTML5 есть элемент для отображения прогресса, может лучше его использовать?

Если у тебя jPlayer это отдельная библиотека, лучше наверно не смешивать ее с твоими файлами, а положить в отдельную папку как есть.

> https://github.com/nsdvw/file-sharing/blob/master/views/header.tpl#L8
>  src="http://code.jquery.com/jquery-1.11.1.min.js"></script>
не подключай скрипты с внешних источников когда можно скопировать файл к себе, иначе твой сайт перестанет открываться при их падении/зависании. Ты можешь увидеть такое в туториалах, но там это делают ради экономии времени, и чтобы не объяснять какой файл куда скопировать. Но у тебя же не тот случай.

> https://github.com/nsdvw/file-sharing/blob/master/views/register_form.tpl
Нет escape

> https://github.com/nsdvw/file-sharing/blob/master/views/register_form.tpl#L6
> <div id="initial-letter">f</div>
>        <div id="logo-text">ile-sharing</div>
Это не нужно, делать отдельный див, так как в CSS есть псевдокласс :first-letter.

> https://github.com/nsdvw/file-sharing/blob/master/views/register_form.tpl#L26
> <div class="register-field-input">
>  <div>
А нужен ли тут лишний пустой див?


> https://github.com/nsdvw/file-sharing/blob/master/views/video_player.tpl
> https://github.com/nsdvw/file-sharing/blob/master/views/audio_player.tpl
Эти файлы явно сделаны методом копипасты. от нее надо избавиться.

> https://github.com/nsdvw/file-sharing/blob/master/app/Helper/Pager.php
В пейджере не должно быть SQL запросов, вся работа с базой должна быть  в мапперах. Боее того, там это и не надо, задача пагинатора рассчитывать номера страниц, а число страниц вполне можно передать снаружи.