Anti-patterns Discussion session

Anti-pattern

Anti-patterns are certain patterns in software development that are considered bad programming practices.

 

The same pattern may have been considered correct at one point in the past, but now developers have realized that they cause more pain and hard-to-track bugs in the long term.

Anti-pattern

  • You are probably right and there is no bug in the code, but you should not do it.

Topic for discussion

Discussion

  1. Use of index as key in component
  2. Using object literals directly to component
  3. Copying props to state
  4. Handling multiple Promises
  5. Binding Event to component or React element
  6. Using setState
  7. Find the issue

Use index as key in component

Text

import React, { Component } from 'react';
import { render } from 'react-dom';
import Hello from './Hello';
import './style.css';

class App extends Component {
  constructor() {
    super();
    this.state = {
      list: [
        { id: "1", name: "Suraj K.C." },
        { id: "2", name: "Ajay Shrestha" },
        { id: "3", name: "Sujan Shrestha" },
        { id: "4", name: "Arjun Yonjan" },
        { id: "5", name: "Saraswati Shrestha" },
        { id: "6", name: "Rosy Shrestha" },
        { id: "7", name: "Aishwarya Shrestha" }
      ]
    };
  }

  update = () => {
    const list = [...this.state.list];
    list.splice(1,1);
    this.setState({
      list
    })
  }

  render() {
    return (
      <div>
        <NameList nameList = {this.state.list} />
        <button onClick={this.update}>click me</button>
      </div>
    );
  }
}

class NameList extends React.Component {
  render () {
    return this.props.nameList.map((name, index) => {
        return <Name key={index} {...name} />
    });
  }
}

class Name extends React.Component {
  componentWillUpdate(nextProps) {
    console.log(this.props.name, nextProps.name);
  }
  render () {
    return (
      <div>Name: {this.props.name}</div>
    )
  }
}

render(<App />, document.getElementById('root'));

Problem

React use key to match children in the original tree. The keys are used for identification

DEMO

https://stackblitz.com/edit/react-3cnztu

Solution

key should be unique, stable, Predictable

Like index, random numbers or timestamps should not be used as keys

Using object literals directly to component

Demo

Problem: https://stackblitz.com/edit/react-ugnjq5?file=index.js

Reason & Solution?

Solution

https://stackblitz.com/edit/react-ejycu5?file=index.js

Avoid copying props into state

class EmailInput extends Component {
  state = { email: this.props.email };

  render() {
    return <input onChange={this.handleChange} value={this.state.email} />;
  }

  handleChange = event => {
    this.setState({ email: event.target.value });
  };

  componentWillReceiveProps(nextProps) {
    // This will erase any local state updates!
    // Do not do this.
    this.setState({ email: nextProps.email });
  }
}

Problem

  • Adding additional layer to change state won't help
class EmailInput extends Component {
  state = {
    email: this.props.email
  };

  componentWillReceiveProps(nextProps) {
    // Any time props.email changes, update state.
    if (nextProps.email !== this.props.email) {
      this.setState({
        email: nextProps.email
      });
    }
  }
  
  // ...
}

But it is an anti-pattern

Solutions

  • Data down action up approach
  • Redux
class EmailInput extends Component {
  render () {
   return <input value={this.props.email} onChange={this.props.onChange} />
  }
}

class Form extends Component {
  state = {
    email:""
  }
  onChange = (event) => {
    this.setState({
    	email: event.target.value
    })
  }
  render() {
    return (
       <EmailInput email={this.state.email} onChange={this.onChange} />
    )
  } 
 }

Promises

loadSomething().then(function(something) {
    loadAnotherthing().then(function(another) {
                    DoSomethingOnThem(something, another);
    });
});

Problem?

Solution

promise.all([loadSomething(), loadAnotherThing()])
    .spread(function(something, another) {
        DoSomethingOnThem(something, another);
});

Use of bind in react app

<input onChange={this.updateValue.bind(this)} value={this.state.name} />


<input 
  onChange={ (evt) => this.setState({ name: evt.target.value }) } 
  value={this.state.name} 
/>

Problem?

Solution

  • arrow function
  • use bind in constructor
class EmailInput extends React.Component {
   constructor(props) {
     this.state = {
        value: ""
     }
     this.onChange = this.onChange.bind(this); // creates new instance of function
   }
   
   function onChange(event) {
       this.setState({value: event.target.value})
   }
   
   render() {
       <input value={this.state.value} onChange={this.onChange} />
   }
}


class EmailInput extends React.Component {
   constructor(props) {
     this.state = {
        value: ""
     }
   }
   
   onChange = (event) => {
       this.setState({value: event.target.value})
   }
   
   render() {
       <input value={this.state.value} onChange={this.onChange} />
   }
}

Using Set state

class MyComponent extends Component {
  constructor(props) {
    super(props);
    this.state = {
      counter: 350
    };
  }

  updateCounter() {
    this.state.counter = this.state.counter + this.props.increment;
    
    this.setState({
      counter: this.state.counter + this.props.increment; 
    });
    
    this.setState({
      counter: this.state.counter + this.props.increment;
    });
    
  }
  ...
}

Problem?

class MyComponent extends Component {
  constructor(props) {
    super(props);
    this.state = {
      counter: 350
    };
  }

  updateCounter() {
    // this line will not work
    this.state.counter = this.state.counter + this.props.increment;
    
    // ---------------------------------
    
    // this will not work as intended
    // May not render
    this.setState({
      counter: this.state.counter + this.props.increment;
    });
    
    // what value this.state.counter have?
    this.setState({
      counter: this.state.counter + this.props.increment; 
    });
    
  }
  ...
}
class MyComponent extends Component {
  constructor(props) {
    super(props);
    this.state = {
      counter: 350
    };
  }

  updateCounter() {
    // this will work
    this.setState((prevState, props) => ({
      counter: prevState.counter + props.increment
    }));
    
    this.setState((prevState, props) => ({
      counter: prevState.counter + props.increment
    }));
    
  }
  ...
}

Find the issue?

import React, { Component } from 'react';
import axios from 'axios';

// API
import { onLoadUser } from './UserAPI';

class Example extends Component {
  state = {
    isLoading: false,
    user: {},
  }
  
  componentDidMount() {
    this.onLoadUser();
  }
  
  onLoadUser = async () => {
    try {
      this.setState({ isLoading: true });
      const data = await onLoadUser(this.signal.token);
    } catch (error) {
      this.setState({ isLoading: false });

    }
  }
    
  render() {
    return (
      <div>
      	<pre>{JSON.stringify(this.state.user, null, 2)}</pre>
      </div>
    )
  }
 
}

Solution?

import React, { Component } from 'react';
import axios from 'axios';

// API
import { onLoadUser } from './UserAPI';

class Example extends Component {
  signal = axios.CancelToken.source();
  _isMounted = true;
  state = {
    isLoading: false,
    user: {},
  }
  
  componentDidMount() {
    this.onLoadUser();
  }
  
  componentWillUnmount() {
    this._isMounted = false;
  }
  
  onLoadUser = async () => {
    try {
      this.setState({ isLoading: true });
      const data = await onLoadUser(this.signal.token);
      if(this._isMounted === true) {
        this.setState({ user: data, isLoading: true });
      }
    } catch (error) {
      if(this._isMounted === true) {
        if (axios.isCancel(err)) {
           console.log('Error: ', err.message); // => prints: Api is being canceled
        } else {
        this.setState({ isLoading: false });
        }
      }
    }
  }
    
    render() {
      return (
        <div>
          <pre>{JSON.stringify(this.state.user, null, 2)}</pre>
        </div>
      )
    }
  };
 
}

It is considered as anti-pattern

Solution

import React, { Component } from 'react';
import axios from 'axios';

// API
import { onLoadUser } from './UserAPI';

class Example extends Component {
  signal = axios.CancelToken.source();

  state = {
    isLoading: false,
    user: {},
  }
  
  componentDidMount() {
    this.onLoadUser();
  }
  
  componentWillUnmount() {
    this.signal.cancel('Api is being canceled');
  }
  
  onLoadUser = async () => {
    try {
      this.setState({ isLoading: true });
      const data = await onLoadUser(this.signal.token);
      this.setState({ user: data, isLoading: true });
    } catch (error) {
      if (axios.isCancel(err)) {
        console.log('Error: ', err.message); // => prints: Api is being canceled
      } else {
        this.setState({ isLoading: false });
      }
    }
  }
    
    render() {
      return (
        <div>
          <pre>{JSON.stringify(this.state.user, null, 2)}</pre>
        </div>
      )
    }
  };
 
}
Made with Slides.com