Оценить простое задание на react?

Question

[beDenz]

Здравствуйте.
Недавно взялся за изучение react. Случайно наткнулся на тестовое задания от одной известной компании. Решил попробовать реализовать, не для собеседования, а просто для себя, чтобы попытаться понять работу библиотеки. И вот что получилось.
Прошу покритиковать, кому не сложно. Уровень владения ниже плинтуса. По сути это первое приложение, если не считать элементарных крестиков-ноликов и тудушки.
Специально не стал разбивать на кучу мелких компонентов, сервисов, утилит, что бы удобно было делать код ревью. В некоторых местах хромает логика сортировок, это я не допилил.

Код
Deploy

Comments

[profesor08]

фильтр не работает

[beDenz]

profesor08, спасибо!
Логика фильтра действительно до конца дописана, на данный момент объект фильтруется только по одному выбранному параметру. Просто целью было разобраться в реакт, а не написать production код для поиска.
А фильтр я переработаю, когда буду переписывать на TS.

[Андрей Костяев]

1. Структура проекта ужасная, именно про то, что даже App лежит просто в src, не и т.д., найди нормальный проект, где хорошая структура.
2. Вместо useState лично я бы заюзал useReducer, код получится намного чище.
3. Ну определение компонетов, как по мне для функциональных компонентов лучше делать так:

import React from "react";
import logo from "./logo.png";

const Logo = () => {
  return (
    <div className="logo">
      <img src={logo} alt="" />
    </div>
  );
}

export default Logo

4. Избегать дублирования, для примера инпут чекбокса можно было вынести как отдельный ui компонент.
5. Для таких моментов

className={props.sort === "price" ? "tabs__item active" : "tabs__item"}

лучше юзать либу classnames
6.

const duration = value => {
    // Функция выводит форматированное время полета
    let hour = parseInt(value / 60);
    let minute = value - hour * 60;
    return (
      <span>
        {hour} ч {minute} м
      </span>
    );
  };

названия таких функций желает лучшего, читаемость кода нужно держать на уровне. тут бы лучше подошло renderDuration, ну и можно было вообще мини компонент сделать, куда прокидывали бы value.
7. Я конечно не углублялся в код, но зачем юзать for? даже у ванильного js есть такая штука как array.map и т.д.
Это так, чисто мое мнение, по коду просто бегло пробежался.

P.S. еще почитайте про useCallback.
Ну и много моментов по поводу наименования переменных, констант и т.д.
Например

const getSearchIdUrl = "https://front-test.beta.aviasales.ru/search";

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

[Дмитрий]

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

fetch(getSearchIdUrl).then(response => {
  response.json().then(data =>
    fetch(tickets + `${data.searchId}`).then(response => {
      if (response.status !== 200) {
        setRequestStatus(true);
        return;
      }
      response.json().then(data => setTicketList(data));
    })
  );
});

написать

fetch(getSearchIdUrl)
  .then(response => response.json())
  .then(data => fetch(tickets + `${data.searchId}`))
  .then(response => {
      if (response.status !== 200) {
        setRequestStatus(true);
      } else {
        return response.json()
      }
    })
  .then(data => setTicketList(data))

[Дмитрий]

Функция sortStops по названию не отвечает назначению.
sort - Это сортировка, то есть расположение в порядке возрастания или убывания.
А отбор это filter,

Также функцию можно написать значительно короче, причем чтобы верно фильтровалось, то есть так например

const filterByStops = (x, stopsFilter) => {
  //  Теперь функция фильтрации по числу остановок и верно названа
  // и работает как надо и в 10 раз короче =)
  
  if (x === undefined) return 
  
  const arr = Array.from(x);
  if (stopsFilter.all) return arr

  // Получаем массив в виде [true, false, false, true], где для каждого индекса
  // указано отбираем ли его
  const stopsOptions = Array.from(['noStop', 'oneStop', 'twoStop', 'threeStop']).map(
    option => Boolean(stopsFilter[option])
  )

  // Фильтруем элементы так чтобы у каждого сегмента значение остановок было true
  return arr.filter(item =>
    item.segments.every(segment => stopsOptions[segment.stops.length])
  )
}

[Дмитрий]

К верхней функции также замечание. Вместо того чтобы писать

if (x !== undefined) {
    // куча кода
} else {
    return
}

Лучше написать

if (x === undefined) return

// Куча кода без ненужного отступа

[Дмитрий]

Обработчик нажатия на фильтры также легко сократить

const onClickStops = e => {
  // обработчик нажатия на фильтры выбора количества остановок
  const chosenOption = e.target.getAttribute("type")
  setSortStops({
    ...sortStops,
    chosenOption: !sortStops[chosenOption]
  })
}

[beDenz]

Дмитрий, спасибо. Это очень элегантно. С фильтром у меня были мысли как сделать, но количество кода требовалось в разы больше.
С неймингом и структурой стараюсь исправляться.

Answer 1

[Дмитрий]

Filters также можно реорганизовать элегантней. Во-первых вывести все фильтры в цикле, чтобы не плодить кода

const allFilters = [
  { id: 'all', title: 'Все' },
  { id: 'noStop', title: 'Без пересадок' },
  { id: 'oneStop', title: '1 пересадка' },
  // и т.д.
]

return <div className="filters">
  <h3 className="filters__title"> Количество пересадок </h3>
  {allFilters.map(({id, title}) => (
    <div className="filters__item" key={id}>
      <input type="checkbox" id={id} hidden
             defaultChecked={props.sortStops[id]}
      />
      <label htmlFor={id} onClick={() => props.onClick(id)}>
        {title}
      </label>
    </div>)
  )}
</div>

Во вторых обратите внимание вместо
onClick={props.onClick}

можно сразу передать id в функцию-обработчик, а не выковыривать ее через e.target
onClick={() => props.onClick(id)}

И обработчик нажатия тогда будет выглядеть

const onClickStops = chosenOption => {
  // обработчик нажатия на фильтры выбора количества остановок
  setSortStops({
    ...sortStops,
    chosenOption: !sortStops[chosenOption]
  })
}