gisaac85 / hyf-javascript3 Goto Github PK

Hi Isaac, here is my feedback on your revised homework in the folder async-promises.

I think you got my initial feedback pretty well covered and you managed to get rid of the unnecessary XMLHttpRequests. Good work!

Coming back on your sortList() function:

In your manipulateSelect() function you create a <select> element and add <option> elements in the order of the repository data returned by GitHub. Then, you use your sortList() function to correct the text and value properties of each <option> element to make them appear in alphabetically sorted order. This seems rather convoluted. Why not sort the repo array before creating <option> statements in the first place, like shown here.

function manipulateSelect(repos) {

  repos.sort((a, b) => a.name.localeCompare(b.name)); // <== added

  const select = createAndAppend('select', document.getElementById('header'));

  // ...

    repos.forEach((repo, i) => {
    createAndAppend('option', select, {
      html: repos[i].name,
      value: i
    });
  });

  // sortList(select); // <== removed

  // ...
}

So instead of sorting afterwards (and correcting what was the wrong order initially), do it directly on the data as you receive it (without creating something in the wrong order first and then correcting it). You only need a single line for the sort (but of course you need to know how you can use .sort() in combination with the localeCompare() string method).

You can now completely get rid of your sortList function.

By the way, never do this const clTexts = new Array();. Use an array literal instead: const clTexts = []. This, for the same reason that you don't use const helloWorld = new String('Hello Word'); to define a string.

Hi Isaac, here is my feedback on the final week 3 homework.

This review is based on your OOPClasses folder.

Overall I'm very pleased with what I see in terms of functionality and code quality. Your application works fine and is responsive, although the layout get a bit screwed up on certain device types, for instance on the iPhone 6/7/8 Plus.

1. It would be nice if you could click on a contributor to open a new Tab with that contributor's GitHub page. That would also make it a bit more of a challenge to make your application ARIA-compliant (so that a user can also navigate through the contributor list using the keyboard).

With respect to the JavaScript code:

2. Your code looks well organised. I don't see any ESLint warnings and only one or two spelling warnings because you chose to use Contr as abbreviation for Contributors. I still think your code is now and then unnecessarily stretched out vertically due to an abundance of unneeded blank lines. This makes it for instance difficult to oversee in one glance the whole of your function start() in App.js. For that function I would probably condense some of your code and use blank lines to separate blocks of related code like this:

async start() {
  try {
    const url = 'https://api.github.com/orgs/HackYourFuture/repos?per_page=100';
    const root = document.getElementById('root');

    this.createAndAppend('h1', root, {
      html: 'HYF SPA <br> BY <br> >> OOP ES6 Classes <<'
    });

    const header = this.createAndAppend('div', root, { id: 'header' });
    this.createAndAppend('label', header, { html: 'Select a Repository:' });

    const container = this.createAndAppend('div', root, {
      class: 'container',
      id: 'container'
    });

    const informationDiv = this.createAndAppend('div', container, { class: 'infoDiv' });
    this.createAndAppend('ul', informationDiv, { id: 'info' });

    const imagesDiv = this.createAndAppend('div', container, { class: 'imgDiv' });
    this.createAndAppend('ul', imagesDiv, {  id: 'imgUl' });

    const data = await this.fetchJSON(url);
    this.manipulateSelect(data);
  } catch (err) {
    document.getElementById('container').innerHTML = err.message;
  }
}

I recommend that you use blank lines only for a reason. Remove any blank line for which you cannot come up with a good reason for why it's there.

3. You have duplicated code: fetchJSON() in App.js and fetchContributors() in Repository.js do exactly the same thing. You could just add fetchJSON() to the View base class, making it available to all its subclasses. Note that you don't need the async keyword for fetchContributors(). You only need that keyword if in the function body you use await.

4. Your render() function in the Repository class doesn't use await and therefore doesn't need to be prefixed with the async keyword. There is also no need for try/catch in that function because there is no exception to be expected here. Although the async and try/catch obviously do not prevent your code from running correctly, they are confusing to the reviewer of your code (thinking: am I missing something here??).

5. I intended the Contributor class to hold one contributor only. That's why I chose the word Contributor over Contributors. However, your implementation, where you use the Contributor class for the array of contributors is not wrong per se.

6. Your function sortList() in View.js is now dead code (i.e. unused) and should be removed.

With this, I have no doubt you will do well in the rest of the curriculum.

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.

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

Just running your code reveals that it meets the stated functional requirements. From that perspective: well done!

However, when looking at your code I am less pleased. I see a single, large function, called populateSelect. Of course, this function does far more than populate the <select> element. It contains all of your program (apart from the createAndAppend() function). This is not the way to write programs in general. It is not good enough that your program is working. It must also be readable and maintainable by humans. Take into consideration that in real life the programs that you write must be maintained by others, perhaps long time after you have switched jobs. All the maintainer then has is your source code to work with. Conversely, you may have to maintain software that somebody else wrote. In that case you would hope for code that is readable, well organized and easy to grasp.

The assignment requires you to write a function called fetchJSON():

Write a function called fetchJSON (or copy from the lecture code) which accepts (at least) the following parameters: url and callback.

I don't see that in your code.

It also stated:

Make your functions small and reusable (modular)! That means create separate functions to handle certain steps.

Again, this is also not done.

I completed a write-up today on this topic and posted it in slack: https://gist.github.com/remarcmij/57101c1107f2bd479a162b18a1e7ae01

Your code could benefit from restructuring using the principles discussed in this document. This restructuring would also help making the changes required in week 2 and week 3. I would recommend to make these changes before starting with the week 2 homework.

Note: although your program is relatively small and therefore not too difficult to oversee, it is important to always apply the same rigorous style of writing code using the stated principles.