Feedback homework week 2

[remarcmij]

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

The comments below refer to your 'promises' version of the application. I will review the final version with async/await etc next week.

As you can see I found quite some things to comment about, despite that your code is working and functional. The devil is in the detail.

1. Your .eslintrc.json file contains a rule that it should give a warning when you use semicolons to terminate a statement. However, you do use semicolons (as we generally recommend in HYF) and therefore you get warnings from ESLint about them. So I suggest that you change the rule in .eslintrc.json as follows:

"semi": [
  "warn",
  "always"
],

Now you will get a warning if you forget to use a terminating semicolon.

2. Your app does not display all the HYF repos, but only the first 30. This is because the URL that you use for the GitHub API has the string json appended to the end, which should not be there. If your remove that you get the full 42 repos present at the time of this writing.

3. Your sorting procedure is overly complicated. Rather than sorting the <option> elements it is far easier to sort the array of repository information before you create the <option> elements. In your manipulateSelect() function you need to add only one line to do the sorting, and you can scrap your complete sortList() function.

function manipulateSelect(repos) {
  // ...
  repos
    .sort((a, b) => a.name.localeCompare(b.name)) // <== added
    .forEach(repo => {
      createAndAppend('option', select, {
        html: repo.name,
        value: repo.url
      });
    });
  //...
}

Notice that I also renamed the parameter from data to repos, because the name repos reflects more accurately what the parameter represents (always seek the most descriptive name.)

4. In your function getContributorInformation() you catch errors only for the inner fetchJSON(). If I force an error for the first fetchJSON() by for instance appending an x to its argument:

fetchJSON(data + 'x')
  .then(data => {

I get this error message in the browser console:

script.js:19 Uncaught (in promise) Error: Error: 404 Not Found
    at XMLHttpRequest.xhr.onreadystatechange (script.js:19)

Clearly, you are not catching the error. You must use .catch() at the end of the first fetchJSON(). Because you return the promise returned by the inner fetchJSON() this outer .catch() will pickup all errors from the promise chain. So your code could look like this:

  function getContributorInformation(url) {
    const ulImg = document.getElementById('imgUl');
    ulImg.innerHTML = '';

    fetchJSON(url)
      .then(repo => {
        return fetchJSON(repo.contributors_url)
          .then(contributors => {
            for (const contributor of contributors) {
              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(err => {
        document.getElementById('container').innerHTML = err.message;
      });
  }

Also note how I changed some variable names. Some of the names that you have been using are confusing and misleading. For instance, you have used the parameter name data for the getContributorInformation() function. As I mentioned in my slack PM to you:

Remember to use the most descriptive names for your variables. The computer doesn’t care about the names you use, but we, fellow humans reading your code, would like to understand without needing to guess! And the data parameter of your getContributorInformation is actually a URL, so url is a better name. The name data is as meaningless as the word stuff (unless you actually mean stuff).

The name url1 is also misleading:

for (const contributor of url1) 

Because what you call url1 is actually an array of contributors and therefore should be named contributors.

Your code looks very stretched out vertically because of the many blank lines. It is better to only use a blank line where it is necessary, like when you use blank lines in a piece of text to separate paragraphs. Consider blank lines in your code to serve a similar purpose.

5. You are making three XMLHttpRequests when the user changes the repo selection. Strictly speaking, you only need to make one XMLHttpRequest. If you want to know how, then it is best if we discuss this in a short hangout. Let me know if you are interested and we can schedule something later this week.

6. When you use JSDoc to document your functions, you must use a specific format to describe function parameters. You did it (almost) correctly for function createAndAppend():

/**
 * Add DOM element with properties
 * @param {string} Name of DOM child
 * @param {string} parent Name of DOM parent
 * @param {object} options Attributes of DOM Child
 */
function createAndAppend(name, parent, options = {}) {
  //...
}

The first word after @param {xxx} must be the name of the parameter, as it appears in the parameter list. So name and not Name. Then, it should be followed by the description of the parameter. And of course, the description must be accurate. So, for instance:

/*
  * Creates an element and appends it to a parent
  * @param {string} name The tag name of the element to create
  * @param {string} parent The element to which to append the created element
  * @param {object} options Attributes to assign to the created elements
  */
function createAndAppend(name, parent, options = {}) {
  //...
}

If you now hover your mouse over the function name it tell a true story.

In other places the information you provide is downright wrong:

/**
 * Return Information of Repo
 * @param {object} DOM element
 */
function getRepoInformation(data) {
  // ...
}

This function doesn't 'return' anything. In fact, the function name getRepoInformation() is misleading because, although you are getting repo information you are not 'getting' it for the caller of your function. A name such as fetchAndRenderRepo() would be better. Also, the JSDoc parameter description is not in the required format. I would prefer to have no JSDoc information over inaccurate information.

[gisaac85]

thanks Jim I did all your notes ....