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

London 9 - turing - susan - JavaScript Core 1 - Week 3 - #161

Open
susanssky wants to merge 4 commits into
CodeYourFuture:mainfrom
susanssky:main
Open

London 9 - turing - susan - JavaScript Core 1 - Week 3#161
susanssky wants to merge 4 commits into
CodeYourFuture:mainfrom
susanssky:main

Conversation

@susanssky

Copy link
Copy Markdown

No description provided.

@samhoooo
samhoooo self-requested a review November 28, 2022 21:15

@samhoooo samhoooo 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 work! You are already proficient in Javascript~

Here are some comments and potential problems I have spotted.


function getTemperatureReport(cities) {
// TODO
let arr = []

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just want to share an interesting thing, you can actually use const in here!
Using const makes the variable cannot be reassigned, but doesn't mean it cannot be modified. (with arr.push() in this example)

You can learn more in this short video: https://www.youtube.com/watch?v=RE6qf3As-XU

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.

Because the array and the object both are "By Reference", so they could be modified..

const arr = [1,2,3,4,5]
arr.push(2)
console.log(arr) //[ 1, 2, 3, 4, 5, 2 ]

const obj = {}
obj.text='hello'
console.log(obj) //{ text: 'hello' }

let n
do {
n = generateRandomNumber()
return n + 50

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your code works, but if you are adding 50 in the return value here anyway, do you really need the while loop?
Anyway, the question requires us to use while loop.

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.

The problem is we could not control the random number must be greater than 50

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Run a while loop, until n is greater than 50, do you think we can handle it?

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.

It is do-while loop, not while loop. Do you have some tips?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do you think we can acieve by putting return outside the do-while loop?

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.

Now the 2nd version match your request?

Comment on lines 29 to 36
function headlinesWithNumbers(allArticleTitles) {
// TODO
let NumberArray = []
const regex = /[0-9]/g;
for (let i = 0; i < allArticleTitles.length; i++) {
if (allArticleTitles[i].search(regex) > 0) { NumberArray.push(allArticleTitles[i]) }
}
return NumberArray
}

@samhoooo samhoooo Dec 1, 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.

These are example headlines which contain a number, should return by the function but failed

headlinesWithNumbers(["2", "2 words in here"])

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.

What failed? Have you ran the npm run test?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your code passed the test cases, but doesn't mean it is correct.
There are cases which contains number I think we should handle

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.

allArticleTitles[i].search(regex) > 0->allArticleTitles[i].search(regex) >= 0

Comment on lines 13 to 30
function getHighestRatedInEachGenre(books) {
// TODO
// --because it has not written how much books I can get OR How rating can I get--
// --so there are 2 ways to get the 3 books--
// --first way--
// const getThan48 = books.filter(value => value.rating > 4.8)
// let arr = []
// for (let i = 0; i < getThan48.length; i++) {
// arr.unshift(getThan48[i].title)
// }
// return arr
// --second way--
books.sort((a, b) => b.rating - a.rating)
let arr = []
for (let i = 0; i < 3; i++) {
arr.push(books[i].title)
}
return arr
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Have we considered genre?

The question states Each title in the resulting array should be the highest rated book in its genre.

For example, in the test case, "The Book Your Dog Wishes You Would Read" has the highest rating in the genre "non-fiction"

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.

Do you think 3-2 2nd version could be simpler?

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 would make use of the Map Object

function getHighestRatedInEachGenre(books) {
  const genreMap = new Map();
  for (let book of books) {
      const existingBook = genreMap.get(book.genre)
      if (existingBook == null || book.rating > existingBook.rating)
        genreMap.set(book.genre, book)
  }
  return Array.from(genreMap.values()).map(book => book.title)
}

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