AleksanderDishnica/react_weather

Code review

Willaiem opened this issue · 0 comments

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'
}
  1. 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.
  2. No validation for "city" and "key" parameters. You can pass something wrong and your app will crash because it wasn't expected.
  3. Prefer using "const" over "let".
  4. 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.
  5. 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!