2
\$\begingroup\$

This is my first react app and it was for a job interview challenge. The app is really simple and it works, but although I don't think I'm gonna get it, could I have some feedback about what is wrong and how could I improve it?

***I already submitted this, so is not cheating, I just need to know how can I be better

  • The way requests are being made is correct?
  • Should planetas object be populated this way? Is there a better approach? (thats how I managed to make it work)
  • planetas.films was taking too long to be loaded and giving me a map is undefined problem, is my conditional render bad?
  • And anything else you find to be wrong

service:

import axios from 'axios'

const PLANETS_URL = 'https://swapi.co/api/planets'
const FILMS_URL = 'https://swapi.co/api/films'

const GetPlanets = async () => {
    let planetas = {}

    await axios.get(`${PLANETS_URL}`)
        .then((p) => {

            planetas = p.data.results

            return axios.get(`${FILMS_URL}`)
        }).then((f) => {

        planetas.forEach((pv, i) => {
            pv.films.forEach((pf, j) => {
                f.data.results.forEach((el, k) => {
                    if(pf === el.url){
                        pv.films[j] = el 
                    }
                })
            })
        })
        })        
        .catch((e) => {
            console.error(e);
        })

    return planetas
}

export default GetPlanets

component:

import React, { useState, useEffect } from 'react'
import GetPlanets from '../services'
import { Loader, Container, Name, Info, Button } from '../components'

export default function Planetas () {

  const [ counter, setCounter ] = useState(1)
  const [ planetas, setPlanetas ] = useState([])
  const [ toShow, setToShow ] = useState({})

  useEffect( () => {

    const fetchPlanetas = async () => {
      const planetas = await GetPlanets()

      setPlanetas(planetas)
      setToShow(planetas[0])
    };
    fetchPlanetas()
  }, [])

  const handleClick = () => {
    if(counter === (planetas.length - 1)){
      setCounter(0)
      setToShow(planetas[counter])
    } else {
      setCounter(counter + 1)
      setToShow(planetas[counter])
    }
  }

  if(typeof toShow.films !== 'undefined') {
    return (    
      <Container>
        <Name>{toShow.name}</Name>
        <Info>
        <p>Population: {toShow.population}</p>
        <p>Climate: {toShow.climate}</p>
        <p>Terrain: {toShow.terrain}</p>
        Featured in Films:
        <ul>
        {toShow.films.map((film, key) => (
            <li key={key}>{film.title}</li>
           ))}
        </ul>
        </Info>
        <Button onClick={handleClick}>Next</Button>
      </Container>
     )
  } else {
    return(
      <Loader>Loading...</Loader>
    )
  }
}
\$\endgroup\$
2
\$\begingroup\$
  • Requests could be handled better by first fetching planets and then fetching films, instead of waiting for all data to fetched before render. I would prefer to make a service call to get planets and then fetch films asynchronously after that.
  • As I mentioned above, I think it's better to get planets and then fetch films asynchronously.
  • Since you're looping through all planets and films, I expect it to take long time. In addition, you're waiting for films to be fetched all and then you display planet information. Also, this approach would not be working correct if there was more than one page of films response.
  • You can fetch planet and keep a current index value to show current planet information. And you can fetch featured films of that planet after planet info is rendered. To implement a better structured code, you can also use reducers.

I've implemented two versions, first version is without reducers and second one is with reducers. You can take a look at to get some idea and compare both solutions.

Without reducer: https://codesandbox.io/embed/swapiplanetsfilms-simple-wdtoh

With reducer: https://codesandbox.io/embed/swapiplanetsfilms-reducer-yg44l

\$\endgroup\$
  • \$\begingroup\$ hi, could you explain why "Also, this approach would not be working correct if there was more than one page of films response", please. thank you \$\endgroup\$ – tramada Jul 25 at 17:29
  • 1
    \$\begingroup\$ In your code it's only sending one request to Films API and gets all films because there is only 7 results. If there was more results and next attribute of films API response was not null, then you'd have to send another request to fetch those films too. I hope I made it more clear. \$\endgroup\$ – Ahmet Kapusuz Jul 30 at 7:28

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.