Skip to content
This repository was archived by the owner on Jan 14, 2024. It is now read-only.

Glasgow Class 6 - HERISH TURKI - JS Core 2 - Week 1 #221

Open
HereshT wants to merge 6 commits intoCodeYourFuture:mainfrom
HereshT:main
Open

Glasgow Class 6 - HERISH TURKI - JS Core 2 - Week 1 #221
HereshT wants to merge 6 commits intoCodeYourFuture:mainfrom
HereshT:main

Conversation

@HereshT
Copy link

@HereshT HereshT commented Mar 20, 2023

Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in HOW_TO_MARK.md in the root of this repository

Your Details

  • Your Name: HERISH TURKI
  • Your City: GLASGOW
  • Your Slack Name: HereshT

Homework Details

  • Module:
  • Week:

Notes

  • What did you find easy?

  • What did you find hard?

  • What do you still not understand?

  • Any other notes?

Copy link

@shieldo shieldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good stuff! A few really minor comments and things to think about.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach here is fine, and it's really good that you've been able to use array restructuring here to write simpler and more comprehensible code!

The format of COUNTRY_CURRENCY_CODES_OBJECT would more usually be written as countryCurrencyCodeObject in JavaScript - the capital letters joined by underscores format is more for constants that don't change their values during the lifetime of the program, rather than (in this case) a temporary variable for an object that is being built up and then returned. But that's a minor comment.


// 2 lovely array methods which I learned here

const allPantryContents = Object.values(pantry).flat();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a neat way to get the ingredients in the pantry into one list! Note that it wouldn't work if the pantry object had other properties too that didn't contain lists of ingredients. But if we know that it never will (and that might be quite reasonable here) then this is a good way to go.

for (let i = 0; i < recipe.ingredients.length; i++) {
if (!allPantryContents.includes(recipe.ingredients[i]))
missingIngredients.push(recipe.ingredients[i]);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works fine, but it would also be quite a good opportunity to use .filter() on the recipe ingredients, e.g.:

const missingIngredients = recipe.ingredients.filter(
  (ingredient) => !allPantryContents.includes(ingredient)
);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions are fine and clear - it's generally not advised though to change the value of an argument passed in to a function unless that is the explicit purpose of the function. It happens that we expect a number to be passed in here, which won't cause problems to any code calling these functions. In general, though, if we are building a value to be returned, it's best to declare a new variable to hold this new value as we are building it rather than modifying an argument (here balance).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all fine!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants