London | Saba Farjamfard |Module-Structuring-and-Testing-data | Sprint 3 | Week 3 #160

sabafarjamfard · 2024-11-18T10:10:53Z

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.

SallyMcGrath

Hi @sabafarjamfard , thanks for this.

Instead of reviewing this file by file, I am just stopping here to say that you are writing console.log instead of writing assertions thoughout.

I think this coursework's underlying goal is to practice writing test first or "TDD" style code, where you make an assertion "I assert x is true/false", write a test for that assertion, then write just enough code to pass the test.

This is the real goal - these exercises are just to explore, practice, and develop this habit. I wouldn't redo the whole thing. My suggestion here is to pick one or two of these exercises and redo them, using this strategy explicitly, so you get the learning value out of this work. Good luck! Let me know how you find it.

SallyMcGrath · 2024-11-30T08:41:18Z

Sprint-3/implement/get-angle-type.js

+if (require.main === module) {
+    const getAngleType = require('./get-angle-type');
+
+    console.log(getAngleType(90) === "Right angle" ? "Pass" : "Fail");


There aren't any assertions in here. Can you go through a bit what your goal is here?

hello
Thanks for reviewing, I created a test file and I did all

SallyMcGrath · 2024-11-30T08:47:20Z

Sprint-3/implement/get-angle-type.js

+
+
+ //Test code 
+if (require.main === module) {


Oh hm, what's going on here?

SallyMcGrath · 2024-11-30T08:47:54Z

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

+
+
+
+if (require.main === module) {


I think we've got in a bit of a muddle here! 😅

It doesn't need to be this complicated. You can just write your assertions (and they should be assertions, not logs) without this check.

Hi,
Thank you for your feedback! As you suggested, I’ve simplified the code and ensured that all assertions are written properly without unnecessary checks

cjyuan

I left the comments in the code.

Why not prepare a Jest test script for every function you implemented in the revise folder?

cjyuan · 2024-11-30T10:32:33Z

Sprint-3/implement/get-angle-type.js

+if (require.main === module) {
+    const getAngleType = require('./get-angle-type');
+
+    console.log(getAngleType(90) === "Right angle" ? "Pass" : "Fail");


Could also test 0, a borderline case.

hello
thanks for reviewing , I create a test file for all

cjyuan · 2024-11-30T10:40:57Z

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

+    const cardRank = card[0]; // The first character represents the rank of the card
+
+    // If the card is a number between 2 and 9
+    if (cardRank >= '2' && cardRank <= '9') {


Part of the description mentions:

// Handle Invalid Cards: // Given a card with an invalid rank (neither a number nor a recognized face card),

So that implies the rank could possibly be "20" or "B", or "111".
If these ranks are possible, how would you modify your code to treat them as invalid rank? (Just checking the first character would not not be enough)

The description does not say anything about the suite (the last character), so we can presume the last character is always a valid suite character.

cjyuan · 2024-11-30T10:54:31Z

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

+if (require.main === module) {
+    try {
+        console.log(getCardValue("5") === 5 ? "Pass" : "Fail"); // Numeric card 5
+        console.log(getCardValue("9") === 9 ? "Pass" : "Fail"); // Numeric card 9


The value passed to the function should always have a suite character as the last character. For examples, "A♠", "10♠". You can copy the character in the description (or just substitute the suite character by another character since the function is not required to validate the suite character).

Using a boundary value such as "2♠" as a test case is always a good idea. Many program fails because they do not check the boundary cases properly.

cjyuan · 2024-11-30T11:01:35Z

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

+    }
+
+    // Check if numerator and denominator are equal (not a proper fraction)
+    if (Math.abs(numerator) === Math.abs(denominator)) {


This check is optional because Math.abs(numerator) < Math.abs(denominator) is false when both values are equal.

cjyuan · 2024-11-30T11:04:17Z

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

+    }
+
+    // Check the Triangle Inequality conditions
+    if (a + b > c && a + c > b && b + c > a) {


Since after line 45, the return value is determined only by the value of (a + b > c && a + c > b && b + c > a), we can also just write return (a + b > c && a + c > b && b + c > a);.

cjyuan · 2024-11-30T12:10:35Z

Sprint-3/revise/implement/is-prime.test.js

+    }
+
+    // Check if num is divisible by any number between 2 and sqrt(num)
+    for (let i = 2; i <= Math.sqrt(num); i++) {


You can possibly improve the performance of the code in the following manners:

Check if num is 2, and check only odd numbers >= 3 in the loop

Avoid calling Math.sqrt(num) repeatedly by assigning the value of Math.sqrt(num) to a variable once, and then refer to the variable in the condition of the loop.

Note: The condition is checked at the start of every iteration.

cjyuan · 2024-11-30T12:15:18Z

Sprint-3/revise/implement/password-validator.test.js

+  }
+
+  // Rule 5: It must have at least one non-alphanumeric symbol (e.g., !, #, $, %, etc.)
+  if (!/[\W_]/.test(password)) {


Please note that \W also matches spaces. It real application, we would probably want to avoid using it.

cjyuan · 2024-11-30T12:20:23Z

Sprint-3/revise/implement/password-validator.test.js

+}
+
+// Example of usage:
+const previousPasswords = ["Password123!", "12345678", "helloWorld!"];


Some of the passwords in this array are invalid passwords.

cjyuan · 2024-11-30T12:41:05Z

Sprint-3/revise/implement/password-validator.test.js

+const previousPasswords = ["Password123!", "12345678", "helloWorld!"];
+console.log(isValidPassword("NewPassword1!", previousPasswords)); // Expected: true
+console.log(isValidPassword("password123", previousPasswords));  // Expected: false
+console.log(isValidPassword("1234", previousPasswords));        // Expected: false


These test cases are not comprehensive.

Because the function can return false as soon as one of the checks in the function fails,
even if isValidPassword("1234", previousPasswords) returns false, the test cannot indicate exactly why the function returns false. For example, the function actually failed to check for "password too short" but it still returned false because "1234" does not contain any letter.

We should use test values that only test a specific case. For example, to test if the function can correctly check "password has >= 5 characters", we should use a string like "Ab1$" as test value because it should pass all other checks in the function except check for "password >= 5".

Hi,
Thank you for your feedback earlier! I’ve reviewed and updated the code and test cases to ensure that each condition is tested individually. The tests comprehensively cover all the validation rules, including cases like length, uppercase, lowercase, digits, special characters, and previously used passwords.

cjyuan · 2024-11-30T12:56:22Z

Sprint-3/revise/implement/repeat.test.js

+    }
+
+    // Return the repeated string
+    return str.repeat(count);


Why not simply use str.repeat(count) when count >= 0?

sabafarjamfard added 14 commits November 14, 2024 16:05

Completing sprint-3/Implement/get-angle-type.js

0ef3b40

completing sprint-3/implement/get-card-value.js

879e1e8

completing sprint-3/implement/is-proper-fraction.js

a8532e5

completing sprint-3/implement/is-valid-triangle.js

fd4e76d

completing sprint-3/implement/rotate-char.js

6e960af

completing sprint-2/revise/implement/count.test.js

7b3d968

completing sprint-3/revise/implement/count.test.js

404c59a

completing sprint-3/revise/implement/card-validator-1.js

eed572c

completing sprint-3/revise/implement/card-validator-1.js

01b0e55

completing sprint-3/revise/implement/ get-ordinal-number.test.js

506356d

completing sprint-3/revise/implement/ is-prime.test.js

e836797

completing sprint-3/revise/implement/password-validator.test.js

bd1d4dd

completing sprint-3/revise/implement/repeat.test.js

5bae5d3

completing sprint-3/revise/investigate/find.js

27044bc

sabafarjamfard added the Needs Review Participant to add when requesting review label Nov 18, 2024

sabafarjamfard requested a review from codrex November 25, 2024 15:30

sabafarjamfard added Needs Review Participant to add when requesting review and removed Needs Review Participant to add when requesting review labels Nov 27, 2024

Merge branch 'main' into sprint-3-exercise-3

2baca3b

SallyMcGrath reviewed Nov 30, 2024

View reviewed changes

SallyMcGrath added 👀 Review Testing Changes requested to do with testing Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Nov 30, 2024

cjyuan reviewed Nov 30, 2024

View reviewed changes

sabafarjamfard added 2 commits December 2, 2024 11:05

completing/sprint-3-all-test

119cdf5

merged sprint-3-exercise-3 into local branch

0fb5862

sabafarjamfard added Needs Review Participant to add when requesting review and removed Needs Review Participant to add when requesting review labels Dec 2, 2024

completing/Sprint-3/implement/is-valid-triangle.js

f31f756

sabafarjamfard added the Complete Volunteer to add when work is complete and review comments have been addressed label Dec 5, 2024

Uh oh!

London | Saba Farjamfard |Module-Structuring-and-Testing-data | Sprint 3 | Week 3 #160

Are you sure you want to change the base?

London | Saba Farjamfard |Module-Structuring-and-Testing-data | Sprint 3 | Week 3 #160

Uh oh!

Conversation

sabafarjamfard commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

Changelist

Questions

Uh oh!

SallyMcGrath 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!

cjyuan 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sabafarjamfard commented Nov 18, 2024 •

edited

Loading