Renderowanie warunkowe wiele if else refactor kodu w React

Odpowiedz Nowy wątek
2019-08-10 15:06
0

Czy ktoś ma pomysł jak zrefactorować poniższą metode z wieloma if else? Komponent służy do wyrenderowania Selecta z różnymi opcjami w zależności jaki obiekt został przekazany do komponentu przez props.

render() {
        const selectValue = this.state.selectValue;
        const element = this.props.element;
        const id = element.id;
        let options;

        if (id === 'location') {
            options = element.items.map(option => {
                const content = option.city + ', ' + option.country + ', ' + option.postCode;
                return <Option option={option} content={content} />;
            });
        } else if (id === 'range') {
            options = element.items.map(option => {
                if (option.isRange) {
                    const content = option.currencySymbol + option.min + ' - ' + option.max + ' ' + option.type.name;
                    return <Option option={option} content={content} />;
                } else {
                    const content = option.currencySymbol + option.value + ' ' + option.type.name;
                    return <Option option={option} content={content} />;
                }
            });
        } else if (id === 'value') {
            options = element.items.map(option => {
                if (option.isRange) {
                    const content = option.min + ' - ' + option.max;
                    return <Option option={option} content={content} />;
                } else {
                    const content = option.value;
                    return <Option option={option} content={content} />;
                }
            });
        } else {
            options = element.items.map(option =>
                <Option option={option} content={option.name} />
            );
        }

        return (
            <div className={'form-group'}>
                <label>{id}</label>
                <select
                     value={selectValue}
                     onChange={this.handleTemplateChange}
                     className={'form-control'}
                 >
                     <option value={'default'} disabled>Select Template</option>
                     {options}
                </select>
             </div>
        );
    }

Pozostało 580 znaków

2019-08-12 11:27

Ostateczne odchudzenie tego kawałka kodu o 39% - z 1992 znaków do 1222


renderOptions = (element) => {
    const { id } = element;

    return element.items.map((option) => {
      let content = option.name;

      if (id === 'location') {
        content = `${option.city}, ${option.country}, ${option.postCode}`;
      }

      if (id === 'range') {
        if (option.isRange) {
          content = `${option.currencySymbol + option.min} - ${option.max} ${option.type.name}`;
        } else {
          content = `${option.currencySymbol + option.value} ${option.type.name}`;
        }
      }

      if (id === 'value') {
        if (option.isRange) {
          content = `${option.min} - ${option.max}`;
        } else {
          content = option.value;
        }
      }

      return <Option option={option} content={content} />;
    });
  };

  render() {
    const { selectValue } = this.state;
    const { element } = this.props;
    const options = this.renderOptions(element);

    return (
      <div className="form-group">
        <label>{element.id}</label>
        <select
          value={selectValue}
          onChange={this.handleTemplateChange}
          className="form-control"
        >
          <option value="default" disabled>Select Template</option>
          {options}
        </select>
      </div>
    );
  }

Nie daję gwarancji, że to zadziała od strzału.
Zmiany:

  • template literals - ${option.city}, ${option.country}, ${option.postCode} zamiast option.city + ', ' + option.country + ', ' + option.postCode itp. - to nawet eslint przy domyślnych ustawieniach sam zmienia przy auto formatowaniu
  • destrukturyzacja objektów - const { selectValue } = this.state; zamiast const selectValue = this.state.selectValue;
  • osobiście wydaje mi się że dobrą praktyką jest nietrzymanie sporej logiki w renderze, lepiej ją wynosić do metod jak w przykładzie this.renderOptions(). Po pierwsze wygląda to schludniej (zwłaszcza gdyby było tam nawalone jeszcze więcej logiki na kilkadziesiąt/kilkaset linijek, a znam takie przypadki). Po drugie przy każdym rerenderze funkcje z rendera tworzone by były w pamięci, a tak są stworzone tylko raz jako metody.
  • pozbycie się potwarzania dla każdego ifa element.items.map() i <option option="{option}" content="{content}" />
  • pozbycie się ostatniego else poprzez domyślne nadanie wartości let content = option.name; - jeśli żaden if nie zmieni contentu, to on pozostanie i będzie przekazany do <option content="{content}" />

Pozostało 580 znaków

2019-08-12 12:20
4

W React refactor skomplikowanej logiki if else przeprowadzasz tak samo jak w każdym innym języku programowania czyli przy użyciu wzorca projektowego Command. W praktyce wygląda to tak, że tworzysz hashmapę która mapuje klucz na wartość. W przedstawionym przypadku kluczem będzie id, a wartością funkcja anonimowa, która robi co trzeba. W JS hasmapa to Map.prototype.

renderOptions = (element) => {
  const { id } = element;
  const options = new Map();

  options.set('location', (option) => `${option.currencySymbol + option.min} - ${option.max} ${option.type.name}`);
  options.set('range', (option) => option.isRange ? 
    `${option.currencySymbol + option.min} - ${option.max} ${option.type.name}` :
    `${option.currencySymbol + option.value} ${option.type.name}`);
  options.set('value',  (option) => option.isRange ? `${option.min} - ${option.max}` : option.value);

  return element.items.map((option) => <Option option={option} content={options.get(id)} />);
};

render() {
  const { selectValue } = this.state;
  const { element } = this.props;
  const options = this.renderOptions(element);

  return (
    <div className="form-group">
    <label>{element.id}</label>
    <select
      value={selectValue}
      onChange={this.handleTemplateChange}
      className="form-control"
    >
      <option value="default" disabled>Select Template</option>
      {options}
    </select>
  </div>
  );
}

Dla range wyszedł długi ternary, który jest brzydki, więc można się jeszcze pokusić na osobny Command w zależności od option.isRange


Wiedza to potęga
edytowany 4x, ostatnio: Haskell, 2019-08-12 12:29

Pozostało 580 znaków

2019-08-12 15:02
2

Ładne rozwiązanie. Tego nie znałem i czekam aż wykorzystam w praktyce.

Jedno małe ale. Wygląda na to, że Twoje rozwiązanie nie bierze pod uwagę tego:

       else {
            options = element.items.map(option =>
                <Option option={option} content={option.name} />
            );
        }

być może wystarczyłoby

    <Option option={option} content={options.get(id) || option.name} />

czy jest może inna alternatywa?

edytowany 1x, ostatnio: castorFiber, 2019-08-12 15:05
dzięki rzeczywiście tego nie bierze pod uwagę, nie testowałem tego kodu tylko pisałem z ręki. Twoje rozwiązanie wydaje się ok. - Haskell 2019-08-12 15:48

Pozostało 580 znaków

2019-08-16 23:53
0

@castorFiber: a jak to ma się do wydajności? W moim rozwiązaniu są maksymalnie 3 sprawdzenia warunku, a w twoim dla każdego elementu w liście są wykonywanie sprawdzenia

Pozostało 580 znaków

2019-08-17 08:02
1

@discostar to nie będzie miało znaczącego wpływu na wydajność. Problemy z wydajnością pojawiają się w React gdy komponent jest renderowany wielokrotnie bez potrzeby.


Wiedza to potęga
edytowany 2x, ostatnio: Haskell, 2019-08-17 08:15

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0