the-hip-hip-arrays / apisproject Goto Github PK

This is to get access to the photograph link and other details like cost

It's a good idea to declare your variables at the top of functions. In many of your functions, you define them part-way down or throughout the function.

For example, here: https://github.com/The-Hip-Hip-Arrays/apisproject/blob/master/js/app.js#L75

Consider moving the variable declarations to the top. Even if the value is not immediately available at the start of the function, just define an empty variable (var randomHatsNumber;), and then reassign the value further down.

Interested to hear if you have a different perspective on this. You can read more about this in Maintainable JavaScript (page 39).

Michelle (FAC5)

We have 2 of each of these, we need to delete the older versions

We really should at least attempt to add in a second API seen as that's the main point of the project!

Put it in the readme.

How many images, what should they be called, what requests are going into each image.

basic API call can be refactored into a function that then is reused.

I understand that testing may have been difficult to wrap your heads around.

One of the reasons for this is that the way you have written your code makes it inherently difficult to test.

For example, see here. In these lines you are:

• Parsing your response data
• Mutating a global variable
• Choosing a random element of an array
• Some DOM manipulation
  That is quite a lot of different things going on. Ideally there would be a separation of concerns in your code.

This process is also repeated later on in your code. Repeated operations can be abstracted out to functions that can be tested by themselves. Similarly, instead of passing an anonymous function as your event handler, define a function separately that you can subsequently test.

Always a good idea to include run instructions in your README. Just a quick guide to cloning the repo, getting an API key (if you need one) and viewing the project will do.

Michelle (FAC5)

You have quite a lot of global variables in app.js: https://github.com/The-Hip-Hip-Arrays/apisproject/blob/master/js/app.js. You are defining 9 variables at the top of the file.

Global variables are usually a bad idea, because they pollute the global namespace of your project. Relying too much on global variables can result in conflicts between various scripts on the same page. Read more about it here: http://stackoverflow.com/questions/2613310/ive-heard-global-variables-are-bad-what-alternative-solution-should-i-use.

Consider declaring them inside your functions and returning the values from there.

Michelle

You could separate the update image change function from the API call. API pulls 200 results. You could use all 200 results before making another call (and avoid annoying Zalando).

👍

Pick up your game guys

Be aware of what size it is...

But... Can we add them to the .gitignore? 😄

We will reuse this code, so write a function called getRandomArticle