Code review
Willaiem opened this issue · 0 comments
Willaiem commented
Hi! I saw your comment on Reactiflux's Discord server and I decided to check out your code.
App,js
async function makeGetRequest(city, key) {
2. The
// Create an api key at https://api.openweathermap.org
let res = await axios.get('https://api.openweathermap.org/data/2.5/weather?q='+city+'&units=metric&appid=');
document.getElementsByTagName('img')[key].src = 'http://openweathermap.org/img/wn/'+res.data.weather[0].icon+'@2x.png'
document.getElementsByClassName('city')[key].innerHTML = city
document.getElementsByClassName('temp')[key].innerHTML = res.data.main.temp + ' °C'
}
- This could be a custom hook - for example, check out this video: https://www.youtube.com/watch?v=6ThXsUwLWvc
Then create it and insert it in a different file. - No validation for "city" and "key" parameters. You can pass something wrong and your app will crash because it wasn't expected.
- Prefer using "const" over "let".
- The URL you provided to
axios.get
could be a variable with decent naming. And replace the string with template literal - this will make your URL more readable. - Never, ever do any direct DOM operation like this in React (or any JavaScript framework). Also, the Weather component seems a bit wrong by doing these operations.
In my opinion, this could look like this:
const useWeather = (city, key) => {
// validate the city and key params
try {
const weatherDetails = await axios.get(`https://api.openweathermap.org/data/2.5/weather?q=${city}&units=metric&appid=`);
// it would be awesome to make validation if this response is valid.
return {weatherDetails, error: null}
} catch (error) {
return {weatherDetails: null, error}
}
}
const Weather = ({city, key}) => {
const {weatherDetails, error} = useWeather(city, key)
return error ? (
<div>
<h1>Something went wrong...</h1>
<p>{error.message}</p>
</div>
) : (
<div>
<h3 className='city city_a'>{weatherDetails.city}</h3>
<h1 className='temp'>{weatherDetails.temp}</h1>
<div className='img_div'>
<img className='main_img' src={weatherDetails.src} />
<div>Today</div>
</div>
</div>
)
}
Also these:
// why?
makeGetRequest('London', 0)
// this could be in different file.
const cities = [
'Rome',
'Milan',
'Barcelona',
'Paris',
'Madrid',
'Oslo'
]
// also why?
for(let i = 0; i < cities.length; i++){
makeGetRequest(cities[i], i+1)
}
- you could remove the App.test.js, it's completely unneccesary.
Overall, there is still a lot to learn. I can see that there are your first steps in React, that's why you made these mistakes.
I recommend checking out my comment with two, really useful resources (that are 100% free!)
https://dev.to/willaiem/comment/1k169
(and you can check out the post I commented)
Cheers!