NW-6 | Fikret Ellek | JS1| [TECH ED] Complete week 3 exercises | WEEK-3 #193

fikretellek · 2024-01-03T22:14:49Z

Learners, PR Template

Self checklist

I have committed my files one by one, on purpose, and for a reason
I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
I have tested my changes
My changes follow the style guide
My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

pseudopilot

Hi @fikretellek . This is really good and every implementation you created here will definitely work.
Though, I'd like to help you to make your solutions more effective and readable. Please, check my comments below and let me know if you have any difficulty with it.

pseudopilot · 2024-04-30T21:37:33Z

week-3/stretch/rotate-char.js

+  if (alphabetLower.includes(char)) {
+    const value = (alphabetLower.indexOf(char) + shift) % 26;
+    return alphabetLower[value];
+  } else if (alphabetUpper.includes(char)) {
+    const value = (alphabetUpper.indexOf(char) + shift) % 26;
+    return alphabetUpper[value];
+  } else {
+    return char;
+  }


This is good and definitely should work. Though it may be optimised in a few ways:

For now each time we call the function rotateCharacter we check/iterate the same array of letters 2 times:

1st time when we call includes

2nd time when we call indexOf
Can we avoid using includes at all in this task and just use only indexOf and only once?
What this method returns us when there is no value we search in array?

In the worst case (e.g. the argument is a number) we will check 2 arrays of letters before returning original value back. But I think we can use one array of letters (e.g. alphabetLower) instead of two if we utilise the string methods toLowerCase() and toUpperCase()

Ahh I see, yes it makes the code more efficient. So that way we will turn the chat in to lower case and then check if it is included in the lower case array. Then shift the char and return upper case or lower case. When there is no matching element (char is not a letter) in the array we will get -1 and return the char itself

pseudopilot · 2024-04-30T21:43:08Z

week-3/refactor/format-as-12-hours.js

+  const hour = time.slice(0, 2);
+  if (Number(hour) > 12) {
+    return `${Number(hour) - 12}:${time.slice(3, 5)} pm`;


Very good! But we still can improve it. Do we really need to call Number(hour) twice or we can do it only once somehow?

We can assign it to a variable and use it multiple times. I couldn't see it 😔

pseudopilot · 2024-04-30T21:57:09Z

week-3/implement/get-card-value.js

+  const type = ["♣", "♦", "♥", "♠"];
+  let num = cards.slice(0, -1);
+
+  if (number.includes(num) && type.includes(cards.slice(-1))) {
+    if (num === "A") {
+      result = 11;
+    } else if (num === "J") {
+      result = 10;
+    } else if (num === "Q") {
+      result = 10;
+    } else if (num === "K") {
+      result = 10;
+    } else {
+      result = Number(num);
+    }
+    return result;
+  } else {
+    return "Invalid card rank.";
+  }
+}


This solution will definitely work! Though we can make it more effective.
Try rewriting it using Objects instead of Arrays for storing cards and their values like:

const cards = { '2': 2, '3': 3, ... }

We can use Object for card types too.
In this case we won't need to use includes and iterate through the whole list of cards every time.

we can just return

return card[cards.slice(0, -1)] && type[cards.slice(-1)] ? number[cards.slice(0, -1)] : "Invalid card rank." ;

🤯

pseudopilot · 2024-04-30T22:10:44Z

week-3/implement/is-proper-fraction.js

+  if (denominator === 0) {
+    return "Error (Denominator cannot be zero)";
+  } else if (
+    (fraction < 1 && fraction > 0) ||
+    (fraction > -1 && fraction < 0)
+  ) {
+    return true;
+  } else {
+    return false;
+  }
+}


It's all correct. But I have some ideas how to make it a little shorter:

if (denominator === 0) { return "Error (Denominator cannot be zero)"; } return (fraction > -1 && fraction < 1 && fraction !== 0); }

Let me explain you a couple of things here.
First, If the first check is true (denominator === 0) then the function returns an error and the rest of it won't be performed. If it is false then the function continue to work anyway. It means that we don't really need else here.

Second point is that comparison like fraction < 1 results in either true or false. It mean that we don't really need to write something like:

if (fraction > 1) { return true; } else { return false; }

We can just write return fraction > 1 and that's it.

In this task we have few conditions to check, but the approach may be the same: return (fraction > -1 && fraction < 1 && fraction !== 0); because the result of these 3 checks will be either true or false.

Using return to break the loop. That is brilliant 🤩 . I understand if the comparison returns true or false, there is no need to check with if statement.

pseudopilot · 2024-04-30T22:16:12Z

week-3/implement/is-valid-triangle.js

+  if (a <= 0 && b <= 0 && c <= 0) {
+    return false;
+  } else {
+    if (a + b > c && b + c > a && c + a > b) {
+      return true;
+    } else {
+      return false;
+    }
+  }
+}


This will definitely work! But again it may be improved:

a <= 0 && b <= 0 && c <= 0 - this check means that if all 3 sides are equal to 0, then it's not a triangle. But in fact it's not a triangle if either of the sides is equal to 0. Either a, or b, or c.

As for the rest of this function, could you try refactor it yourself using my advice from is-proper-fraction.js task?

so it's gonna be like

function isValidTriangle(a, b, c) { if (a <= 0 && b <= 0 && c <= 0) { return false; } return (a + b > c && b + c > a && c + a > b) }

What I actually meant was using || instead of && in the first if check:

if (a <= 0 || b <= 0 || c <= 0) { return false; } else { return (a + b > c && b + c > a && c + a > b); // for this line your refactoring is perfect!

or... we can even make it shorter:

return !(a <= 0 || b <= 0 || c <= 0) || (a + b > c && b + c > a && c + a > b);

but the last may be not very clear.

fikretellek

Thank you very much @pseudopilot for your time and brilliant code review. I've learned a lot 🙏

fikretellek · 2024-05-05T13:21:59Z

week-3/stretch/rotate-char.js

+  if (alphabetLower.includes(char)) {
+    const value = (alphabetLower.indexOf(char) + shift) % 26;
+    return alphabetLower[value];
+  } else if (alphabetUpper.includes(char)) {
+    const value = (alphabetUpper.indexOf(char) + shift) % 26;
+    return alphabetUpper[value];
+  } else {
+    return char;
+  }


Ahh I see, yes it makes the code more efficient. So that way we will turn the chat in to lower case and then check if it is included in the lower case array. Then shift the char and return upper case or lower case. When there is no matching element (char is not a letter) in the array we will get -1 and return the char itself

fikretellek · 2024-05-05T13:29:16Z

week-3/refactor/format-as-12-hours.js

+  const hour = time.slice(0, 2);
+  if (Number(hour) > 12) {
+    return `${Number(hour) - 12}:${time.slice(3, 5)} pm`;


We can assign it to a variable and use it multiple times. I couldn't see it 😔

fikretellek · 2024-05-05T13:36:36Z

week-3/implement/is-valid-triangle.js

+  if (a <= 0 && b <= 0 && c <= 0) {
+    return false;
+  } else {
+    if (a + b > c && b + c > a && c + a > b) {
+      return true;
+    } else {
+      return false;
+    }
+  }
+}


so it's gonna be like

function isValidTriangle(a, b, c) { if (a <= 0 && b <= 0 && c <= 0) { return false; } return (a + b > c && b + c > a && c + a > b) }

fikretellek · 2024-05-05T13:37:04Z

week-3/implement/is-proper-fraction.js

+  if (denominator === 0) {
+    return "Error (Denominator cannot be zero)";
+  } else if (
+    (fraction < 1 && fraction > 0) ||
+    (fraction > -1 && fraction < 0)
+  ) {
+    return true;
+  } else {
+    return false;
+  }
+}


Using return to break the loop. That is brilliant 🤩 . I understand if the comparison returns true or false, there is no need to check with if statement.

fikretellek · 2024-05-05T13:46:51Z

week-3/implement/get-card-value.js

+  const type = ["♣", "♦", "♥", "♠"];
+  let num = cards.slice(0, -1);
+
+  if (number.includes(num) && type.includes(cards.slice(-1))) {
+    if (num === "A") {
+      result = 11;
+    } else if (num === "J") {
+      result = 10;
+    } else if (num === "Q") {
+      result = 10;
+    } else if (num === "K") {
+      result = 10;
+    } else {
+      result = Number(num);
+    }
+    return result;
+  } else {
+    return "Invalid card rank.";
+  }
+}


we can just return

return card[cards.slice(0, -1)] && type[cards.slice(-1)] ? number[cards.slice(0, -1)] : "Invalid card rank." ;

🤯

fikret ellek added 2 commits January 2, 2024 22:45

working on week 3

7fb53a2

week 3 is done

b0c9b2f

JayMayer mentioned this pull request Apr 18, 2024

[TECH ED] Complete week 3 exercises fikretellek/Coursework-Planner#94

Open

pseudopilot requested changes Apr 30, 2024

View reviewed changes

fikretellek commented May 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NW-6 | Fikret Ellek | JS1| [TECH ED] Complete week 3 exercises | WEEK-3 #193

NW-6 | Fikret Ellek | JS1| [TECH ED] Complete week 3 exercises | WEEK-3 #193

fikretellek commented Jan 3, 2024 •

edited

Loading

pseudopilot left a comment

pseudopilot Apr 30, 2024

fikretellek May 5, 2024

pseudopilot Apr 30, 2024

fikretellek May 5, 2024

pseudopilot Apr 30, 2024

fikretellek May 5, 2024

pseudopilot Apr 30, 2024

fikretellek May 5, 2024

pseudopilot Apr 30, 2024

fikretellek May 5, 2024

pseudopilot May 15, 2024

fikretellek left a comment

fikretellek May 5, 2024

fikretellek May 5, 2024

fikretellek May 5, 2024

fikretellek May 5, 2024

fikretellek May 5, 2024

NW-6 | Fikret Ellek | JS1| [TECH ED] Complete week 3 exercises | WEEK-3 #193

Are you sure you want to change the base?

NW-6 | Fikret Ellek | JS1| [TECH ED] Complete week 3 exercises | WEEK-3 #193

Conversation

fikretellek commented Jan 3, 2024 • edited Loading

Learners, PR Template

Changelist

Questions

pseudopilot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fikretellek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fikretellek commented Jan 3, 2024 •

edited

Loading