1
\$\begingroup\$

It's a simple app that uses a public API to display data. It works as is, but I'm seeing a lot of spaghetti code that my limited experience can't fix on its own.

import React, { Component } from "react";
import Spinner from "./components/common/Spinner";
import "../src/App.css";

class App extends Component {
  constructor(props) {
    super(props);
    this.state = {
      items: [],
      isLoading: false
    };
  }

  componentDidMount() {
    const urlParams = new URLSearchParams(window.location.search);
    const city = urlParams.get("city");
    fetch(`http://opentable.herokuapp.com/api/restaurants?city=${city}`)
      .then(res => res.json())
      .then(json => {
        this.setState({
          isLoading: true,
          items: json
        });
      });
  }
  render() {
    const { isLoading, items } = this.state;
    let content = <h2>No Data</h2>;
    if (!items.restaurants || items.restaurants === []) {
      content = <h2>No Data</h2>;
    } else if (items.restaurants.length !== 0) {
      console.log(items);
      let itemsToArray = Object.values(items.restaurants);
      content = (
        <div className="App">
          <div className="row">
            <div className="col-md-3">
              {itemsToArray.map(item => (
                <li key={item.id} style={{ listStyleType: "none" }}>
                  <a
                    href={item.reserve_url}
                    style={{ color: "red" }}
                    target="_blank"
                  >
                    {item.name}
                  </a>{" "}
                  | <span style={{ fontStyle: "italic" }}>{item.address}</span>{" "}
                  | Price rating:{" "}
                  <span style={{ color: "red" }}>{item.price}</span>
                </li>
              ))}
            </div>
          </div>
        </div>
      );
    }
    return !isLoading ? (
      <div>
        <Spinner />
      </div>
    ) : (
      <React.Fragment>
        <div className="row">
          <div className="col-md-6">
            <div>{content}</div>
          </div>
        </div>
      </React.Fragment>
    );
  }
}

export default App;
\$\endgroup\$
2
\$\begingroup\$

I can think of mutliple ways to improve this code.

First, error handling. In your case, not receiving data from your API call may cause your program to crash.

You will have to add an error variable in your state :

this.state = {
    items: [],
    isLoading: true,
    error: ''
};

And add a .catch at the end of your fetch function (or use the new async/await syntax surrounded by a try/catch block).

You should also put your response formatting here instead of the rendering function, to avoid reformatting your data everytime your component is rerendered :

fetch(`http://opentable.herokuapp.com/api/restaurants?city=${city}`)
    .then(res => res.json())
    .then(json => {
        this.setState({
            isLoading: false,
            items: Object.values(json.restaurants) //Data formatting
        });
    })
    .catch(error =>{
        this.setState({
            isLoading: false,
            error // Sends the error object (or message, I am not sure about the exact syntax)
        });
    });

I also noticed that you set your isLoading state to true when you finished loading and to false in your initial state so I reversed it.

Now, the rendering.

JSX allows you to put conditions inside your HTML, making the syntax of your render way more readable and avoid code duplication :

render() {
    const { isLoading, items, error } = this.state;

    return (
        <div className="row">
            <div className="col-md-6">
                <div className="App">
                    <div className="row">
                        <div className="col-md-3">
                            {isLoading && <Spinner />}
                            {error && <div>Error : {error}</div>}
                            {!isLoading && !items.length && <h2>No Data</h2>}
                            <ul> // Did you forget this node ?
                                {items.map(({ id, reserve_url, address, price, name }) => //Won't do anything if the items array is empty
                                    <li key={id} style={{ listStyleType: "none" }}>
                                        <a
                                            href={reserve_url}
                                            style={{ color: "red" }}
                                            target="_blank"
                                        >
                                            {name}
                                        </a>{" "}
                                        | <span style={{ fontStyle: "italic" }}>{address}</span>{" "}
                                        | Price rating:{" "}
                                        <span style={{ color: "red" }}>{price}</span>
                                    </li>
                                )}
                            </ul>
                        </div>
                    </div>
                </div>
            </div>
        </div>
    )
}

I also deconstructed your parameters in the map function.

Working example :

class App extends React.Component {
    constructor(props) {
        super(props);
        this.state = {
            items: [],
            isLoading: true,
            error: ''
        };
    }

    componentDidMount() {
        const urlParams = new URLSearchParams(window.location.search);
        const city = urlParams.get("city");

        fetch(`http://opentable.herokuapp.com/api/restaurants?city=London`) // Hard coded London
            .then(res => res.json())
            .then(json => {
                setTimeout(() => { //Just to see the loading element / spinner
                    this.setState({
                        isLoading: false,
                        items: Object.values(json.restaurants)
                    });
                }, 1500)
            })
            .catch(error =>{
                this.setState({
                    isLoading: false,
                    error
                });
            });
    }

    render() {
        const { isLoading, items, error } = this.state;

        return (
            <div className="row">
                <div className="col-md-6">
                    <div className="App">
                        <div className="row">
                            <div className="col-md-3">
                                {isLoading && <div>Loading...</div>}
                                {error && <div>Error : {error}</div>}
                                {!isLoading && !items.length && <h2>No Data</h2>}
                                <ul>
                                    {items.map(({ id, reserve_url, address, price, name }) =>
                                        <li key={id} style={{ listStyleType: "none" }}>
                                            <a
                                                href={reserve_url}
                                                style={{ color: "red" }}
                                                target="_blank"
                                            >
                                                {name}
                                            </a>{" "}
                                            | <span style={{ fontStyle: "italic" }}>{address}</span>{" "}
                                            | Price rating:{" "}
                                            <span style={{ color: "red" }}>{price}</span>
                                        </li>
                                    )}
                                </ul>
                            </div>
                        </div>
                    </div>
                </div>
            </div>
        )
    }
}

ReactDOM.render(<App/>, document.getElementById('root'))
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/16.5.2/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/16.5.2/umd/react-dom.production.min.js"></script>
<div id='root'/>

\$\endgroup\$
  • 1
    \$\begingroup\$ This isn't tagged as an interview question, so what do recruiters have to do with anything? \$\endgroup\$ – Mast Jan 16 at 16:50
  • \$\begingroup\$ Weird, I remembered seeing it somewhere in the question, I'll remove it. \$\endgroup\$ – Treycos Jan 16 at 16:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.