aya-alabrash / hyf-javascript-3 Goto Github PK

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.

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.

Hi Aya, here is my feedback on your homework for week 3 (OOP version):

When I load your index.html file in the browser it looks good and functions as expected. However, your app is not responsive. On an (emulated) iPhone 6/7/8 it looks like this:

On your code, I have little to comment. As last time it is a pleasure to read. Still, there are some minor improvements possible.

1. The sorting of the repository information could be simplified by using the standard string method .localeCompare():

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

The code that you used obviously works, however it does not take into account local language peculiarities. For instance, suppose you have an array with the following French words (listed in the correct dictionary order):

const frenchWords = [ 'effusion', 'égal', 'emballer' ];

The result from sorting:

[ 'effusion', 'emballer', 'égal' ]  // <= your method
[ 'effusion', 'égal', 'emballer' ]  // <= localCompare

The .localeCompare() method might even be able to sort Arabic words (which I know to have no uppercase letters) in the correct dictionary order (perhaps you can try).

2. In the assignment, it was suggested that you create a method fetchContributors() in the Repository class. Because the Repository class has all the information needed (in your case in this._infoData) to manage itself completely, it can fetch the contributors without an external caller needing to tell it what URL to use. So this fetchContributors() method could look like this:

fetchContributors() {
  return fetchJSON(this._data.contributors_url);
}

However, to make this work you would need to move the fetchJSON() function out of the View class and make it a stand-alone function again, similar to createAndAppend().

With this fetchContributors() method in the Repository class your render() method of the View class could look like this:

async render(repoData) {
  try {
    const ulInfo = document.getElementById('infoUl');
    const ulImg = document.getElementById('imgUl');

    const repository = new Repository(repoData);
    const data = await repository.fetchContributors();
    const contributors = new Contributor(data);
    repository.render(ulInfo);
    contributors.render(ulImg);
  }
  catch (err) {
    renderError(err);
  }
}

Now, your View class can remain completely unaware about the internal details of the repository information (that's a good thing).

3. There is a problem with your use of window.onload. The way you coded it does not actually cause your code to be executed when the page is fully loaded. Instead it is executed immediately when the JavaScript engine loads the script file, which may be too early. If I move the <script src="app.js"> tag from <body> to <header> (which is a valid thing to do), you app no longer works. I get this error in the console:

Uncaught (in promise) TypeError: Cannot read property 'appendChild' of null
    at createAndAppend (app.js:112)
    at View.mainPage (app.js:54)
    at new View (app.js:47)
    at app.js:149

The window.onload property must be assigned a function. That function will be called when the page is completely loaded. For instance, you could do this (using an arrow function):

window.onload = () => new View();

Overall, you have done very well in JS3 class and I have no doubt you will do well in the rest of the HYF curriculum too.