anasmos / hyf-javascript3

0 stars 0 forks source link

Feedback final homework #3

Open remarcmij opened 6 years ago

remarcmij commented 6 years ago

Hi Anas,

Here is my feedback on your final homework.

While your code is functionally correct, I would not consider this final, clean code. There are a couple of issues that as a potential employer I would probably ask you to clean up before making a final decision.

I'll now go through the individual criteria that I defined for this assignment:

  1. Your code must run without errors.

    Requirement is met.

  2. Your web page must be fit-for-purpose and well-designed. Place yourself in the role of an end-user: is the web page attractive and useful, or will you quickly click away?

    • Your links (<a> tags) are complete div's with complex content. This goes against the convention of a link being a simple string or image.
    • When you make a new request you should erase the HTML elements created by the previous request, otherwise it gets just appended at the end of the bottom of the previous HTML.
  3. Your code must be well-formatted.

    Requirement is met, although I would use a blank line here and there to separate code blocks.

  4. Your repo must contain an .eslintrc.json file. There should be no avoidable ESLint warnings in your code. If you can fix the warning, do so. Please be prepared to account for any remaining warnings.

    Requirement is met.

  5. There should be no spelling errors in variable and function names, text strings and comments. If the VSCode spelling checker flags certain words as incorrect (and sometimes that is inevitable) you should be able to explain why that is. If you are uncertain about some word, consult an English dictionary.

    There are some sloppy spelling errors in your index.html:

    • Repositorie -> Repository
    • HachYourFuture -> HackYourFuture
  6. _Variable and function names must conform to these naming conventions._

    Requirement is met, but avoid unnecessary abbreviations.

    • Contribut -> Contributer (what is the purpose of abbreviating this?)
    • datas: data is already plural (datum is the singular form)
  7. Consider blank lines to convey a meaning, similar to using blank lines to separate paragraphs in a piece of written text. A blank line between function/method definitions is fine. A blank line that breaks up a piece of code that actually belongs together is wrong. Whenever you add a blank line, carefully consider whether the blank line helps or hurts readability. Keep it in only if it helps.

    Requirement is met.

  8. There should be no commented out code in your final homework. (Just like you wouldn't leave crossed out text in your CV).

    Requirement is met.

  9. There should be no unnecessary console.log statements in your final code. These statements are fine when developing and debugging, but not in your final product (and, as per point 8 above, don't just comment them out).

    Requirement is met.

  10. Review your own code as if you were an employer yourself. Would you offer yourself an internship or job when running and looking at the code?

    Probably dependent on a second round of clean ups.

You have used the createAndAppend utility function in some places, but at other places you still use createElement and appendChild manually. This is inconsistent.

I have refactored your code to make it more readable, see below. I haven't changed any CSS class names or element id's, but would be inclined to do so.

I have extended the createAndAppend function so that it can also deal with optional attributes.

'use strict';
{
  function renderPage() {
    const repoContainer = createAndAppend('div', document.body, null, {
      id: 'container'
    });
    const userContainer = createAndAppend('div', document.body, null, {
      id: 'usernames'
    });
    const contributorsContainer = createAndAppend('div', document.body, null, {
      id: 'containerContribut'
    });

    const searchInput = document.getElementById('search-box');

    const clearContainers = () => {
      repoContainer.innerHTML = "";
      userContainer.innerHTML = "";
      contributorsContainer.innerHTML = "";
    };

    document
      .getElementById('repo-search')
      .addEventListener('click', () => {
        clearContainers();
        onRepoHyfClick(repoContainer, contributorsContainer, searchInput.value);
      });

    document
      .getElementById('username-search')
      .addEventListener('click', () => {
        clearContainers();
        onUserNameClick(userContainer, searchInput.value);
      });
  }

  function onRepoHyfClick(repoContainer, contributorsContainer, repoName) {
    const repoUrl = "https://api.github.com/repos/HackYourFuture/" + repoName;
    fetchJSON(repoUrl)
      .then(data => {
        if (!data.name) {
          throw new Error(`this ${repoName} Repository is not found`);
        }

        const a = createAndAppend('a', repoContainer, null, {
          href: data.html_url,
          target: '_blank',
          id: 'a-id'
        });

        const div = createAndAppend('div', a, {
          class: 'div-hyf'
        });

        createAndAppend('img', div, null, {
          class: 'imgRepo',
          src: data.owner.avatar_url
        });

        const ul = createAndAppend('ul', div);

        createAndAppend('li', ul, "Name of the repo: " + data.name);
        createAndAppend('li', ul, "Description: " + data.description);
        createAndAppend('li', ul, "Created by: " + data.owner.login);
        createAndAppend('li', ul, "Created at: " + data.created_at);
        createAndAppend('li', ul, "Last update at: " + data.updated_at);

        return fetchJSON(data.contributors_url);
      })
      .then(items => {
        createAndAppend('h1', contributorsContainer, 'contributors:');
        items.forEach(item => {
          const div = createAndAppend('div', contributorsContainer, null, {
            class: 'subdiv'
          });
          createAndAppend('h2', div, 'Contributor name:  ' + item.login);
          createAndAppend('img', div, null, {
            class: 'imgRepo',
            src: item.avatar_url
          });
        });
      })
      .catch(error => {
        createAndAppend('h3', repoContainer, error.message);
      });
  }

  function onUserNameClick(userContainer, userName) {
    const url = "https://api.github.com/users/" + userName + "/repos";
    fetchJSON(url)
      .then(items => {
        if (!items[0]) {
          throw new Error(`User ${userName} is not found`);
        }

        items.forEach(item => {
          const username = createAndAppend('div', userContainer, null, {
            class: 'username'
          });

          const a = createAndAppend('a', username, null, {
            href: item.html_url,
            target: '_blank',
            id: 'a-id'
          });

          const div = createAndAppend('div', a, null, {
            class: 'details'
          });

          createAndAppend('img', div, null, {
            class: 'imgRepo',
            src: item.owner.avatar_url
          });

          const ul = createAndAppend('ul', div);
          createAndAppend('li', ul, "Name of the repo: " + item.name);
          createAndAppend('li', ul, "Description: " + item.description);
          createAndAppend('li', ul, "Created by: " + item.owner.login);
          createAndAppend('li', ul, "Created at: " + item.created_at);
          createAndAppend('li', ul, "Last update at: " + item.updated_at);
        });
      })
      .catch(error => {
        createAndAppend('h3', userContainer, error.message);
      });
  }

  function createAndAppend(name, parent, innerHTML, attributes) {
    attributes = attributes || {};
    const child = document.createElement(name);
    parent.appendChild(child);
    if (innerHTML != null) {
      child.innerHTML = innerHTML;
    }
    Object.keys(attributes).forEach(name => {
      child.setAttribute(name, attributes[name]);
    });
    return child;
  }

  function fetchJSON(url) {
    return new Promise((resolve, reject) => {
      const xhr = new XMLHttpRequest();
      xhr.open('GET', url);
      xhr.responseType = 'json';
      xhr.onload = () => resolve(xhr.response);
      xhr.onerror = () => reject(xhr.statusText);
      xhr.send();
    });
  }

  renderPage();
}
anasmos commented 6 years ago

Hi Jim, thank you for your feedback its very useful for me, I will try to fix every thing you mention it, and I thank for teach us and I am proud to you be my teacher.