Using a public API to display data
$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;
javascript react.js
$endgroup$
add a comment |
$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;
javascript react.js
$endgroup$
5
$begingroup$
This is the most generic title possible, applicable to all questions on this site. See How to Ask.
$endgroup$
– 200_success
14 hours ago
add a comment |
$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;
javascript react.js
$endgroup$
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;
javascript react.js
javascript react.js
edited 3 mins ago
Jamal♦
30.3k11116226
30.3k11116226
asked 14 hours ago
legoMyEgolegoMyEgo
142
142
5
$begingroup$
This is the most generic title possible, applicable to all questions on this site. See How to Ask.
$endgroup$
– 200_success
14 hours ago
add a comment |
5
$begingroup$
This is the most generic title possible, applicable to all questions on this site. See How to Ask.
$endgroup$
– 200_success
14 hours ago
5
5
$begingroup$
This is the most generic title possible, applicable to all questions on this site. See How to Ask.
$endgroup$
– 200_success
14 hours ago
$begingroup$
This is the most generic title possible, applicable to all questions on this site. See How to Ask.
$endgroup$
– 200_success
14 hours ago
add a comment |
1 Answer
1
active
oldest
votes
$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
13 hours ago
$begingroup$
Weird, I remembered seeing it somewhere in the question, I'll remove it.
$endgroup$
– Treycos
13 hours ago
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f211626%2fusing-a-public-api-to-display-data%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
$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
13 hours ago
$begingroup$
Weird, I remembered seeing it somewhere in the question, I'll remove it.
$endgroup$
– Treycos
13 hours ago
add a comment |
$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
13 hours ago
$begingroup$
Weird, I remembered seeing it somewhere in the question, I'll remove it.
$endgroup$
– Treycos
13 hours ago
add a comment |
$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$
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'/>
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'/>
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'/>
edited 9 hours ago
answered 14 hours ago
TreycosTreycos
245110
245110
1
$begingroup$
This isn't tagged as an interview question, so what do recruiters have to do with anything?
$endgroup$
– Mast
13 hours ago
$begingroup$
Weird, I remembered seeing it somewhere in the question, I'll remove it.
$endgroup$
– Treycos
13 hours ago
add a comment |
1
$begingroup$
This isn't tagged as an interview question, so what do recruiters have to do with anything?
$endgroup$
– Mast
13 hours ago
$begingroup$
Weird, I remembered seeing it somewhere in the question, I'll remove it.
$endgroup$
– Treycos
13 hours ago
1
1
$begingroup$
This isn't tagged as an interview question, so what do recruiters have to do with anything?
$endgroup$
– Mast
13 hours ago
$begingroup$
This isn't tagged as an interview question, so what do recruiters have to do with anything?
$endgroup$
– Mast
13 hours ago
$begingroup$
Weird, I remembered seeing it somewhere in the question, I'll remove it.
$endgroup$
– Treycos
13 hours ago
$begingroup$
Weird, I remembered seeing it somewhere in the question, I'll remove it.
$endgroup$
– Treycos
13 hours ago
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f211626%2fusing-a-public-api-to-display-data%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
5
$begingroup$
This is the most generic title possible, applicable to all questions on this site. See How to Ask.
$endgroup$
– 200_success
14 hours ago