Code improvement review and implementation

@ReactMeteor Someone kindly had a look through our code and suggested some improvements. Please review their suggestions and implement them, or create tasks here for them, if you agree (as two people are in favour), or comment here if you disagree with them so others can join in and break the tie :yum:

  • A lot of use of global variables stored in window object, which reduces reusability. For example window.__setDocumentTitle can simply be a utility function .
  • The code contains a few deprecated functions, but it’s understandable, since it’s not the newest. (for example string.substr)
  • Class components are used thoughout the app making the code verbose in some places. Also think of only passing necessary props to children. For example:
// before
class Events extends Component {
  constructor(props) {
    super(props);
    this.state = {  }
  }

  render() { 
    const { userEvents, user, isAllEvents, deleteAllEvents } = this.props;   
    const events = userEvents.filter(ele => {
      return ele.organiser._id === user._id;
    })

    return (
      <React.Fragment>
        < EventsDisplay userEvents={events} isAllEvents={isAllEvents} deleteAllEvents={deleteAllEvents} /> 
      </React.Fragment>
      )
  }
}

// after
const OrganizerEvents = ({ userEvents, orgernazierId, ...props }) => (
    <EventsDisplay
      userEvents={userEvents.filter(
        ({ organiser }) => organiser._id === orgernazierId
      )}
      {...props}
    />
);
  • You can also deconstruct props to make the code cleaner
// before 
const idToDelete = this.props.idToDelete;
const deleteDocument = this.props.deleteDocument;
const deleteText = this.props.deleteText;
// after
const { idToDelete, deleteDocument, deleteText } = this.props;
  • In CancelDeleteBtn, prefer creating components globally and not inside another ones.
  • in CategoryFropDown.render you create an array and push components to it. I would prefer to simply map the array to elements instead, since it’s a common pattern.
// before
render() {
    const { allPosibleCategories} = this.props;
    const categoriesForDropDown = [<option key={"Sort By Category"} disabled>Sort By Category</option>];
    allPosibleCategories.forEach(ele=>{
      categoriesForDropDown.push(<option key={ele} value={ele}>{ele}</option>);
    })

    return (
      <select id="category-select"  onChange={this.change}>
        {categoriesForDropDown}
      </select>
    )
  }
// after
render = () => (
    <select id="category-select" onChange={this.change}>
      <option key={"Sort By Category"} disabled>
        Sort By Category
      </option>
      {this.props.allPosibleCategories.map((ele) => (
        <option key={ele} value={ele}>
          {ele}
        </option>
      ))}
    </select>
  );
  • Also instead of using componentDidUpdate, prefer using controlled components with useState (checkout <input> – React)

Based on the suggestions, there are several things I think we can do:

  1. upgrade react to be 16.8.0 or higher to enable hooks and take advantage of other features.
  2. consider using function components and hooks when needed to increase code readability and reduce code verbose
  3. improve the ways to handle props. eg, only pass necessary props, destructuring props
  4. update deprecated functions.
  5. create utility functions to increase code reusability

We can look at the last two suggestions about CancelDeleteBtn and CategoryForDropDown when we are there, since they are more detailed suggestions - there might be many of such that we can fix along the way.

Also we can easily fix the linting issues.

@AndyatFocallocal What do you think about upgrading react? Since we updated meteorjs, I think it makes sense to do it now. I can look into that.

1 Like

@zhna123 Yes i agree, it would be great to have the latest versions of everything running.

As you’ve done the linting issue it would be good to move the other fixes out into their own Tasks so we can advance this into the Done column and work on each separately. It would be good if you’d like to have a go at that so you have a better idea how our internal task system operates.

To do so you’d create a new topic in the React Js and Meteor Category, and give it the same tags as this one currently as. We advance a task in the Kanban by changing the tag backlog to sprint, doing or done.

On a desktop you can drag and drop the card. On a mobile device that functionality hasn’t been built yet so we have to change the tag manually.

Okay I created several tasks. I can modify them or create smaller ones while I work on the code. For now I assigned myself the update react task. I will start on that now.

1 Like