Feedback homework week 1

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

I have spent a bit more time on reviewing your work. I can see that you have prior programming experience and can benefit from some more detailed feedback.

Your application is nicely styled. I would add a bit of margin here and there, but overall it looks nice. Cool that you managed to produce circular avatars.

1. Overall your code looks good and well organized. And, not to forget, it works well. But when I look at the network tab in Chrome developer tools I note that you make two identical network requests per user selection.

2. When I start your application, the initial value of the <select> element is "AngularJS" but I don't see the information displayed in your web page for this repo. The reason for this is that the 'change' event for the <select> element is only fired if the user actually changes the selection. This does not happen when you start up the application. But you can manually call the same functions that you call in your event handler at the end of your manipulateSelect() function:

select.addEventListener('change', () => {
    getDetails(select.value);
    getImg(select.value);
});
getDetails(select.value);
getImg(select.value);

As you can see I replaced the event.target with select, as the event target (i.e. the element generating the event) is in this case the <select> element.

3. Your repo is missing an .eslintrc file. Therefore you are missing out on the useful help and warnings that ESLint can give you. Please create an .eslintrc file in the root of your hyf-javascript-3 folder and paste in the contents that I posted in slack during class. When you add this .eslintrc file you'll see that ESLint suggests that you can change a lot of let statements into const. This is always good because it better describes your intentions of not wanting/needing to change the variables concerned.

4. I like that you used functions to carve up the application in smaller tasks. However, where you now call fetchJSON() in line 43 code after calling main() in line 15, it would be better if you that fetchJSON() call inside main(). In this way your main() function coordinates all startup tasks.

function main() {
    let root = document.getElementById('root');

    // ...

    const ulImg = createAndAppend('ul', imagesDiv);
    ulImg.setAttribute('id', 'imgUl');

    fetchJSON(url, (error, data) => {
        if (error !== null) {
            console.error(error.message);
        } else {
            manipulateSelect(data);
        }
    });
}

I would also consistently use fat arrow functions for all in-line callbacks, if you are comfortable at using them.

Once you have a single function main() that controls all startup activities, you can now take advantage of the window.onload event handler to explicitly specify what should happen when your application is loaded. So rather then calling main() somewhere in your file, I would remove the call to main() in line 14 and move it to end of the file (can go anywhere, just my preference) like this:

window.onload = main;

5. Your manipulateSelect() function (I would call it renderSelect or setupSelect rather than using the prefix manipulate, because this code is run once at startup only) expects an argument called data. That is a rather meaningless name. It is in fact an array of repo information. I think I would choose the name repos for that argument.

Next you use a for...in loop to iterate through the array elements. While that technically works, you should not use a for...in loop on an array. If you want to use a for statement, you must either use a traditional for loop using an index value:

for (let i=0; i < repos.length; i++) {
  // ...
}

or a for...of loop:

for (const repo of repos) {
  // ...
}

As I mentioned in class, I personally try and avoid for loops if I can, for instance by using forEach():

repos.forEach(repo => {
  // ...
});

Coming back on why you should not use for...in on an array, consider this code:

const arr = ['one', 'two', 'three'];
arr.extra = 'test';

for (const key in arr) {
  console.log(arr[key]);
}
// output:
// one
// two
// three
// test

console.log('for...of');
for (const elem of arr) {
  console.log(elem);
}
// output:
// one
// two
// three

So the for...of loop only iterates through real array elements (those with numerical indices), excluding other properties that may have been added to the array object. It is probably also a bit more efficient since we do not need to use the bracket notation.

4. You have a couple of global variables in your code that you use to hold repository information. You may have noticed that the initial XMLHttpRequest that is needed to fill the <select> element actually contains all the information needed to display repo information. When I first wrote the assignment, I overlooked that and asked you to make a new XMLHttpRequest based on the name of the repo. But if we just use the information that we got from the initial request we can greatly simplify our code. The skeleton below show how you could refactor and simplify your code to take advantage of this.

'use strict';
const url = 'https://api.github.com/orgs/HackYourFuture/repos?per_page=100';

function createAndAppend(tagName, parent) {
  // ...
}

function main() {
  const root = document.getElementById('root');
  // ...

  const ulImg = createAndAppend('ul', imagesDiv);
  ulImg.setAttribute('id', 'imgUl');

  fetchJSON(url, (error, data) => {
    if (error !== null) {
      console.error(error.message);
    } else {
      renderSelect(data);
    }
  });
}

function fetchJSON(url, cb) {
  // ...
}

function renderSelect(repos) {
  const select = document.getElementById('select');
  repos.forEach((repo, i) => {
    const options = createAndAppend('option', select);
    options.innerHTML = repos[i].name;
    options.setAttribute('value', i);
  });
  select.addEventListener('change', () => {
    getDetails(repos[select.value]);
  });
  getDetails(repos[0]);
}

function getDetails(repo) {
  // No need to do another XMLHttpRequest to get repo info
  // We receive all information we need in the repo argument
  // ...
  getContributors(repo.contributors_url);
}

function getContributors(url) {
  // ...
  fetchJSON(url, (error, contributors) => {
    // ...
  });
}

window.onload = main;

6. Finally:

Please replace all your for...in loops that operate on arrays to for...of loops or use .forEach().
Walk through your variable and function names and ensure that their names truly reflect their purpose.
Ensure that arrays have names in the plural form, e.g. repos, contributors and that a single element of such an array has a singular name: repo, contributor. See Naming Conventions.

Aya-Alabrash / hyf-javascript-3

Feedback homework week 1 #1