Code-review тестового задания junior react разработчиков

    Что это такое?


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


    выбрал реакт


    Видео

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


    • Плохое Readme;
    • Остаются eslint предупреждения, лишние console.log (redux-логгер не в счет);
    • Иконка Web не вынесен вперед (невнимательное чтение задачи);
    • Иконка Web выносится вперед в компоненте (а лучше бы в редьюсере, или экшене);
    • Пароль не очищается, если запрос вернулся с ошибкой;
    • Submit кнопка в форме логина доступна, если поля пустые (или одно из полей);
    • Submit кнопка в форме логина не поддерживает нажатие Enter;
    • Нет деления на компоненты/контейнеры (не относится к тем, кто делил по другим подходам);
    • URL-адрес для запросов на сервер полностью передается (нет оформления повторяющейся части строки в константу);
    • Ошибка/уведомление "неправильное имя пользователя/пароль" — не очищается;
    • Ошибка "неправильное имя пользователя/пароль" — выводится константой с сервера;
    • Текст ошибки "захардкожен" в коде. Нет обращения в словарик по константе с сервера;
    • Не удален "старый код", то есть такой код, который нигде не используется;
    • Нет блока catch у промисов, нет обработки ошибок, если сервер отвечает не ok;
    • Компоненты размещены в node_modules;
    • Отсутствуют или недостаточно подробно описаны Prop Types.
    • Экшены и редьюсеры в куче в одном файле (или в одном все экшены, в другом все редьюсеры). Нет деления на "модули", то есть каждой сущности — свои экшены и свои редьюсеры;

    Все решения с отметками по времени


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




    6m00s — Артур Донковцев Commit


    7m40s — опечатка в названии функции


    8m07s — асинхронные запросы не вынесены в экшены




    9m30s — Павел Пимкин Commit


    10m07s — все экшены в одном файле. Нет деления на модули.


    10m25s — вынос иконки (перебор данных) сделан в компоненте. Лучше в редьюсере или экшене.




    11m42s Сергей ZackFox Commit


    12m28s — "прикольные" надписи. Лучше делать "нейтрально", чтобы затем подобные задания можно было сразу посылать работодателю.


    13m05s — лишний экшен, указывающий что "загрузка" завершилась. То есть вместо трех экшенов: REQUEST / SUCCESS / STOP, можно уложиться в два: REQUEST / SUCCESS.




    16m16s — Дмитрий Петров Commit


    18m16s — использование var


    18m34s — не вынесена часть URL адреса в константу




    21m15s — Ефим Хлебный Commit


    21m17s — плохое сообщение в коммите


    22m15s — одинаковые названия экшенов.




    24m16s — Кацура Владислав Commit


    25m17s — (не ошибка) — данные приготовлены в редьюсере


    27m38s — использование e.target, лучше e.currentTarget


    28m20s==, а надо бы ===


    28m33s — использование componentWillUnmount


    29m00s (не ошибка) — рассуждение про "до серверную валидацию".


    30m05s — код не отфморатирован (на любителя)




    30m33s — Максим Сафин Commit


    31m35s — использование "не универсального" обработчика, там где это уместно.




    32m02s — Сергей Regies Linkas Commit


    33m42s — нет экшена для прелоадера


    34m30s — порядок методов в компоненте. (eslint плагин)


    35m30s — не существующий PropTypes




    35m57s — Кононов Виталий Commit




    38m02s — Ренат Рысаев Commit


    39m45s — не делаем то, что не интересно




    40m31s — Евгений Санжиев Commit


    41m20s (не ошибка) — словарик для работы с ошибками




    42m46s — Виталий Набережный Commit


    42m54s — не убраны тестовые данные




    44m50s — Вениамин Трепачко Commit


    Ачивка: очень классный дизайн.


    47m42s — redux версия не полноценная.




    47m57s — Ingvarr6 (Игорь) Commit


    48m21s — нет 404 роута




    51m20s — Екатерина Н Commit


    51m30s — не очищается ошибка




    54m48s — Роман Палесика Commit


    55m30s — не хватает экшенов на загрузку/ошибку


    56m49s — использование side-effects в редьюсере


    58m10s — (не ошибка) вынос иконки web с помощью css (sick!)




    58m53s — Умяр Юсупов Commit


    59m15s — использование callback'a в setState, что приводит к лишней перерисовке. Лучше валидировать прямо в рендере.


    61m01s — неуместное использование else if




    62m13s — dsfcv d (boortcore) Commit




    63m15s Константин Липский Commit


    65m11s — в экшен передается URL целиком, лучше просто id передавать в данном варианте.




    67m14s — Ikaow Ikaow Commit


    67m50s — сложное условие в shouldComponentUpdate, можно проще (сразу проверить на props.data и все)


    69m32se.preventDefault не первый в обработчике




    70m01s — Ali Gasymov Commit




    71m50s — Ахметанов Альберт Commit


    72m20s — компоненты в node_modules


    73m15s — дублирование обращений к переменным




    74m04s — Женя Белый Commit


    76m04s — privateRoute не вынесен в отдельный компонент


    76m33s — сложный код для перемещения иконки web


    76m56s — избыточное свойство loaded




    77m35s — Аладьин Александр Commit


    80m33s — ошибка не вынесена в словарик




    81m19s — Misha Mihail Commit


    81m43s — избыточное использование withRouter




    83m04s — Dmitrii Shapovalenko Commit




    84m00s — Даниил Commit


    84m58s — избыточное деление экшенов


    85m55s — ошибка в названии lifecycle-метода




    86m58s — Порошин Роман Commit


    87m15s — семантически неверное использование тэга article


    90m46s — лишний вызов метода массива




    91m10s — Артем Бочков Commit

    Поделиться публикацией

    Комментарии 25

      0
      «Ошибка „неправильное имя пользователя/пароль“ — выводится константой с сервера;»
      Если я правильно понял, то текст ошибки приходит с сервера. Я вообще не React-разработчик, но почему так нельзя? Многие backend-фреймворки умеют валидировать данные и выводить текст ошибок. Какой смысл его (текст ошибки) переопределять на фронтенде?
        0
        Зависит от бэкэнда. Если бэк сразу выдает «читаемую» ошибку — то можно сразу ее и выводить. Если нет — то нужно «переопределить». В задании, бэкэнд не умел выдавать красивую ошибку, а присылал: wrong_email_or_password
          +1

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

            0
            мне тоже импонирует такой подход
              0
              Мне сей подход тоже импонирует, но, в каком виде отдавать коды ошибок? Классические http? Или что-то свое кастомное? Не приведет ли кастомное к ошибкам. Например сервер возвращает строку «wrong_email_or_password», а на клиенте при проверке на равенство я потерял букву. Насчет локализации, что фронтенд выбирает, на каком языке, это получается у фронта сразу вшито несколько языковых пакетов?
              Как вам такой дизайн. Бэкэнд выдает ошибку, а фронт обращается к бэку еще раз с ошибкой и нужным языком, чтобы получить текст ошибки?
                0
                При локализации, мне кажется, текущий код языка пользователя (в мультиязычном приложении) должен отправляться на сервер с каждым запросом (куки/заголовок). Соответственно сервак должен отдавать уже локализированый ответ.
                А плодить кучу запросов, мне кажется совсем не верным решением.
                  0
                  Или что-то свое кастомное? Не приведет ли кастомное к ошибкам. Например сервер возвращает строку «wrong_email_or_password», а на клиенте при проверке на равенство я потерял букву.

                  Кастомные. Тут можно использовать строковые константы. Если бэкенд на ноде, то можно использовать общий код, иначе держать константы в синхронизированном виде для обоих частей.
            +2

            Скажу только об "общем" списке ошибок.


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


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


            P.S. Надеюсь не обидетесь, в прошлом ваши статьи не читал, поэтому попросил бы разъяснить значение термина "junior-react" из заголовка.

              0
              Спасибо за развернутый комментарий. По макету/примеру хорошее замечание, возможно, будут добавлены в будущем.

              Про детали и лень — хороший пример. Если делаешь — делай хорошо. Фронтенд, это не только про «вау как здорово и классно, и платят много», это так же про не любимые «доработки/допиливания» мелких деталей, особенно возня с формами и ошибками как раз. Мы на видео этого коснулись. На трансляции некоторые ребята ответили — да, было лень. С ленью надо работать. Заодно они увидели реальные примеры того, что им могут отправить на доработку после code-review. Поэтому, я считаю, что это «несет реальную информацию».

              p.s. хабр взрастил какой-то пласт обидчивых авторов и комментаторов?) Конструктивная критика всегда хорошо, никаких обид. Для меня junior react — начинающий разработчик (фронтендер), который пишет на стэке react. Это отчасти и начинающий javascript разработчик, или не начинающий js разрабочтик, но начинающий в react, или наоборот ничего не знающий в javascript, но так же тот, кто начал свой путь с react.

              0
              я 1 не понимаю откуда у этой статьи растут ноги?
                0
                Не понял вопроса.
                  0
                  я к тому, где вообще ссылка на предыдущую статью этого вебинара? Наткнулся на этот пост чисто случайно. Без ссылки на анонс этого теста и теста первого, пост кажется каким-то ущербным.
                    0
                    Вас понял. Я не делал анонса на хабре, так как не знаю как это здесь подать. У меня платного аккаунта нет, а анонс больше подходит под рекламу. В противовес этому, в разборе уже без ссылок (только на github), без призывов к переходу в паблики, разобраны ошибки, которые в отрыве от всей движухи в целом тянут на отдельную заметку, полезную для начинающих.

                    p.s. надо подумать, как можно сделать анонс и не разгневать публику. Аккаунт с карточкой подписи, в которой можно указать vk/telegram и тд на 3 месяца стоит дорого (14 тысяч), и тоже не особо позволяет постить ссылки в самой статье, а корпоративный аккаунт стоит и того дороже (было 70).
                      0
                      ну так что тут думать. Описываем историю своего начинания, завязываем сюжетную линию, делаем развязку и как итог снизу в виде выводов этот пост.
                0
                Иконка Web выносится вперед в компоненте (а лучше бы в редьюсере, или экшене);

                А если будет два компонента, в одном из которых нужно иконку отрисовать первой, а в другом — последней?

                  0
                  Хороший вопрос. Вижу два варианта:
                  1) диспатчнуть два экшена (например, SET_ICONS_FOR_COMP_A и SET_ICONS_FOR_COMP_B)
                  2) делать это в компоненте

                  В зависимости от частоты перерисовок компонента и каких-то еще условиий задачи, можно выбрать то, что больше подойдет. Мне пока импонирует вариант 1.

                  Так или иначе, в задаче хотелось показать, что если вам с бэкэнда пришли не угодные данные, то на мой взгляд, лучше их «перевернуть» в редьюсере.
                  +1
                  1) диспатчнуть два экшена (например, SET_ICONS_FOR_COMP_A и SET_ICONS_FOR_COMP_B)

                  Тогда либо вы храните одну копию данных и последовательное выполнение второго экшена "сломает" данные предыдущего (что ломает логику приложения, если оба компонента находятся на странице одновременно либо отображаются друг за другом без перезагрузки данных), либо вы храните две копии и это нарушает SSOT.


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

                  Все зависит от того, что считать "неугодными".


                  Система стор-экшоны-редьюсер — это m из mvc, модель ничего не должна знать о свойствах вида (о том, что вашему виду нужно вывести какой-это элемент первым). Это целиком и полностью ответственность вида, то есть компонента. В модели данные могут вообще находиться в сильно другой форме, чем потом выводится пользователю.


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

                    0
                    Отсутствуют или недостаточно подробно описаны Prop Types.

                    Я всегда считал, что PropTypes — это фишка только для development-версии. Когда это ещё было встроенным функционалом React.js, это игнорировалось в продакшене, потому что даёт снижение производительности приложения, (Насколько я понимаю, отдельный пакет prop-types, который появился после версии 15.5, тоже работает только в режиме development.)

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

                    По сути, нет смысла тратить время на PropTypes. Лучше его потратить на то, чтобы компонент самостоятельно мог выдавать ошибки или мог принудительно генерировать дефолтные значения для props, если ничего не было передано (или конвертировать входные значения в подходщий формат, если они имеют неправильный). Чтобы PropTypes мог это делать, нужно либо использовать в продакшене development-версию React, либо модифицировать его, чтобы PropTypes мог работать в продакшене.

                    Основная моя критика в этом:

                    • В режиме production PropTypes не защищает от неправильных входных данных;
                    • В режиме production PropTypes нарушает ещё один ваш критерий оценки: «Не удален „старый код“, то есть такой код, который нигде не используется»
                      0
                      Спасибо за мнение, не совсем согласен.

                      PropTypes — это некий конспект того, что должно приходить в компонент и в каком виде. В процессе разработки и поддержки — это очень удобно. Конечно, игнорирование предупреждений или внезапное изменение чего-то от бэкэнда, приведет к проблемам в production версии.

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

                      Здесь +1, но не согласен, что делать это «лучше, вместо описывания PT».

                      p.s. вместе с flow, propTypes становятся еще более удобными в режиме разработки и поддержки.
                        0
                        p.s. вместе с flow, propTypes становятся еще более удобными в режиме разработки и поддержки.

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

                          Если у вас и правда часто возникает такая необходимость, то пишите тесты то сделайте так, чтобы PropTypes работали и в prod-версии. К примеру npm:check-prop-types.

                          0
                          Удобно в качестве конспекта — да. Но это удобство в ущерб стабильности. Грубо говоря, разработчику будет удобно разрабатывать код 10 человеко-часов, но 10 000 000 человеко-часов код будет работать нестабильно на production-сервере.
                          +1
                          Лучше его потратить на то, чтобы компонент самостоятельно мог выдавать ошибки или мог принудительно генерировать дефолтные значения для props, если ничего не было передано

                          Дык это будет не бесплатно. Любой runtime-код это runtime-код. Вне зависимости от того, библиотечный он или нет. Проверок PropTypes нет в prod-коде именно из-за этого. Также как и никто не прогоняет тесты на машинах клиентов, их прогоняют на build-серверах, на локальных машинах разработчиков. DEV-инструменты это DEV-инструменты. Также как и eslint…


                          В общем ваша критика непонятна. PropTypes очень сильно выручают в случае, если вы не используете flow или TypeScript. А вот насколько они нужны, если всё таки статическая типизация используется я судить не берусь.

                            0
                            Если типизация есть, тоже удобно. PT просто сразу описываются на flow синтаксисе (плагин)
                              0
                              а мне кажется, что джунам нужны четкие правила, что делать плохо и что хорошо. Лишь со временем человек понимает, что — «Тут можно сделать по правилам, а тут их лучше проигнорировать». Это очень хорошо довели в одном докладе на holyJS youtu.be/5IjUVUlkpvQ?t=17478
                              так что данная статья больше подходит для самих джунов и тех, кто их будет нанимать. Для мидла и выше подобное уже не котируется, сказывается опыт в проектах и способы решения проблем.

                          Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                          Самое читаемое