6
\$\begingroup\$

Assignment Description:

Make a web application that compares starships based upon the Star Wars API.

There shall be:

  • A label "Select two Starships from the dropdown lists to compare".

  • Two dropdown-controls filled with starships.

  • Button "Compare".

  • Table displaying the specs (speed, cargo, cost, etc.) of the two starships.

When the user clicks the compare-button the table is filled. Based upon the selected options in the drowdown-controls.

On each row: The cell with the higher value is highlighted.

(() => { 
    function removeHigherValueClass() {
        document.querySelectorAll('.data-cell').forEach((dataCell) => {
            dataCell.classList.remove('higher-value');
        });
    }

    function runGen() {
        let genObj = gen();

        function nextItem(obj) {
            if (obj.done) {
                return obj.value;
            }
            // As long as there a further yield
            //  statements in the generator function ...
            obj.value.then((value) => {
                nextItem(genObj.next(value));
            }).catch((error) => {
                console.log(error);
            });    
        }

        nextItem(genObj.next()); // Trigger the recursion the first time.
    }

    function* gen() {  
        let url = 'https://swapi.co/api/starships/';
        let dataCells = document.querySelectorAll('.data-cell');
        let json = [];

        let oneResponse = yield fetch(
            url + document.getElementById('starShipOne').value, 
            { mode: 'cors' } );
        json.push(yield oneResponse.json());

        let twoResponse = yield fetch(
            url + document.getElementById('starShipTwo').value, 
            { mode: 'cors' } );
        json.push(yield twoResponse.json());

        dataCells.forEach((dataCell) => {
            let key = dataCell.dataset.key; // The more general name ...

            // Starship One == 0, Starship Two == 1
            for (let i = 0; i < 2; i++) {
                document.getElementById(key + i).textContent = json[i][key]; // The general name together with the index gives the concrete ID.
            }

            if (!isNaN(json[0][key])) { // If it is computable, comparable data ...

                if (parseFloat(json[0][key]) > parseFloat(json[1][key])) { // ... then compare the current spec. Class has only to be attached if one value is really higher. No attachment when equal.
                    document.getElementById(key + 0)
                            .classList.add('higher-value');
                } else if (parseFloat(json[1][key]) > parseFloat(json[0][key])) {
                    document.getElementById(key + 1)
                            .classList.add('higher-value');
                }

            }
        });
    }   

    document
        .getElementById('compare')
        .addEventListener('click', () => {
            removeHigherValueClass();
            runGen();
        });
})();
html {
            background-color: rgba(245, 245, 245, .5);
        }

        .higher-value {
            background-color: pink;
            font-weight: 900;
        }

        .main-wrap {
            max-width: 800px;
            margin: 40px auto;
        }

        .main-nav {
            margin: 0 0 30px;
        }

        .data-cell {
            text-align: center;
            width: 200px;
        }
<section>
        <div class="main-wrap">
            <h3>Select two Starships from the dropdown lists to compare</h3>
            <nav class="main-nav">
                <select id="starShipOne">
                    <option value="2" selected="selected">CR90 Corvette</option>
                    <option value="75">V-wing</option>
                    <option value="74">Belbullab-22 Starfighter</option>
                    <option value="65">Jedi Interceptor</option>

                    <option value="3">Star Destroyer</option>
                    <option value="59">Trade Fedaration Cruiser</option>
                    <option value="58">Solar Sailer</option>
                    <option value="63">Republic Attack Cruiser</option>

                    <option value="28">A-wing</option>
                    <option value="29">B-wing</option>
                    <option value="39">Naboo Fighter</option>
                    <option value="10">Millenium Falcon</option>
                </select>
                <select id="starShipTwo">
                    <option value="2">CR90 Corvette</option>
                    <option value="75" selected="selected">V-wing</option>
                    <option value="74">Belbullab-22 Starfighter</option>
                    <option value="65">Jedi Interceptor</option>

                    <option value="3">Star Destroyer</option>
                    <option value="59">Trade Fedaration Cruiser</option>
                    <option value="58">Solar Sailer</option>
                    <option value="63">Republic Attack Cruiser</option>

                    <option value="28">A-wing</option>
                    <option value="29">B-wing</option>
                    <option value="39">Naboo Fighter</option>
                    <option value="10">Millenium Falcon</option>
                </select>
                <button id="compare">Compare</button>
            </nav>
            <table border="1">
                <tr>
                    <th></th>
                    <th>Starship 1</th>
                    <th>Starship 2</th>
                </tr>
                <tr>
                    <td>Name</td>
                    <!-- 'data-key' will later determine the row. 
                         id='data-key-value[ 0 || 1 ]' determines the cell -->
                    <td class="data-cell" data-key="name" id="name0"></td>
                    <td class="data-cell" data-key="name" id="name1"></td>
                </tr>
                <tr>
                    <td>Cost</td>
                    <td class="data-cell" data-key="cost_in_credits" id="cost_in_credits0"></td>
                    <td class="data-cell" data-key="cost_in_credits" id="cost_in_credits1"></td>
                </tr>
                <tr>
                    <td>Speed</td>
                    <td class="data-cell" data-key="max_atmosphering_speed" id="max_atmosphering_speed0"></td>
                    <td class="data-cell" data-key="max_atmosphering_speed" id="max_atmosphering_speed1"></td>
                </tr>
                <tr>
                    <td>Cargo Size</td>
                    <td class="data-cell" data-key="cargo_capacity" id="cargo_capacity0"></td>
                    <td class="data-cell" data-key="cargo_capacity" id="cargo_capacity1"></td>
                </tr>
                <tr>
                    <td>Passengers</td>
                    <td class="data-cell" data-key="passengers" id="passengers0"></td>
                    <td class="data-cell" data-key="passengers" id="passengers1"></td>
                </tr>
            </table>
        </div>
    </section>

What do you think about my idea with the general name-attribute and the specific ID? Is there are better solution to this? Is it good practice at all to depend so much on HTML-attributes?

What do you think about the implementation in general?

All hints, recommendations and comments are appreciated.

\$\endgroup\$
  • 3
    \$\begingroup\$ What's the source of this challenge? That's the second time I see someone with a solution for it on this site without a link to the original challenge. \$\endgroup\$ – Mast Nov 19 '17 at 14:52
  • 2
    \$\begingroup\$ It‘s part of an edX MOOC : edx.org/course/… \$\endgroup\$ – michael.zech Nov 20 '17 at 2:01
3
\$\begingroup\$

Feedback

That's a neat little application for multiple reasons:

  • It uses a Star Wars API
  • It uses a generator function

I like it! I did however find some inefficiencies - see the section below labeled Excess Looping.

Suggestions

Cache DOM elements on load

DOM lookups aren't cheap, so it is generally best to store references to them in constants. For your application, the select lists and the elements with class name data-cell could be stored in constants, and then those constants could be used in place of the current DOM lookups

Use getElementsByClassName() instead of querySelectorAll(), getElementById() instead of querySelector()

In most browsers it would generally be quicker to fetch the elements with class name data-cell using document.getElementsByClassName(), but that would return a live collection (refer to this SO answer for an explanation). It may be best to use the spread operator to put that collection in an array (or else use Array.from()) to allow use of array methods like forEach:

const dataCells = [...document.getElementsByClassName('data-cell')];

Excess looping

The for loop within the forEach is actually setting the text content of the cells twice as many times as necessary. Instead of doing the following:

dataCells.forEach((dataCell) => {
      let key = dataCell.dataset.key; // The more general name ...
      for (let i = 0; i < 2; i++) {
          document.getElementById(key + i).textContent = json[i][key]; // The general name together with the index gives the concrete ID.
      }

There is already a reference to the data cell, so add the index parameter to the callback function and use that to reference with the modulo operator:

dataCells.forEach((dataCell, i) => {
    let key = dataCell.dataset.key; // The more general name ...
    dataCell.textContent = json[i%2][key];

And the condition for adding the higher-value class can be simplified to:

if (parseFloat(json[i%2][key]) > parseFloat(json[(i+1)%2][key])) { // ... then compare the current spec. Class has only to be attached if one value is really higher. No attachment when equal.
    dataCell.classList.add('higher-value');
} 

The else block can be removed as well.

SelectListElement.value

I haven't heard much on this front lately but typically to get the value of the selected option in a select list, one would need to use the selectedIndex property, like this:

const starShipOne = document.getElementById('starShipOne');
const starShipOneValue = starShipOne.options[starShipOne.selectedIndex].value;

Unfortunately the MDN page for HTMLSelectElement.value yields a Page Not Found message and none of the archive results appear to be any different. Perhaps the only browsers that don't support .value on the selectList element also don't support so it is a moot point.

EDIT

Now that I think about it, it feels like the current implementation of gen has multiple responsibilities:

  • fetching results from the API
  • updating the DOM

I know this isn't a large O.O. application but it would be advisable to separate the functionality that updates the DOM into a separate function - which would be in line with the Single Responsibility Principle. In order for that to work, the promises will need to be returned from the generator function and the recursive function. See the updated code below for an illustration of this.

Updated code

(() => {
  const dataCells = [...document.getElementsByClassName('data-cell')];
  const starShipOne = document.getElementById('starShipOne');
  const starShipTwo = document.getElementById('starShipTwo');

  function removeHigherValueClass() {
    dataCells.forEach((dataCell) => {
      dataCell.classList.remove('higher-value');
    });
  }

  function runGen() {
    let genObj = gen();

    function nextItem(obj) {
      if (obj.done) {
        return obj.value;
      }
      // As long as there a further yield
      //  statements in the generator function ...
      return obj.value.then((value) => nextItem(genObj.next(value))).catch(console.log);
    }

    nextItem(genObj.next()).then(updateDOM); // Trigger the recursion the first time.
  }

  function* gen() {
    let url = 'https://swapi.co/api/starships/';
    let json = [];
    let oneResponse = yield fetch(
      url + starShipOne.value, {
        mode: 'cors'
      });
    json.push(yield oneResponse.json());

    let twoResponse = yield fetch(
      url + starShipTwo.value, {
        mode: 'cors'
      });
    json.push(yield twoResponse.json());
    return json;
  }
  function updateDOM(json) {

    dataCells.forEach((dataCell, i) => {
      let key = dataCell.dataset.key; // The more general name ...
      dataCell.textContent = json[i % 2][key];
      // Starship One == 0, Starship Two == 1
      if (!isNaN(json[0][key])) { // If it is computable, comparable data ...

        if (parseFloat(json[i % 2][key]) > parseFloat(json[(i + 1) % 2][key])) { // ... then compare the current spec. Class has only to be attached if one value is really higher. No attachment when equal.
          dataCell.classList.add('higher-value');
        }
      }
    });
  }

  document
    .getElementById('compare')
    .addEventListener('click', () => {
      removeHigherValueClass();
      runGen();
    });
})();
html {
  background-color: rgba(245, 245, 245, .5);
}

.higher-value {
  background-color: pink;
  font-weight: 900;
}

.main-wrap {
  max-width: 800px;
  margin: 40px auto;
}

.main-nav {
  margin: 0 0 30px;
}

.data-cell {
  text-align: center;
  width: 200px;
}
<section>
  <div class="main-wrap">
    <h3>Select two Starships from the dropdown lists to compare</h3>
    <nav class="main-nav">
      <select id="starShipOne">
        <option value="2" selected="selected">CR90 Corvette</option>
        <option value="75">V-wing</option>
        <option value="74">Belbullab-22 Starfighter</option>
        <option value="65">Jedi Interceptor</option>

        <option value="3">Star Destroyer</option>
        <option value="59">Trade Fedaration Cruiser</option>
        <option value="58">Solar Sailer</option>
        <option value="63">Republic Attack Cruiser</option>

        <option value="28">A-wing</option>
        <option value="29">B-wing</option>
        <option value="39">Naboo Fighter</option>
        <option value="10">Millenium Falcon</option>
      </select>
      <select id="starShipTwo">
        <option value="2">CR90 Corvette</option>
        <option value="75" selected="selected">V-wing</option>
        <option value="74">Belbullab-22 Starfighter</option>
        <option value="65">Jedi Interceptor</option>

        <option value="3">Star Destroyer</option>
        <option value="59">Trade Fedaration Cruiser</option>
        <option value="58">Solar Sailer</option>
        <option value="63">Republic Attack Cruiser</option>

        <option value="28">A-wing</option>
        <option value="29">B-wing</option>
        <option value="39">Naboo Fighter</option>
        <option value="10">Millenium Falcon</option>
      </select>
      <button id="compare">Compare</button>
    </nav>
    <table border="1">
      <tr>
        <th></th>
        <th>Starship 1</th>
        <th>Starship 2</th>
      </tr>
      <tr>
        <td>Name</td>
        <!-- 'data-key' will later determine the row. 
                         id='data-key-value[ 0 || 1 ]' determines the cell -->
        <td class="data-cell" data-key="name" id="name0"></td>
        <td class="data-cell" data-key="name" id="name1"></td>
      </tr>
      <tr>
        <td>Cost</td>
        <td class="data-cell" data-key="cost_in_credits" id="cost_in_credits0"></td>
        <td class="data-cell" data-key="cost_in_credits" id="cost_in_credits1"></td>
      </tr>
      <tr>
        <td>Speed</td>
        <td class="data-cell" data-key="max_atmosphering_speed" id="max_atmosphering_speed0"></td>
        <td class="data-cell" data-key="max_atmosphering_speed" id="max_atmosphering_speed1"></td>
      </tr>
      <tr>
        <td>Cargo Size</td>
        <td class="data-cell" data-key="cargo_capacity" id="cargo_capacity0"></td>
        <td class="data-cell" data-key="cargo_capacity" id="cargo_capacity1"></td>
      </tr>
      <tr>
        <td>Passengers</td>
        <td class="data-cell" data-key="passengers" id="passengers0"></td>
        <td class="data-cell" data-key="passengers" id="passengers1"></td>
      </tr>
    </table>
  </div>
</section>

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

I also noticed some variables can be declared with const instead of let - like key, since it is never re-assigned within the forEach callback.

dataCells.forEach((dataCell) => {
  const key = dataCell.dataset.key; // The more general name ...

You could abstract out the URL to the top, as a constant (or else in a configuration file). That way if the URL ever changes, you can do it in one spot, instead of having to find it mixed in with the code.

const API_URL = 'https://swapi.co/api/starships/';

Similarly, the options for fetch() could be stored in a constant

  const fetchOptions = {mode: 'cors'};

Another option to simplify things is to use Promise.all()` to run the two API requests concurrently, and then to get the JSON from each response.

const API_URL = 'https://swapi.co/api/starships/';
(() => {
  const dataCells = [...document.getElementsByClassName('data-cell')];
  const starShipOne = document.getElementById('starShipOne');
  const starShipTwo = document.getElementById('starShipTwo');

  function removeHigherValueClass() {
    dataCells.forEach((dataCell) => {
      dataCell.classList.remove('higher-value');
    });
  }
  const fetchOptions = {mode: 'cors'};
  function getData() {
    const oneResponse = fetch(API_URL + starShipOne.value, fetchOptions);

    const twoResponse = fetch(API_URL + starShipTwo.value, fetchOptions);
    return Promise.all([oneResponse, twoResponse]);
  }
  function getJSONFromResponses(responses) {
    return Promise.all(responses.map(response => response.json()));
  }
  function updateDOM(json) {
    dataCells.forEach((dataCell, i) => {
      const key = dataCell.dataset.key; // The more general name ...
      dataCell.textContent = json[i % 2][key];
      // Starship One == 0, Starship Two == 1
      if (!isNaN(json[0][key])) { // If it is computable, comparable data ...

        if (parseFloat(json[i % 2][key]) > parseFloat(json[(i + 1) % 2][key])) { // ... then compare the current spec. Class has only to be attached if one value is really higher. No attachment when equal.
          dataCell.classList.add('higher-value');
        }
      }
    });
  }

  document
    .getElementById('compare')
    .addEventListener('click', () => {
      removeHigherValueClass();
      getData().then(getJSONFromResponses).then(updateDOM);
    });
})();
html {
  background-color: rgba(245, 245, 245, .5);
}

.higher-value {
  background-color: pink;
  font-weight: 900;
}

.main-wrap {
  max-width: 800px;
  margin: 40px auto;
}

.main-nav {
  margin: 0 0 30px;
}

.data-cell {
  text-align: center;
  width: 200px;
}
<section>
  <div class="main-wrap">
    <h3>Select two Starships from the dropdown lists to compare</h3>
    <nav class="main-nav">
      <select id="starShipOne">
        <option value="2" selected="selected">CR90 Corvette</option>
        <option value="75">V-wing</option>
        <option value="74">Belbullab-22 Starfighter</option>
        <option value="65">Jedi Interceptor</option>

        <option value="3">Star Destroyer</option>
        <option value="59">Trade Fedaration Cruiser</option>
        <option value="58">Solar Sailer</option>
        <option value="63">Republic Attack Cruiser</option>

        <option value="28">A-wing</option>
        <option value="29">B-wing</option>
        <option value="39">Naboo Fighter</option>
        <option value="10">Millenium Falcon</option>
      </select>
      <select id="starShipTwo">
        <option value="2">CR90 Corvette</option>
        <option value="75" selected="selected">V-wing</option>
        <option value="74">Belbullab-22 Starfighter</option>
        <option value="65">Jedi Interceptor</option>

        <option value="3">Star Destroyer</option>
        <option value="59">Trade Fedaration Cruiser</option>
        <option value="58">Solar Sailer</option>
        <option value="63">Republic Attack Cruiser</option>

        <option value="28">A-wing</option>
        <option value="29">B-wing</option>
        <option value="39">Naboo Fighter</option>
        <option value="10">Millenium Falcon</option>
      </select>
      <button id="compare">Compare</button>
    </nav>
    <table border="1">
      <tr>
        <th></th>
        <th>Starship 1</th>
        <th>Starship 2</th>
      </tr>
      <tr>
        <td>Name</td>
        <!-- 'data-key' will later determine the row. 
                         id='data-key-value[ 0 || 1 ]' determines the cell -->
        <td class="data-cell" data-key="name" id="name0"></td>
        <td class="data-cell" data-key="name" id="name1"></td>
      </tr>
      <tr>
        <td>Cost</td>
        <td class="data-cell" data-key="cost_in_credits" id="cost_in_credits0"></td>
        <td class="data-cell" data-key="cost_in_credits" id="cost_in_credits1"></td>
      </tr>
      <tr>
        <td>Speed</td>
        <td class="data-cell" data-key="max_atmosphering_speed" id="max_atmosphering_speed0"></td>
        <td class="data-cell" data-key="max_atmosphering_speed" id="max_atmosphering_speed1"></td>
      </tr>
      <tr>
        <td>Cargo Size</td>
        <td class="data-cell" data-key="cargo_capacity" id="cargo_capacity0"></td>
        <td class="data-cell" data-key="cargo_capacity" id="cargo_capacity1"></td>
      </tr>
      <tr>
        <td>Passengers</td>
        <td class="data-cell" data-key="passengers" id="passengers0"></td>
        <td class="data-cell" data-key="passengers" id="passengers1"></td>
      </tr>
    </table>
  </div>
</section>


I know this was originally tagged with but I've been learning more about the features like the keyword await and async functions and after having used them a bit realize that they could be used to simplify the code here immensely. For example, instead of using the generator function, you could make it a regular function (but with the async keyword before it) and just add await before each asynchronous request:

async function getData() {
    const oneResponse = await fetch(API_URL + starShipOne.value, fetchOptions);
    const firstJSON = await oneResponse.json();

    const twoResponse = await fetch(API_URL + starShipTwo.value, fetchOptions);
    const secondJSON = await twoResponse.json();
    return [firstJSON, secondJSON];
}

Then there is no need to have the code that calls .next() on the iterator. Instead, just call .then(updateDOM):

getData().then(updateDOM);

See rewritten code using those updates

const API_URL = 'https://swapi.co/api/starships/';
(() => {
  const dataCells = [...document.getElementsByClassName('data-cell')];
  const starShipOne = document.getElementById('starShipOne');
  const starShipTwo = document.getElementById('starShipTwo');

  function removeHigherValueClass() {
    dataCells.forEach((dataCell) => {
      dataCell.classList.remove('higher-value');
    });
  }
  const fetchOptions = {mode: 'cors'};
  async function getData() {
    const oneResponse = await fetch(API_URL + starShipOne.value, fetchOptions);
    const firstJSON = await oneResponse.json();

    const twoResponse = await fetch(API_URL + starShipTwo.value, fetchOptions);
    const secondJSON = await twoResponse.json();
    return [firstJSON, secondJSON];
  }
  function updateDOM(json) {
    dataCells.forEach((dataCell, i) => {
      const key = dataCell.dataset.key; // The more general name ...
      dataCell.textContent = json[i % 2][key];
      // Starship One == 0, Starship Two == 1
      if (!isNaN(json[0][key])) { // If it is computable, comparable data ...

        if (parseFloat(json[i % 2][key]) > parseFloat(json[(i + 1) % 2][key])) { // ... then compare the current spec. Class has only to be attached if one value is really higher. No attachment when equal.
          dataCell.classList.add('higher-value');
        }
      }
    });
  }

  document
    .getElementById('compare')
    .addEventListener('click', () => {
      removeHigherValueClass();
      getData().then(updateDOM);
    });
})();
html {
  background-color: rgba(245, 245, 245, .5);
}

.higher-value {
  background-color: pink;
  font-weight: 900;
}

.main-wrap {
  max-width: 800px;
  margin: 40px auto;
}

.main-nav {
  margin: 0 0 30px;
}

.data-cell {
  text-align: center;
  width: 200px;
}
<section>
  <div class="main-wrap">
    <h3>Select two Starships from the dropdown lists to compare</h3>
    <nav class="main-nav">
      <select id="starShipOne">
        <option value="2" selected="selected">CR90 Corvette</option>
        <option value="75">V-wing</option>
        <option value="74">Belbullab-22 Starfighter</option>
        <option value="65">Jedi Interceptor</option>

        <option value="3">Star Destroyer</option>
        <option value="59">Trade Fedaration Cruiser</option>
        <option value="58">Solar Sailer</option>
        <option value="63">Republic Attack Cruiser</option>

        <option value="28">A-wing</option>
        <option value="29">B-wing</option>
        <option value="39">Naboo Fighter</option>
        <option value="10">Millenium Falcon</option>
      </select>
      <select id="starShipTwo">
        <option value="2">CR90 Corvette</option>
        <option value="75" selected="selected">V-wing</option>
        <option value="74">Belbullab-22 Starfighter</option>
        <option value="65">Jedi Interceptor</option>

        <option value="3">Star Destroyer</option>
        <option value="59">Trade Fedaration Cruiser</option>
        <option value="58">Solar Sailer</option>
        <option value="63">Republic Attack Cruiser</option>

        <option value="28">A-wing</option>
        <option value="29">B-wing</option>
        <option value="39">Naboo Fighter</option>
        <option value="10">Millenium Falcon</option>
      </select>
      <button id="compare">Compare</button>
    </nav>
    <table border="1">
      <tr>
        <th></th>
        <th>Starship 1</th>
        <th>Starship 2</th>
      </tr>
      <tr>
        <td>Name</td>
        <!-- 'data-key' will later determine the row. 
                         id='data-key-value[ 0 || 1 ]' determines the cell -->
        <td class="data-cell" data-key="name" id="name0"></td>
        <td class="data-cell" data-key="name" id="name1"></td>
      </tr>
      <tr>
        <td>Cost</td>
        <td class="data-cell" data-key="cost_in_credits" id="cost_in_credits0"></td>
        <td class="data-cell" data-key="cost_in_credits" id="cost_in_credits1"></td>
      </tr>
      <tr>
        <td>Speed</td>
        <td class="data-cell" data-key="max_atmosphering_speed" id="max_atmosphering_speed0"></td>
        <td class="data-cell" data-key="max_atmosphering_speed" id="max_atmosphering_speed1"></td>
      </tr>
      <tr>
        <td>Cargo Size</td>
        <td class="data-cell" data-key="cargo_capacity" id="cargo_capacity0"></td>
        <td class="data-cell" data-key="cargo_capacity" id="cargo_capacity1"></td>
      </tr>
      <tr>
        <td>Passengers</td>
        <td class="data-cell" data-key="passengers" id="passengers0"></td>
        <td class="data-cell" data-key="passengers" id="passengers1"></td>
      </tr>
    </table>
  </div>
</section>

| improve this answer | |
\$\endgroup\$

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.