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

WM4_Dawit-Abraha_JavaScript-Core-1_Week-3 - #92

Open
Dawit-Dev wants to merge 2 commits into
CodeYourFuture:mainfrom
Dawit-Dev:main
Open

WM4_Dawit-Abraha_JavaScript-Core-1_Week-3#92
Dawit-Dev wants to merge 2 commits into
CodeYourFuture:mainfrom
Dawit-Dev:main

Conversation

@Dawit-Dev

Copy link
Copy Markdown

No description provided.

let array = allArticleTitles[0].split(" ").length;
let spaceCount;
for (let i = 0; i < allArticleTitles.length; i++) {
wordCount = allArticleTitles[i].split(" ").length;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I suggest to add .trim() method before split ,Because if there is any space at first or in the end of string it will wrongly add to the number of words

@simeonbikov simeonbikov 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.

Well done Dawit!

i++
} while (n > evenNum.length)

let sum = evenNum.reduce((acc, curr) => acc + curr, 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.

I like that you used reduce method here


// TODO Use a for-of loop to capitalise and output each letter in the string seperately.
let str = "codeyourfuture";
for (arr of str){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

it would be nice to declare variable - arr

*/
function headlinesWithNumbers(allArticleTitles) {
// TODO
return allArticleTitles.filter(headline => /[0-9]/.test(headline));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like this solution

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 do you like it? What is appealing about this code?

@simeonbikov simeonbikov Aug 30, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

My comment is a sample of how beginners comment who make a review for beginners))
I liked that it fits in one line and it is clear what is going on.


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

@jdbevan jdbevan Aug 30, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice use of .filter(). Why did you chose this solution vs the commented out one?


function titleWithFewestWords(allArticleTitles) {
// TODO
let array = allArticleTitles[0].split(" ").length;

@jdbevan jdbevan Aug 30, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What do you think will happen on this line of code if there are no articles?

Naming is hard. What data does the variable array contain? Can you think of a better variable name?

let array = allArticleTitles[0].split(" ").length;
let spaceCount;
for (let i = 0; i < allArticleTitles.length; i++) {
wordCount = allArticleTitles[i].split(" ").length;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What is missing from this line?

Comment thread 2-mandatory/4-stocks.js
function getAveragePrices(closingPricesForAllStocks) {
// TODO
let arr = [];
let average = 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.

What is the benefit of defining this variable outside the for loop?

Comment thread 2-mandatory/4-stocks.js
const sortedPrices = closingPricesForAllStocks.map((prices) =>
prices.sort((a, b) => b - a)
);
return sortedPrices.map((price, index) => {

@jdbevan jdbevan Aug 30, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice use of .map() 😄 why did you go with this vs the commented out code?

@jdbevan jdbevan 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.

Good effort!

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.

4 participants