Skip to content

Week 4#60

Open
havvarslan wants to merge 13 commits intorarmatei:masterfrom
havvarslan:week-4
Open

Week 4#60
havvarslan wants to merge 13 commits intorarmatei:masterfrom
havvarslan:week-4

Conversation

@havvarslan
Copy link
Copy Markdown

No description provided.

@tekul tekul self-requested a review April 11, 2019 13:30
Copy link
Copy Markdown
Collaborator

@tekul tekul left a comment

Choose a reason for hiding this comment

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

Great work Havva! 💯 . I posted a few suggestions and questions but they are mostly minor details, let me know what you think.

isTouchScreen: false,
hasCamera: true,
batteryTime: "3 hours"
};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good 👍 . Can you think of a better way of storing the screen size? A single number isn't too useful.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

can I write screenSize = '10 inches'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A screen has two dimensions, so I was thinking you could use a nested object, e.g.

screenSize: { width: 1024, height: 768 }

var artist = {
name: "Picassso",
ageHeDied: 78,
painting: ["Guernica", "The Old Guitarist", "Bulls Head"]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 Maybe name this field paintings since it is a list rather than a single painting.

name: "Pluto",
chemicalComposition: "Mixture of %70 Rock",
radius: 1150
};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm guessing you mean "planets" rather than "galaxies" 🙂 ? Also again the name should be singular (i.e. planet) rather than plural. If it was a list of objects rather than a single object a plural name would make sense.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

planet :)


var kitten = {
name: "Gilbert"
};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍


if (coffee == "flatWhite") {
if (this.insertedAmount >= this.prices.flatWhite) {
return "Please take your blackcoffee";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

"Expected result: 'Please take your flatWhite'. Actual result: Please take your blackcoffee" 🤔 .

Make sure you check the test output for each exercise.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

for this code I really try many way then I found the answer it was working but i didn't release I didn't change blackCoffee:(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah. It's just a cut and paste error :).

return restaurants.filter(parameter =>
parameter.address.area.includes(area)
).length;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍. Again, I would use restaurant instead of parameter.

// # 3
// prints [ 'head_intern', 'intern' ]
console.log();
console.log(Object.keys(storeBranches.edinburgh.interns));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good 🙂 👍 .

friends: ["John", "Nina"]
friends: ["John", "Nina"],
makeFriend: function(name) {
this.friends[this.friends.length] = name;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a case when I would use push. Indexing past the end of the array is a bit of a weird Javascript thing and best avoided. My advice would be to avoid using array indexes in general if you can think of another way to do something.

@havvarslan havvarslan closed this Apr 11, 2019
@havvarslan havvarslan deleted the week-4 branch April 11, 2019 15:18
@havvarslan
Copy link
Copy Markdown
Author

Great work Havva! . I posted a few suggestions and questions but they are mostly minor details, let me know what you think.
I always think small details are also important in my every step. Thank you very much it is good to see all this comment and are really helpful for my process.

@havvarslan havvarslan restored the week-4 branch April 13, 2019 22:54
@havvarslan
Copy link
Copy Markdown
Author

h

@havvarslan havvarslan reopened this Apr 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants