FACG4 / nigerian-airlines

MIT License
0 stars 1 forks source link

Some ideas for refactoring duplication in admin pages #25

Open tomduggan85 opened 6 years ago

tomduggan85 commented 6 years ago

Hi @razan99 @Balsam-Faysal @InassTubail @amusameh @NoureldeanSaed There's a little bit of common / repeated jsx in the three admin pages (AddFlight, UpdateFlight, Flight) that looks like this:

<div className="container">
    <Header />
    <div className="sub-container-sidenav-form">
        <SideNav />
        <div className="sub-container-form">
        ...

Ideally, that common code could be moved / restructured so it doesn't have to be repeated on each page (the DRY principle - Don't Repeat Yourself). That's best because then if you need to change it, you only have to change it in one place (otherwise you'll change it everywhere and you might forget one of the places). But this is a small project and it won't have more than these 3 pages, so there are probably more important things to worry about :)

But just for fun, here are some ideas on how you could remove that duplication in the future / on other projects:

1) Put that common code in a parent class and use inheritance to set up AddFlight, UpdateFlight, Flight, as child classes of it. The child classes then override part of the rendering with their specific render logic. The parent class looks like this (sorry for bad formatting)

class AdminPage extends React.Component {
  render() {
    return (
      <div className="container">
        <Header />
          <div className="sub-container-sidenav-form">
          <SideNav />
            <div className="sub-container-form">
              {this.renderPageContent()}
            <div>
       </div>
     </div>
    )
  }
  renderPageContent() {
    return null// Do nothing here, because the child classes will override this
  }
}

.. and the child looks something like this:

class UpdateFlight extends AdminPage {
  renderPageContent() {
    // All of the interesting stuff goes here 
    return (<DetailsCard .......
  }
  //Notice how UpdateFlight does not define a render() method, it relies on the AdminPage render method
}

OR

2) Use a Higher-Order Component - a HOC (https://reactjs.org/docs/higher-order-components.html). Create a HOC function to wrap all of the pages in their common jsx structure, and then export the pages using that HOC. This is how react-redux's connect() function works. The HOC could look like this:

const withAdminComponent = ( PassedComponent ) => {
  render() {
    return (
      <div className="container">
        <Header />
          <div className="sub-container-sidenav-form">
          <SideNav />
            <div className="sub-container-form">
              <PassedComponent {...this.props} />
            <div>
       </div>
     </div>
    )
  }
}

And then UpdateFlight, Flight and AddFlight wouldn't need that jsx in their render functions, but would export themselves already wrapped in the HOC:

class UpdateFlight extends React.Component {
  ...
}
export default withAdminComponent(UpdateFlight)

OR 3) Nested router You could have an AdminPage class that renders the same common jsx as shown above and then, in its render function, has a react-router <Switch> tag to conditionally display either UpdateFlight, Flight, or AddFlight. This is my least favorite option because there would still have to be routes in App.js, to handle other routes, and I think it's harder to understand an app when there are multiple react-router switches happening in the codebase.