Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feras aldmeashki-w2-javascript #12

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Feras-ALdemashki
Copy link

No description provided.

}

function main() {
// TODO substitute your own name for "HackYourFuture"
const myName = 'HackYourFuture';
const myName = 'Feras Al-Demashki';

console.log(giveCompliment(myName));
console.log(giveCompliment(myName));
Copy link

@waxs waxs Dec 3, 2024

Choose a reason for hiding this comment

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

Nice job looks good! 😎 Your variable called randomCompliments could have been named better because it returns a random number. So for instance randomNumber or randomIndex. This will make the variable more meaningful and easier to understand for other people reading the codebase :)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I'll edit that and keep it in mind for future

const partnerNames = selectRandomly(y);
const locations = selectRandomly(s);
const jobTitles = selectRandomly(z);
return `You will be a ${jobTitles} in ${locations}, married to ${partnerNames} with ${numKids} kids.`;
Copy link

Choose a reason for hiding this comment

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

Good job, the choice to create multiple variables for each part of your sentence makes it easy to read :)

function addToShoppingCart(/* parameters go here */) {
// TODO complete this function
function addToShoppingCart(item) {
if (item === undefined) {
Copy link

Choose a reason for hiding this comment

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

In this case what you could have done is use !item that means there is no item specified. When using undefined you state that a variable has been declared but has not been assigned a value. So let's say someone adds the following:

let x; 

addToShoppingCart(x); 

This if statement wouldn't work. Then (if I am correct) the shoppingCart.push would add a empty item to the array.

Copy link
Author

Choose a reason for hiding this comment

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

got it ! ,

if (shoppingCart.length > 3) {
shoppingCart.shift();
}
return `You bought ${shoppingCart.join(', ')}!`;
Copy link

Choose a reason for hiding this comment

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

Good job on using the join method :)

Copy link
Author

Choose a reason for hiding this comment

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

thanks,

function addToShoppingCart(/* TODO parameter(s) go here */) {
// TODO complete this function
function addToShoppingCart(shoppingCart, item) {
const newShoppingCart = shoppingCart.concat(item);
Copy link

Choose a reason for hiding this comment

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

The concat method is used to merge multiple array's. In this case the item is a single value and not of type array so it would be best to either push or unshift method.

Copy link
Author

Choose a reason for hiding this comment

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

i replaced it with .push

for (let item in cart) {
total += cart[item];
}
return `Total: €${total}`;
Copy link

Choose a reason for hiding this comment

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

Good job, and it seems to work well. This could have also been done with the reduce function like so (the reduce function is often used to sum values):

array.reduce((accumulator, currentValue) => {
  // logic
}, initialValue);
const numbers = [1, 2, 3, 4, 5];

const sum = numbers.reduce((total, value) => total + value, 0);

console.log(sum);

Copy link

Choose a reason for hiding this comment

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

Please note that this is a tip!

Copy link
Author

Choose a reason for hiding this comment

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

i will check the reduce function that's for sure

name: employee.name,
occupation: employee.occupation,
email: employee.email,
};
Copy link

Choose a reason for hiding this comment

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

Well done. Something else you could have done is deconstruct the object like so:

const { name, occupation, email } = employee

Copy link

@waxs waxs left a comment

Choose a reason for hiding this comment

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

Overall good job, it works, but I think it could be beneficial to apply some minor changes to improve even further :)

@waxs waxs added the Needs work label Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants