Using a public API to display data

Question

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;

Answer 1

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'/>

• Mast: This isn't tagged as an interview question, so what do recruiters have to do with anything?
• Treycos: Weird, I remembered seeing it somewhere in the question, I'll remove it.