Как можно улучшить этот компонент?

Question

[1233211]

Подскажите, что в этом коде можно было бы улучшить и какие тут могут быть или есть проблемы
Спасибо

import React, { Component } from "react";
import { getUsers, getOrganizations } from "./api";

class App extends Component {
  state = {
    loading: true,
    selectedOrg: null
  };
  users = [];
  organizations = [];
  componentDidMount() {
    getUsers()
      .then((users) => (this.users = users))
      .then(() => getOrganizations())
      .then((organizations) => (this.organizations = organizations))
      .then(() => this.setState({ loading: false }));
  }

  selectOrg = (org) => {
    this.setState({ selectedOrg: org });
  };

  resetSelectedOrg = () => {
    this.setState({ selectedOrg: false });
  };

  render() {
    if (this.state.loading) {
      return "Loading...";
    }

    let users = [];

    for (let i = 0; i < this.users.length; i++) {
      const name = this.users[i].name;
      let org;

      for (let j = 0; j < this.organizations.length; j++) {
        if (this.organizations[j].id === this.users[i].organization) {
          org = this.organizations[j].name;
        }
      }

      users.push(
        <div className="user-list-item">
          <div>name: {name}</div>
          <div onClick={() => this.selectOrg(org)}>org: {org}</div>
        </div>
      );
    }

    if (this.state.selectedOrg) {
      users = [];
      for (let i = 0; i < this.users.length; i++) {
        const orgId = this.organizations.find(
          (o) => o.name === this.state.selectedOrg
        ).id;

        if (this.users[i].organization === orgId) {
          users.push(
            <div className="user-list-item">
              <div>name: {this.users[i].name}</div>
              <div>org: {this.state.selectedOrg}</div>
            </div>
          );
        }
      }
    }

    return (
      <div>
        {this.state.selectedOrg && (
          <button onClick={() => this.resetSelectedOrg()}>
            reset selected org
          </button>
        )}
        <div className="user-list">{users}</div>
      </div>
    );
  }
}

export default App;

Answer 1

[Dosya]

getUsers()
            .then((users) => (this.users = users))
            .then(() => getOrganizations())
            .then((organizations) => (this.organizations = organizations))
            .then(() => this.setState({ loading: false }));

Вот здесь место then лучше использовать await и try catch.

if (this.state.loading) {
      return "Loading...";
    }

    let users = [];

    for (let i = 0; i < this.users.length; i++) {
      const name = this.users[i].name;
      let org;

      for (let j = 0; j < this.organizations.length; j++) {
        if (this.organizations[j].id === this.users[i].organization) {
          org = this.organizations[j].name;
        }
      }

      users.push(
        <div className="user-list-item">
          <div>name: {name}</div>
          <div onClick={() => this.selectOrg(org)}>org: {org}</div>
        </div>
      );
    }

Вот здесь переменные всегда должны быть сверху выше всех

{this.state.selectedOrg && <button onClick={() => this.resetSelectedOrg()}>reset selected org</button>}

Не стоит создавать анонимные функции это плохо влияет на React

for (let j = 0; j < this.organizations.length; j++) {
        if (this.organizations[j].id === this.users[i].organization) {
          org = this.organizations[j].name;
        }
      }

Я одно понять не могу зачем делать такие циклы и использовать как перебор массива, когда есть
for (let i of arr)
Работает как у тебя, но кода будет чище

В целом компомнент не плохой, но написан на классовом компоненте, лучше писать на функиональном компоненте кода будет намного чище

Comments

[WbICHA]

Вот здесь переменные всегда должны быть сверху выше всех

Зачем?

Не стоит создавать анонимные функции это плохо влияет на React

Серьёзно? Париться из-за 1-2мс на 1000 вызовов?

Я одно понять не могу зачем делать такие циклы и использовать как перебор массива, когда есть
for (let i of arr)
Работает как у тебя, но кода будет чище

А ещё лучше использовать файнд.

[1233211]

Почему лучше использовать не классовый компонент, а функциональный?

[Dosya]

WbICHA,

Серьёзно? Париться из-за 1-2мс на 1000 вызовов?

ДА!. Я написал чтоб он знал об этом. Ладно сейчас это ничего не значит для его кода, но в каждом собеседование спрашивают "Почему не стоит создовать анонимную функцию?". А представь он будет участвовать в каком-то production проектах и будет тормозить приложение, за это ему руку оторвут.

Вот здесь переменные всегда должны быть сверху выше всех

Код будет намного понятнее, представь если там будет еще десятки переменных что тогда будет

А ещё лучше использовать файнд.

Да, можно, но автор вопроса использовал конкретно циклы, а не метод

[Dosya]

1233211, функиональный компанент намного лучше. Ты просто напиши пару проектов на фукнциональном тогда поймешь

[WbICHA]

Dosya,

Код будет намного понятнее

Нет.

представь если там будет еще десятки переменных что тогда будет

Если их объявлять в рандомном месте, да, а если по конкретному паттерну, то нет. Но этот паттер не обязан быть "все переменные наверху".

Да, можно, но автор вопроса использовал конкретно циклы, а не метод

Допустим, но у тебя значение называется как i и объявлена через лет, а не конст. И, главное, ты не написал, чтобы он добавил брейки в ифы, а это намного важнее в данном случае, нежели фор..оф.

[Dosya]

WbICHA,

Нет.

Да

Если их объявлять в рандомном месте, да, а если по конкретному паттерну, то нет. Но этот паттер не обязан быть "все переменные наверху".

Я об этом и говорю же что не нужно рандомно месте создавать переменные.

Допустим, но у тебя значение называется как i и объявлена через лет, а не конст. И, главное, ты не написал, чтобы он добавил брейки в ифы, а это намного важнее в данном случае, нежели фор..оф.

Ну, ок.

На этом хочу остановится у меня нет времени дальше спорить с тобой