Anaiah & Hsiang-Ting

[Anaiahm]

No description provided.

[yangashley]

👌 Good work using a constant value to refer to the proxy-server URL.

[yangashley]

👀 ❗ Nitpick: the spacing in this function is incorrect and makes it difficult to read. Please take care to fix up style issues in your code, especially when you're in internship. Code with style issues can signal to teammates an inattentiveness to detail.

A codebase with many small style issues is difficult to understand and maintain.

[yangashley]

🚨Nitpick: indentation issues on lines 30 through 37. Using a linter can help catch these issues while you're coding so that you can fix them up in the moment. In industry, almost certainly, your teammate that reviews your code will ask you to fix up style issues before they'll provide a review.

[yangashley]

The difference between these increaseTemp and decreaseTemp are + and -. Can you think of a way to write this logic with just one function that increase or decrease the temperature without repeating logic.

[yangashley]

Nice job organizing your code! I like that you have a function that registers all the event handlers.

The function registerEventHandlers does more than just registering event handlers though because it also performs some initial logic that needs to happen when the page loads (formatTempAndBackground and updateSky).

I'd prefer this initial set up logic to be in a separate helper function, maybe something like setUpUI. And also introduce a function like onLoaded that would call setUpUI and registerEventHandlers. You can see an example of how we do this here in pick-me-up-pup.

const setUpUI = () => {
  // steps to carry out when the page has loaded
  formatTempAndBackground();
  updateSky();
};

const onLoaded = () => {
  setUpUI();
  registerEventHandlers()
};

document.addEventListener('DOMContentLoaded', onLoaded);

[yangashley]

I'd also prefer that you have a function like loadControls that initializes all the variables that reference the HTML elements. Having a single place to 'load' all the elements that you manipulate on the page makes the code easier to maintain because I would know exactly where I need to go to make an update (imagine an element's id changes: document.getElementById('sky-options') --> document.getElementById('sky-display-options'))

Currently, I'd have to crawl through the entire file to find the element I want to update, but if everything was in onLoaded then I'd just go to the function and look for the element I'm concerned with.

loadControls would also be called in onLoaded.

[yangashley]

This function is brittle and can be difficult to maintain because it uses large chained if-statements.

What if the adjectives that describe the weather like "Cloudy", "Sunny", etc were keys in an object and thesky content were values?

If you had an object that held this data then you could refactor this implementation so you wouldn't need to chain many if-statements.

let skyText = document.getElementById('sky-options').value;

const sky_data = {
  'Cloudy': '☁️',
  'Sunny': '☀️',
  // rest of your data 
};

tempSky = sky_data[skyText];

[yangashley]

Again, this logic chains several if-statements and the solution is brittle and difficult to maintain. I'd prefer to see a data structure that stores the weather data so that your code could iterate over the data structure to set the temperature background and class. Using a loop to iterate over such a data structure would make your function more concise too.

[yangashley]

Remove debugging print statements when you're finished with them.

[yangashley]

I find the name for this function a little confusing because this function currently has two responsibilities: it gets a city's lat and lon and it also gets the weather for this city on line 18.

We should strive for a separation of concerns so that our code is easier to understand, test, and maintain.

You could rename this function to be more descriptive so that it explains the function's full responsibilities, something like getLocationAndWeather.

Ideally, you'd have 3 functions to make this logic more organized: findLatAndLon, getWeather and then getCityWeather that would then invoke the two other functions. Each function would be descriptively named, have a single concern, and be easier to test/maintain.

[yangashley]

When I run npm run lint against your project I get several errors that tempClass is not defined. You initialize tempBackground to an empty string before resetting the value based on some logic. The same should be done for `tempClass.

Suggested change

	let tempBackground = '';
	if (currentTemp > 80){
	tempBackground = 'summer-weather';
	tempClass = 'hot-weather';
	let tempBackground = '';
	let tempClass = '';
	if (currentTemp > 80){
	tempBackground = 'summer-weather';
	tempClass = 'hot-weather';