yasershomaf/homeworks

This repository contains all the required assignments by "Hack your future" team

homeworks's Issues

Do you really need all these global vars?

You prob only need your datastructure to be global. If not, youre doing something wrong. Look at other students hw to see how they solved this.

var reposList, repoItems, followers, publicRepos, intervalId;
    var userInfo = [];
    var oldRequest = "";

You might be able to improve your userInfo datastructure

If I understand it correctly, what you're doing now is this:

userInfo[0] has the users profile info.
userInfo[1] has the users repo
userInfo[2] has the follower info

You could also follow this structure:

userInfo.profile = { name: resultJson.login, etc }
userInfo.repos = [repos]
userInfo.followers = [followers]

It will make the code more readable.

Great readme

You could also add a list of features. Take a look at some other repos to see what I mean.

Event List not very useful.

There is a lot of information coming through the API. Why not include at least the dates so it has some more info? Without context the events (besides the commits that have some) they have no or very little meaning.

Why is the rightArticle an ul inside a ol???

This is a pretty neat idea

You probably dont have to ask the user to update this info but ti's still a cool pattern.

var intervalId = setInterval(checkForUpdate, 60000);
function checkForUpdate() {
  makeRequest(requestedURL).then(function(json) {
    if (json.updated_at != userInfo[0].updatedAt) {
      clearInterval(intervalId);
      if (confirm("The data on user's page was updated on GitHub. It's probably better to include these updates. do you want that?")) {
        //code here
      }
    }
  });
}

You have unclosed issues from last week!

Why did you wrap all your code in window.onload?

There's really no need to do this since you load your js at the bottom of the body element.
If it's about protecting the scope, there are better ways to do that. Please ask me about this in class.

Now it's ugly and it makes your code indent unnecessarily.

You should separate getuserinput into several functions

userinput should only get userinput and then make a request. the result of that request should be sent to a function like storeProfileInfo, then you could make another request and call storeReposInfo, etc.

Why are you setting the style from js?

As a general rule you dont want to write css directly in js.

No README.md (missing documentation)

You could put this in the Week6 folder (and maybe rename the folder to have a more descriptive name)

Pressing enter in searchbar is bugged

I cannot use enter to search, however, when I press enter it does show the github info of the owner again. Could not find out why so fast

getInputField does so much more than just getting the input field

Functions should do just one thing. They should be short and self descriptive. Your getinputfield function does waaay to much. If you want to clear innerhtml for instance, you could do that in a separate function. Or even create a function called clearInnerHtml(element) that you pass html elements to.

Show other info code

It's just pasting linebreaks to every field and setting the innerHTML. Also you are using inline-css. This doesn't happen in the rest of the code either and you should've learned by now that this is a no-no ;)

showRepoInfo url construction vague

Since this is a complex URL to construct it would be good to do it in parts (assign login to a var, name to a var, then concatenate the whole URL.

It would also make sense to have api.github.com in a variable so you don't have to retype it the whole time (this can be global, for that it is very useful :))

Nice use of callbacks when making requests!

Great, even better would be to make the first argument err contain the actuall error or error message. So every function that needs a response can handle the error in it's own way.

Note that XmlHTTPRequest also has the onload and onerror events which might be even more useful since you don't have to check for the property message.

Overal, very good work!

One line if - else statements used

If you use if (/else) statements always use accolades, even though you only write one line and use indenting.

Your page looks good but you can improve a lot on the code structure

Look at some other students' work to get an idea of a better structure, or check this codepen:
https://codepen.io/Razpudding/pen/VbgKpd

Code is not readable very easily

This is due to:

• Few newlines (enters)
• Single line comments for funtions (use multi line: /**)
• Almost no comments about what a group of statements does (these should be single line)

oldRequest is not a good variable name for userinput

It's confusing. Try calling it userName.

Use .forEach or .map instead of forloops

Use css classes for this

 followers.style.color = "#000000";
        publicRepos.style.color = "#ffffff";

IT's better to add and remoce CSS classes to achieve this effect. Take a look at my codepen examples to see how that's done.

Use onload or domready event to load javascript

It is now time to start loading your scripts like that. This makes it so you can place the content of your script anywhere and just bind to the event that is fired when the page is loaded and you can execute your initial function.

This also makes sure that you have no code execution on the global level (outisde of functions)

If you use an objectstructure, things like this can be so much more readable

followerUListItem.innerHTML = userInfo[2][i][1][j];

No polling, no map-filter-reduce

These were both new assignments. Try to still complete them (link)

Filename!

file.js is not(!) a good name. Why not name it after the functionality it holds? (Something with Github)