4
\$\begingroup\$

I've been studying javascript for the love of it, I'm currently learning how to use async functions and my code actually works, but I know there is always room for improving.

I'm open to any help regarding my use of async functions and how I use the return of sizeJokesArray() in fetchJokes().

const jokeContainer = document.querySelector('.joke-container');

async function sizeJokesArray() {
  let url = 'https://api.icndb.com/jokes/count';
  let data = await (await fetch(url)).json();
  return data.value;
}

async function fetchJokes() {
  let url = `https://api.icndb.com/jokes/random/${length}`;
  let jokesData = [];
  let data = await (await fetch(url)).json();
  for (jokePosition in data.value) {
    jokesData.push(data.value[jokePosition].joke);
  }
  return jokesData;
}

sizeJokesArray().then(size => (length = size)); // return of sizeJokesArray

jokeContainer.addEventListener('click', event => {
  if (event.target.value === 'Fetch') {
    fetchJokes(length).then(jokesData => (jokesArray = jokesData)).then(jokesData => (console.log(jokesData)));
  }
});
<div class="joke-container">
    <div class="joke-text">
        <p>Fetch some jokes bro!!</p>
    </div>
    <div class="joke-controls">
        <button type="button" value="Fetch">Fetch Jokes</button>
    </div>
</div>

the existing console.log() it's not actually used in my code just used it here to log my results for the question purpose.

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

I've improved a few things to your script, making it more consistent over all:

;(function () {
  const jokeContainer = document.querySelector('.joke-container')

  async function fetchCount() {
    const url = 'https://api.icndb.com/jokes/count'
    const { value } = await (await fetch(url)).json()

    return value
  }

  async function fetchJokes(length = fetchCount()) {
    const url = `https://api.icndb.com/jokes/random/${await length}`
    const { value } = await (await fetch(url)).json()

    return value.map(({ joke }) => joke)
  }

  jokeContainer.addEventListener('click', async event => {
    if (event.target.value === 'Fetch') {
      const jokes = await fetchJokes()

      console.log(jokes)
    }
  })
}())
<div class="joke-container">
  <div class="joke-text">
    <p>Fetch some jokes bro!!</p>
  </div>
  <div class="joke-controls">
    <button type="button" value="Fetch">Fetch Jokes</button>
  </div>
</div>

  • Wrapped your script in an IIFE to avoid polluting the global scope.
  • Removed your bizarre usage of the global length variable, which you defined without using var, let, or const, and is considered bad practice.
  • Used object destructuring to improve the readability of your code in some places.
  • Declared your event listener as async in order to take advantage of await instead of using a then() chain to improve consistency and readability.
  • Converted your imperative initialization of jokesData into a functional idiomatic approach using Array#map() in order to further improve readability.
\$\endgroup\$
4
  • 1
    \$\begingroup\$ is this const { value } = await (await fetch(url)).json() object destructuring or what is this doing? it blew my mind! \$\endgroup\$ Sep 12, 2017 at 2:03
  • 2
    \$\begingroup\$ Yes, it's the same as const value = (await (await fetch(url)).json()).value \$\endgroup\$ Sep 12, 2017 at 2:44
  • 1
    \$\begingroup\$ Learned more from your answer then like the past month studying and reading about .then(), await, async and such... Thanks now ill read about the last point you mentioned. \$\endgroup\$ Sep 12, 2017 at 3:51
  • \$\begingroup\$ @ricardoNava it's called the destructuring assignment, but it's also used in the argument of the Array#map() callback in this answer as well. \$\endgroup\$ Sep 12, 2017 at 3:53
0
\$\begingroup\$

Here you can improve.

sizeJokesArray().then(size => (length = size)); // return of sizeJokesArray

jokeContainer.addEventListener('click', event => {
  if (event.target.value === 'Fetch') {
    fetchJokes(length).then(jokesData => (jokesArray = jokesData)).then(jokesData => (console.log(jokesData)));
  }
});

You can change it to

let jokes = [];
jokeContainer.addEventListener('click', event => {
  if (event.target.value === 'Fetch') {
    try {
         let length = await sizeJokesArray(); // You dont get ".then" here where you were assigning length variable, if you are using async/await. Use it properly, there is no use of an async function if you are calling it in promise fashion.
         jokes = await fetchJokes(length) // Isn't it easy and clean to read that jokes are meant to be fetch and we are waiting for it.
         console.log(jokes); 
    } catch (err) {
        // Handle API error here
    }
  }
});

When you use async/await, Use it properly everywhere. instead of calling then() you should be awaiting it. also handle error via try/catch

\$\endgroup\$
8
  • \$\begingroup\$ so ... why is this an improvement? Elaborate just a tad here ;) \$\endgroup\$
    – Vogel612
    Aug 28, 2017 at 13:46
  • \$\begingroup\$ Why would I want to have my variables in the global scope? \$\endgroup\$ Aug 28, 2017 at 19:23
  • \$\begingroup\$ and, using let in the global scope is kinda pointless \$\endgroup\$ Aug 31, 2017 at 20:46
  • \$\begingroup\$ You dont want to use var because its more pointless. I am assuming that this function is within some module and not global. also if you use const you cant change its reference, thus you have to stick with var in this scenario \$\endgroup\$ Sep 1, 2017 at 5:53
  • \$\begingroup\$ @Iwrestledabearonce using let in global scope is not pointless. Unlike the respective var declaration, the variable name is not accessible on the window object. So given a top-level let foo = 'bar';, window.foo does not exist. \$\endgroup\$ Jun 20, 2019 at 14:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.