Glasgow 6 - Malkit Benning - JS Core 1 Week 3 by malkitbenning · Pull Request #225 · CodeYourFuture/JavaScript-Core-1-Coursework-Week3

malkitbenning · 2023-03-03T11:07:24Z

All exercises completed and all tests passed.

weather and financial times exercises complete

completed all mandatory exercises

all tests passed

Dedekind561

Well done, Malkit!
Some excellent work here - generally really good variable names, which make your code really clear to read.
I've left a few minor comments for things to improve too!

Dedekind561 · 2023-03-08T12:22:37Z

 */
 function potentialHeadlines(allArticleTitles) {
-    // TODO
+  const charLimit = 65;


Excellent - great use of a variable name charLimit! No magical numbers :)

Dedekind561 · 2023-03-08T12:22:47Z

 function potentialHeadlines(allArticleTitles) {
-    // TODO
+  const charLimit = 65;
+  const articlesUnderLimit = [];


Good variable name

Dedekind561 · 2023-03-08T12:26:27Z

-    // TODO
+  // TODO
+  let charCount = 0;
+  const articleTitleCount = allArticleTitles.length;


Don't generally need to store allArticlesTitles.length in a variable. Absolutely fine to write allArticleTitles.length inline.

Dedekind561 · 2023-03-08T12:26:44Z

+  // TODO
+  let charCount = 0;
+  const articleTitleCount = allArticleTitles.length;
+  for (const articleTitle of allArticleTitles) {


Suggested change

for (const articleTitle of allArticleTitles) {

for (const articleTitle of allArticleTitles.length) {

Dedekind561 · 2023-03-08T12:28:53Z

+  for (const companyPrices of closingPricesForAllStocks) {
+    const daysCount = companyPrices.length;
+    let companyPriceTotal = 0;
+    for (const singlePrice of companyPrices) {


This is a good solution - variable names are very clear.
As a refactor, consider moving the inner logic in the for loop into a separate function. What name would you give to this function?

Dedekind561 · 2023-03-08T12:29:21Z

+  for (const companyPrices of closingPricesForAllStocks) {
+    const firstPricePosition = 0;
+    const lastPricePosition = companyPrices.length - 1;
+    const stockPriceDiff =


Very readable 👍

Dedekind561 · 2023-03-08T12:33:31Z

 function findFirstJulyBDay(birthdays) {
-    // TODO
+  // TODO
+  //   for (let i = 0; i < BIRTHDAYS.length; i++) {


Remember to remove old comments. Remember you still have old versions of your code in your version history - so you should be able to inspect the old version if you want to.

AppolinFotso

Well done @malkitbenning! Your codes are clean and readable.

msimmdev

Hi Mal, great work on this exercise.

There's a few problems in the extra exercises I've noted, but these will be things we're exploring more in upcoming classes.

I noticed that a lot of the files have been reformatted, I'd suggest checking your editor settings as where possible it's better to avoid reformatting existing code in a file.

msimmdev · 2023-03-15T14:28:31Z


    We have a list of cities that the user wants to track.
-    We also already have a temperatureService function which will take a city as a parameter and return a temparature.
+    We also already have a temperatureService function which will take a city as a parameter and return a temperature.


Praise: Good Spot

msimmdev · 2023-03-15T14:34:16Z

+  const charLimit = 65;
+  const articlesUnderLimit = [];
+
+  for (const articleTitle of allArticleTitles) {


Suggestion: What you've done is fine, but it would also be appropriate to use a filter here such as

articlesUnderLimit = allArticleTitles.filter((articleTitle) => articleTitle <= charLimit);

msimmdev · 2023-03-15T14:35:16Z

 */
 function titleWithFewestWords(allArticleTitles) {
-    // TODO
+  // TODO


Nitpick: Remove the TODO comments once you've done the work, in production code TODO comments are often used to note places where further work still needs to be done so leaving it in can cause confusion

msimmdev · 2023-03-15T14:36:27Z

+  let shortestTitle = "";
+  let shortestSpaceCount = 0;
+  for (const title of allArticleTitles) {
+    if (shortestTitle === "") {


Question: Why do we need to have a check for blank strings specifically?

msimmdev · 2023-03-15T14:39:43Z

 */
+
+function containsNumbers(str) {
+  return /[0-9]/.test(str);


Suggestion: This works and is OK, however in the scope of work in this course I'd recommend avoiding regex's. Instead this could be done by using the string includes function.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/includes

msimmdev · 2023-03-15T14:40:32Z

    (Hint: remember that you can also loop through the characters of a string if you need to)
 */
+
+function containsNumbers(str) {


Praise: Good separation of logic into a new function

msimmdev · 2023-03-15T15:58:25Z

+    const daysCount = companyPrices.length;
+    let companyPriceTotal = 0;
+    for (const singlePrice of companyPrices) {
+      companyPriceTotal += singlePrice;


Suggestion: As we discussed in our recent meetings, it can be better to deal with currency values as pennies when doing math operations on them, here it might make sense to do

companyPriceTotal += singlePrice * 100;

that way we can aggregate a total number of pennies and avoid a lot of floating point math. Remember you will need to divide by 100 at the end.

msimmdev · 2023-03-15T16:11:50Z

 */
+let highestBooks = [];
+let highBook = {};
+let finalList = [];


Issue: While you can declare variables like this in the body of a script it's generally bad practice for this type of script. (We call these globals) Instead you should try and declare your variables in the function body and if you need to share variables between functions pass them around in parameters.

msimmdev · 2023-03-15T16:14:19Z


+function getHighestRatedInEachGenre(books) {


Suggestion: After next class have a think about how this solution might be simplified by using an object.

msimmdev · 2023-03-15T16:14:54Z


 function generateFibonacciSequence(n) {
-    // TODO
+  // TODO


Nitpick: Remove TODO comments

malkitbenning added 4 commits March 2, 2023 10:44

Initial Exercises Complete

e4fdc88

Mandatory pt 1 and pt 2 done

da2cce3

weather and financial times exercises complete

All mandatory exercises passed

cbba1a0

completed all mandatory exercises

extras all complete

b39cc9e

all tests passed

Dedekind561 added the review in progress label Mar 8, 2023

Dedekind561 reviewed Mar 8, 2023

View reviewed changes

Dedekind561 added reviewed A mentor has reviewed this code and removed review in progress labels Mar 8, 2023

AppolinFotso reviewed Mar 13, 2023

View reviewed changes

msimmdev reviewed Mar 15, 2023

View reviewed changes

	for (const articleTitle of allArticleTitles) {
	for (const articleTitle of allArticleTitles.length) {

Uh oh!

Uh oh!

Conversation

malkitbenning commented Mar 3, 2023

Uh oh!

Dedekind561 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppolinFotso left a comment

Choose a reason for hiding this comment

Uh oh!

msimmdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants