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

London 9 Lovelace - Oleksii Chepurnyi - JavaScript Core1 - Week 3 - #171

Open
OleksChep wants to merge 16 commits into
CodeYourFuture:mainfrom
OleksChep:main
Open

London 9 Lovelace - Oleksii Chepurnyi - JavaScript Core1 - Week 3#171
OleksChep wants to merge 16 commits into
CodeYourFuture:mainfrom
OleksChep:main

Conversation

@OleksChep

Copy link
Copy Markdown

There is something to work on.

@maxf
maxf self-requested a review December 10, 2022 11:50

@maxf maxf left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Excellent. Very good work. Two things:

  • you can now finish the exercises you've not started
  • you need to review how you use git. There are far too many commits, with no useful commit message. You need to understand that reading a git history should describe what you did, and not just be a list of 'updated' messages. In a professional environment, communicating how you write and change your code is just as important as the code itself. And git should be used towards that goal, not just as a backup system.

return birthdays[i];
}
i++;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Very good. You can also write it in fewer lines with find()

}
let i = 0;
let sum = 0;
do {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

too much indentation

sum = sum + i*2;
i++;
}
while(i < n );

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

whitespace missing before i < n

// TODO - Write for loop code here

/*
let i = 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you don't need to declare i here, since it's declared in the line below, in the for statement.
This could lead to future errors because now you have 2 different i variables, one outside of your for loop (which you don't need) and the one inside the for loop (which you need)

"Canada Water",
"Forest Hill",
" "
];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why did you change the list?

*/
function potentialHeadlines(allArticleTitles) {
// TODO
return allArticleTitles.filter((items) => items.length < 65);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This works perfectly, well done.
Hint: the choice of variable and parameter names is important to make your code readable. Here, items doesn't really describe what it contains. For instance, it could have the value "This is the title", which isn't really an items. Instead it's just a title of an article, so it would make more sense to call it just title for example.

Comment thread 3-extra/1-factorial.js
let result = 1;
while (n) {
result *= n--;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This works, but some teams prefer not to use -- or ++ because they make expressions hard to understand, specifically because expressions that change the values of variables (also called expressions with side effects) are confusing. So try to rewrite this expression to move the change of n on a separate line.

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