Feedback homework week 2

Hi Aya, here is my feedback on your week 2 homework.

Your week 2 homework looks fine, and the code is a pleasure to look at. I think you covered all the issues I identified in my previous feedback.

The comments I'm going to make are about details to further improve already excellent code.

1. At some point in the lectures, I introduced a slightly more advanced version of the createAndAppend utility function:

function createAndAppend(name, parent, options = {}) {
  const elem = document.createElement(name);
  parent.appendChild(elem);
  Object.keys(options).forEach(key => {
    const value = options[key];
    if (key === 'html') {
      elem.innerHTML = value;
    } else {
      elem.setAttribute(key, value);
    }
  });
  return elem;
}

It extends the functionality through an optional, third parameter, which should be an object specifying the innerHTML and/or the attributes that you want to set for the new HTML element. It can make your code a bit more concise and reduce the need to introduce intermediate variables. For instance, you could rewrite your getDetails() function as follows:

function getDetails(repo) {
  const ulInfo = document.getElementById('infoUl');
  ulInfo.innerHTML = '';

  const li = createAndAppend('li', ulInfo, { html: 'URL: ' });
  createAndAppend('a', li, {
    html: repo.html_url,
    href: repo.html_url,
    id: 'ali',
    target: '_blank'
  });
  createAndAppend('li', ulInfo, { html: 'Name : ' + repo.name });
  createAndAppend('li', ulInfo, { html: 'Description : ' + repo.description });
  createAndAppend('li', ulInfo, { html: 'Forks : ' + repo.forks });
  createAndAppend('li', ulInfo, { html: 'Updated : ' + repo.updated_at });

  getContributors(repo.contributors_url);
}

The code is shorter and there is no longer a need to introduce variables li1, li2, li3 and li4. (Note that instead of using textContent I've used innerHTML. Technically, there is a difference between the two, but in the way we use it here it makes no difference).

You can apply the same principle in other places where you use createAndAppend(), e.g. in function main().

I wrote this function createAndAppend() because I noticed the repetition we were doing with createElement() and appendChild(). Then later, I realised that setting attributes was also repetitively done in a couple of places and that I could improve createAndAppend() to the current version that accepts this optional third parameter. I'm telling you this to show how you should be looking out for ways to make your code simpler and more concise, to make it easier for yourself and reduce the chance for error.

2. This code snippet comes from your function main():

fetchJSON(url)
    .then(data => {
        setupSelect(data);
    })
    .catch(error => renderError(error));

In the then() method you has passed a (fat arrow) function that takes a single parameter and executes the function setupSelect() passing that same parameter. By setupSelect() itself is a function that takes a single parameter. The same holds true for renderError(). So, you could simply rewrite this snippet as follows:

fetchJSON(url)
  .then(setupSelect)
  .catch(renderError);

3. As per my feedback, you replaced some for loops with forEach. But you are not taking full advantage of the forEach construct. The (fat arrow) function that you pass as the argument to forEach() receives as its first argument, the 'next' array element. If we look at your getContributors() function, we get contributor as the first argument. This contributor is the same as contributors[i].

function getContributors(url) {
    const ulImg = document.getElementById('imgUl');
    ulImg.innerHTML = '';
    fetchJSON(url)
        .then(contributors => {
            contributors.forEach((contributor, i) => {
                const el = createAndAppend('li', ulImg);
                el.setAttribute('class', 'element');

                const contributorImg = createAndAppend('img', el);
                contributorImg.setAttribute('src', contributors[i].avatar_url);

                const contributorName = createAndAppend('div', el);
                contributorName.innerHTML = contributors[i].login;
                contributorName.setAttribute('id', 'contributorName');

                const contributorCounter = createAndAppend('div', el);
                contributorCounter.innerHTML = contributors[i].contributions;
                contributorCounter.setAttribute('id', 'contributionsCounter');
            });
        })
        .catch(error => renderError(error));
}

This becomes self-evident if you could peek inside the implementation of forEach. It will look like this (since you now know about this and prototype):

Array.prototype.forEach = function (fn) {
  for (let i = 0; i < this.length; i++) {
    fn(this[i], i, this);
  }
};

So your code could be simplified (including the improved createAndAppend) to:

function getContributors(url) {
  const ulImg = document.getElementById('imgUl');
  ulImg.innerHTML = '';
  fetchJSON(url)
    .then(contributors => {
      contributors.myForEach(contributor => {
        const el = createAndAppend('li', ulImg, { class: 'element' });
        createAndAppend('img', el, { src: contributor.avatar_url });
        createAndAppend('div', el, {
          html: contributor.login,
          id: 'contributorName'
        });
        createAndAppend('div', el, {
          html: contributor.contributions,
          id: 'contributionsCounter'
        });
      });
    })
    .catch(renderError);
}

Finally, most JavaScript developers now prefer two spaces instead of four to indent their code. If you would like to try that, in VSCode you can do that by adding these User Settings:

"editor.detectIndentation": false,
"editor.tabSize": 2,

If you now reformat your code in VSCode it will indent with two space.

Overall, excellent work. I'm looking forward to review your final version next week.

Aya-Alabrash / hyf-javascript-3

Feedback homework week 2 #2