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

Glasgow_6 - HERISH TURKI - JS -Core-1-Coursework-Week3 - #242

Open
HereshT wants to merge 2 commits into
CodeYourFuture:mainfrom
HereshT:main
Open

Glasgow_6 - HERISH TURKI - JS -Core-1-Coursework-Week3#242
HereshT wants to merge 2 commits into
CodeYourFuture:mainfrom
HereshT:main

Conversation

@HereshT

@HereshT HereshT commented Mar 7, 2023

Copy link
Copy Markdown

Mandatory section is completed .

Mandatory section is completed .

@ali-nasir-ali ali-nasir-ali 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.

amazing progress keep going.

const temperature = temperatureService(city);
const statement = `The temperature in ${city} is ${temperature} degrees`;
temperatureStatementArray.push(statement);
}

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 way to do that.

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.

I learned a lotfrom that Udemy tutorial, he always uses template literal, you have watched it too

// TODO
// TODO
const headlines = allArticleTitles.filter((title) => title.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.

Very readable


for (const eachTitle of allArticleTitles) {
for (const char of eachTitle) {
if (!isNaN(parseInt(char))) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

that's intrusting use of the Boolean values i learned something here thank you.

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.

Hi Nasir :) I like this too, took me half a day to find this and sink in, especially this 3 devils :))) ===> ! is NaN
this feels like a maze inside another maze

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

This is really good work! I've made a couple of comments about variable naming, but that's very minor. All your tests pass and everything you have done here is reasonable. I'd encourage you to try looking at array methods like .map() and .filter() that can help simplify the code here in some cases.

Comment thread 2-mandatory/3-stocks.js Outdated
// TODO
// TODO

const average_CLOSING_PRICES_LAST_5_DAYS_FOR_ALL_STOCKS = [];

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's better to use variable names that are just short enough here - and remember that this function doesn't know that the closing prices are from the last 5 days - they could be from the last 10 days and the function should still work! closingPriceAverages or averageClosingPrices could be a good name for the variable 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.

Thanks a lot, I will try to use those methods as many times as possible for the week4.

Comment thread 2-mandatory/3-stocks.js Outdated
function highestPriceDescriptions(closingPricesForAllStocks, stocks) {
// TODO
// TODO
const eachArrayHighestPricDdescriptions = [];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

highestPriceDescriptions might be a clearer name here.

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.

3 participants