Разбор тестового задания frontend разработчика

Привет, участникам и гостям. Ниже текстовая версия разбора третьего тестового задания.

Так же есть видео версия разбора:

Если ошибка / хороший ход повторяются — в общем ревью это не указывается во второй, третий и так далее разы. Для некоторых моментов, я все же повторяюсь, это сделано либо по невнимательности, либо мне хочется заострить внимание на отличном решении / проблеме.

Общие проблемы

• Многие ошибки видны, если просто запустить демо, не открывая код. Невнимательность или «пофигизм» — навык джуниора. Чем чище ваще демо, тем лучше впечатление о вас.
• Ни одного теста. Стыд! Для кого я пишу такие подробные туториалы? (Раз, два)
• Приложение падает из-за отсутствия try/catch / ErrorBoundaries. Чаще всего, это было связано с ошибкой загрузки библиотеки для google sign in.
• Нет разделения на редьюсер одной новости и всех новостей
• Верстка не адаптивная (не учитывал, так как оформление не являлось обязательным условием)

Общие плюсы

• Работа с api вынесена отдельный модуль
• Присутствуют хелперы (общие функции), вынесенные отдельно
• Оформление
• Желание сделать больше чем было в ТЗ, либо использовать новые техники и подходы к архитектуре

Частые вопросы:

Как проверить автора новости?

Для сравнения авторства, если автор новости заходил через google:

• авторизоваться с помощью google sign in и получить google токен
• отправить токен на /api/v1/auth/google
• получить токен нашего приложения и декодировать его (для получения данных о пользователе)
• сравнить id (article.creator._id и id декодированный из токена гугла)

Regies Linkas

Commit

Собственные замечания в readme, не разбирал google jwt

Минусы:

У новости троеточие, даже если она короткая.

В просмотре новости, текст так же обрезается.

Сессия не сохраняется между рефрешами.

Подписка на весь store

Использование длинных названий переменных через ., более одного раза

Можно не биндить в конструкторе, при использовании стрелочных функциий

NewsItem грязноват, например: один раз проверено this.props.detail и потом еще доп.проверки, возможно недорефакторено

Опять лишняя проверка, если в cdm сделана проверка на новость, то нет смысла в шаблоне еще раз проверять это

let вместо const

Для методов delete & edit можно было бы заранее настроить axios заголовки, или вынести работу с API в отдельный модуль. (будет показано в следующей работе у Виталия Володенко)

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

Плюсы:

Есть proptypes.

Передача флага editable в тупой компонент

Использование сокращенной записи React.Fragment

Есть обработка ошибок, но нет уведомлений (о чем в readme признались)

Есть хелпер для даты

Vitaly Volovodenko

Master branch

Не сторонник «веселых» исполнений, но в данной работе стиль выдержан полностью, а не частично, поэтому отнесусь к этому нейтрально с юморком.

Образцовая работа

Минусы

Не найдено

Плюсы

Есть демо

Есть env.example файл

Бэк подпилен под свои нужды — можно было бы указать, под какиие.

Использование ErrorBoundary

Если новости отсутствуют

API для запросов + пример, когда запрос под api не подходит

Прокачанный хелпер для дат

Нейтрально

Можно было бы вынести только state.user и не углубляться ниже.

Так же, работа сильная, можно добавить использование селекторов.

Из письма:

С реакт работаю 6 месяцев, с JS — 10 месяцев, с TS — 2 месяца.
Выполнил бы еще и бонусное задание, но уже устроился на работу, поэтому будет некогда.

Rostislav Futornoy

Commit

.env файл не в .gitignore

Поехала верстка

Иконки не очень аккуратно сверстаны.

Минусы:

При удалении новости не спрашивается подтверждение. Всегда для удаления чего-либо — должно быть подтверждение, если заказчик не требует иного специально.

В reducers не используются константы, хотя они описаны.

Вместо того, чтобы добавлять новый флаг для условия в render, можно было взять курс на reducer новостей, и проверять флаг оттуда, но так как в reducer не хватает isFetching флага и ошибки — то решение со state допускается, хотя я бы лучше «прокачал» reducer.

NewsContainer подписан на user, но user не используется. Так же это потенциальная проблема для производительности, так как при изменении в user будет перериисовка и в NewsContainer.

Рекомендую использовать более подробную запись для массива с объектами. Подробнее на StackOverflow + документация

import PropTypes from 'prop-types';

ReactComponent.propTypes = {
arrayWithShape: PropTypes.arrayOf(PropTypes.shape({
color: PropTypes.string.isRequired,
fontSize: PropTypes.number.isRequired,
})).isRequired,
}

Нельзя сравнивать авторство по не уникальному атрибуту (в данном случае, по имени).

Плюсы:

Ошибки показываются клиенту

Сергей ZackFox

Commit

Возможно, когда работа была послана, этого предупреждения еще не было, но тем не менее, не забывайте проверять свои зависимости, с помощью npm audit

Минусы:

Нет иконок в карточке новости, на странице всех новостей.

Избыточное условие в проверке, компонент не рисуется без news.

Плюсы:

Создание новости с графическим редактором

Аккуратно в API описан signIn + использованы cookies. Наконец-то, сессия не сбрасывается!

Helper getDecodedUser — как раз, показано как можно вытащить id (отсылка к предыдущей работе), по которому потом сверять(?).

Обыденные hoc‘и, но хочу отметить их.

Передача actions в короткой записи

Кнопка «Назад».

Герман Топалов

Commit

Readme ок! Забыл лишь про env файл.

Методология Atomic

Минусы

В письме было указано, что времени на полировку не хватило, но ревью от этого «мягче» не стало, так как работа сильная.

У формы входа отсутствует валидация. Например, не «отключаются» (disabled атрибут) кнопки, если поля пустые. Хотя у добавления новости — валидация есть.

Нет возможности вернуться на главную страницу с формы входа

Если пользователь что-то ввел в новости, и жмет «отмена» — не спрашивается подтверждение.

Плюсы

Оформление работы!

Используется годная библиотека для уведомлений react-tostify

Подтверждение удаления новости присутствует.

Используется отдельный layout для некоторых роутов

Интересный HOC withFeedRemove

propTypes и defaultProps

Использование title для иконок (внимание к деталям!)

Использование extraArgument в redux-thunk, затем переменную api можно вызывать так:

(внимание на третий аргумент, документация)

Решение проблемы с инициализацией google api (библиотека будет точно загружена к моменту выполнения логина).

Любопытно

Сокращение T для PropTypes

Образцовая работа

Михаил Карпов

Commit

Есть демо и скриншот в readme.

Новости появляются с анимацией, есть анимация на иконках.

Чтобы запустить проект, нужно не только создать .env файл с ключом, но еще и заменить в config API_URL

Кнопка добавить новость — не очевидная.

Минусы

Редьюсер новостей и новости совмещен. Это две разные сущности, поэтому лучше их разбить на свои отдельные редьюсеры.

Плюсы

Решение с middleware по проверке токена + проверка токена в момент загрузки приложениия

404 страница

Ошибки переведены для пользователей

Наличие ui-компонентов. Но я бы добил эту затею, например, post_button состоит из компонента кнопка/ссылка, определенного цвета и текста. Либо вместо одного компонента «кнопка+ссылка», сделать два «кнопка» и «ссылка».

Любопытно

Хак(?) для редиректа после удаления новости

Нейтрально

Флажок статуса загрузки (loading) сделан непривычным для меня способом.

Александр Блажко

Commit

В решении используются immutable и саги. Подход — ducks.

Добавлены некоторые «плюшки», например — поиск новости по заголовку.

Минусы

Мертвый код в комментариях

Скорее минус, чем нейтрально: проверка истечения токена (так как приходится дублировать. На крайний случай, можно было бы вынести в хелпер повторяющуюся часть). Сократить количество кода в компонентах и не только, поможет иное решение, например проверка в middleware, как это сделано у Михаила.

Много «буков», если так можно выразиться. Например, для получения новости. Проблема уже известная — снова все новости и конкретная новость объединены в один редьюсер. Нужно разделить. Затем простой запрос за новостью в cdm и подписка на редьюсер новости в mapStateToProps.

Повторяющийся код (валидация, вытаскивание переменных и тд тп) — лучше выносить в хелперы. Например, повторение в строках 91-93 и 173-175 в ducks/auth.js

Так же про повторение и для селекторов. Можно было бы добавить это в отдельный файлик и не дублировать код. Но за использование селектора — плюсик.

Плюсы

Общий шаблон для создания/редактирования новости. Стоит отметить, что с ростом разницы между этими формами, может расти сложность шаблона и в какой-то момент, может быть выгоднее сделать 2 разных компонента.

Любопытно

Используется Record вместо привычного мне Map, может быть автор прояснит плюсы?

Итого: считаю, что после данного обзора у автора будут мысли для рефакторинга и работа станет меньше по объему символов и легче для чтения.

Ali Gasymov

Commit

Используется redux-api-middleware

Минусы

Лишний контейнер

Нет обработки request / errors

Адрес API не вынесен в константу

Опасные имена для сохранения в LS, которые с легкостью может перезаписать другое приложение. Рекомендую использовать уникальный префикс.

Плюсы

Данные нормализуются

в итоге удаление превращается в легкий delete по ключу

Json.stringify для более безопасного сохранения новости

Любопытно

Решение проблемы с «еще не загруженным» api для google sign in

Андрей Голушкин

Commit

Минусы

Warning при сборке

Warning: postcss-cssnext found a duplicate plugin (‘autoprefixer’) in your postcss plugins. This might be inefficient. You should remove ‘autoprefixer’ from your postcss plugin list since it’s already included by postcss-cssnext.

Кнопка «Войти» меняется на «Выйти», даже если ошибка получения токена (причем, уведомление с ошибкой показывается ОК)

В подробностях открывается не та новость. Вероятно из-за опечатки. С нормальным сравнением, работает.

cdm расположен ниже метода render. Это не ошибка, но большинство проектов пишутся опираясь на eslint-правило в котором указан порядок следования методов.

Много кода для перехода на редактирование новости, начинается через state. Нужно использовать Link из react-router

Плохое условие для показа кнопок редактирования/удаления

Токен вытащен напрямую, по документации нужно использовать:

googleUser.getAuthResponse().id_token

Плюсы

Компонент кнопки вынесен отдельно

Сергей Козлов

Commit

В работе используются саги и Ant design

Минусы

Проверка доступности редактирования/удаления новости сделана в самой новости. Компонент новости должен просто принимать флаг, например: editable — true/false. В данный момент, он знает больше чем положено.

[придираюсь] Иконка глаза не очевидна для редактирования

Плюсы

Уведомления

JSDoc дамы и господа!!!

Понятный текст ошибок

Работу можно рассматривать как хороший пример для работы с сагами.

Нейтрально

Шаблон новости (короткая, полная версия) в одном файле.

Роман Бабахин

Commit

Проект сделан с 💜

Минусы

Если параметров у функции много, лучше класть их внутрь объекта, чтобы не «запоминать» порядок.

Нет автоформатирования кода, или автор использует непривычные для меня настройки, например строки 25-40 в NewsContainer.

Неуместные операции в редьюсере. Редьюсер, только получает и выдает данные. Все остальное делаем в Action Creator (создателе действия). Преобразование данных вполне можно делать в редьюсере.

Bind в констукторе — устарело. Можно просто использовать стрелочные функции для методов.

В работе выполняется поиск по заголовку, но для поиска всегда необходимо использовать trim и toLowerCase методы.

Избыточная запись, можно указать LoginPage в вызове connect.

cwm «устарел» (deprecated). Куда вынести эти расчеты рекомендации нет, нужно обсудить «желание» автора и подумать.

переменная expires — const (eslint спасет от таких проблем) (данный файл, вообще, не используется в проекте)

Плюсы

Выполнен дополнительный функционал, дизайн приятный, разбита на множество компонентов… (и в целом работа неплохая), но выше уже были описаны схожие плюсы, поэтому не повторяюсь.

Нейтрально

NewsContainer с вложенными роутами, включая рендер роута через функцию

Вижу намек на cache — это здорово, но я не заметил как кэш сбрасывается(?)

Игорь Лученков

Commit

Минусы

Не все зависимости указаны в package.json

Не подчищен console.log

Вновь отмечу, что компонент пытается выяснить, можно ли показывать кнопку «редактирования» + ошибка, что выяснение идет не по уникальному свойству.

React.Fragment и <> на проекте. Хотелось бы в едином стиле.

Вероятно «старый код«, либо неясно назначение isEditing, который всегда true

Опечатка в названии функции (должно быть feedsReducer)

Поскольку ниже в плюсах отмечен Flow, хотелось бы большего покрытия. Как минимум, это обязательно (так как это просто) покрытие propTypes, ниже указана библиотека для этого.

Плюсы

Flow! Для компонентов, рекомендую использовать babel-plugin-flow-react-proptypes

Использование recompose отмечу как плюс. Хотя скорее всего, это привычный для автора способ. С одной стороны выглядит красиво и компоненты очень «тупые», с другой стороны — приходится бегать по файлам, чтобы подсмотреть примитивные обработчики.

Константы с namespace‘ом (область именования)

Работу предлагаю к рассмотрению как туториал по работе с recompose

Евгений Захаров

Commit

В Readme есть подробное описание. Частично цитирую:

Проект создан с помощью структуры react-modern — это моя реализация фрактальной структуры приложения. Это мой первый опыт написания приложений на основе этой структуры, так что удалось найти некоторые проблемы и баги в ней. Само собой следующие приложения, написанные с этой структурой, будут более доработанными и красивыми.

Минусы

О тестах вспомнил только на середине пути, так что не стал их писать.

Евгений, этот мотивирующий минус для тебя лично.

Можно ли обойтись без таймера? Считаю их ненадежными.

Плюсы

подстановка токена (но плохое имя у переменной в LS)

Работа с уведомлениями

Использование Styled-components

Нейтрально

Зачем во всех названиях call? Чтобы отличить, что это http-вызов?

Любопытно

Еще раз отмечу, подход react-modern. Кому интересно — welcome в работу Евгения.

Итого

• Виталий и Герман — самые сильные работы
• Посмотрите работу Сергея если хотите использовать redux-saga
• Посмотрите работу Игоря, если хотите использовать recompose

Посмотрите остальные работы и проанализируйте плюсы/минусы, указанные в review.

Спасибо за участие и до встречи в 2019м!