London | Phone_Naing |Module-structuring-and-testing-data/ sprint 3 #180

Mgphone · 2024-11-21T01:44:38Z

Learners, PR Template

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

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

MonaZaresh · 2024-11-24T21:51:11Z

Sprint-3/implement/rotate-char.js

+    return capitalLetter[capitalLetterIndex];
+  } else if (isAlphabetSmall) {
+    const smallLetterIndex =
+      (capitalSplit.indexOf(alphabet) + number) % smallLetter.length;


Correct Indexing for Lowercase Letters. Replace: smallSplit
Because here you are checking Lowercase Letters.

Thanks you are Legend i did change :)

MonaZaresh · 2024-11-24T21:52:24Z

Sprint-3/implement/rotate-char.js

+  const smallSplit = smallLetter.split("");
+  const isAlphabetCapital = capitalSplit.includes(alphabet);
+  const isAlphabetSmall = smallSplit.includes(alphabet);
+  if (typeof number !== "number") {


It seems unnecessary based on the acceptance criteria.

Yeah that is truth , I added the number check just in case. I can commented out to keep the code simple

MonaZaresh · 2024-11-24T22:00:24Z

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

+}
+console.log(isValidTriangle(3, 3, 3));
+
+module.exports = isValidTriangle;


The code does not check if any of the sides are less than or equal to zero.

Please guide me, when i check jest all are correct...
test("Is a valid triangle", () => {
expect(isValidTriangle(3, 3, 3)).toBe(true);
});

test("Invalid Triangle with Negative", () => {
expect(isValidTriangle(1, -2, 1)).toBe(false);
});

test("Invalid checking triangle with zero sides", () => {
expect(isValidTriangle(0, 1, 2)).toBe(false);
});
test("checking invalid", () => {
expect(isValidTriangle(1, 2, 3)).toBe(false);
});

return a + b > c && b + c > a && a + c > b;
to make it clear you want me to check if any parameter is zero or minus make it return to false?

@MonaZaresh
Yesterday, while trying to find some non-positive values that could fail the test a + b > c && b + c > a && a + c > b, I realised that in JS checking if a, b, c are all positives is not needed. This is because JS uses only double precision floating point type to represent numbers, so we don't have to worry about "integer overflow".

In other programming languages that support integer types, if a, b, c are integer types, then we would need to check if a, b, c are all positives in addition to checking a + b > c && b + c > a && a + c > b.

Thanks, that mean my code for javascript is correct right :)

MonaZaresh · 2024-11-24T22:06:58Z

Sprint-3/implement/test/is-valid-traingle.test.js

+  expect(isValidTriangle(3, 3, 3)).toBe(true);
+});
+
+test("Invalid Triangle with Negative", () => {


This test is correct. However, your code would not pass this test because it doesn't include a check for negative side lengths.

return a > 0 && b > 0 && c > 0 && a + b > c && b + c > a && a + c > b;
is it good right ? for negative check

MonaZaresh · 2024-11-24T22:07:27Z

Sprint-3/implement/test/is-valid-traingle.test.js

+  expect(isValidTriangle(1, -2, 1)).toBe(false);
+});
+
+test("Invalid checking triangle with zero sides", () => {


Your code will fail this test because it doesn't check for zero side lengths.

i just add to >0 to check negative

MonaZaresh · 2024-11-24T22:21:55Z

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

+      : !isNaN(checkingCard) && checkingCard >= 2 && checkingCard <= 10
+      ? Number(checkingCard)
+      : false;
+  return isEmojiValid && valueOfCard ? valueOfCard : "Invalid card rank";


This part of the code does not properly handle multi-character ranks (like "10") or validate the full card format (rank + suit), and it only checks the rank and emoji separately, rather than together as a complete card.

Sure I can do with One Parameter Pass 💯 instead of two parameter pass

MonaZaresh · 2024-11-24T22:25:01Z

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

@@ -0,0 +1,4 @@
+const getCardValue = require("../get-card-value");


The function is not designed to take two separate parameters, so the test is inconsistent with the current implementation.

change it to one parameter

cjyuan

I added my comment in your code.

cjyuan · 2024-12-01T16:48:03Z

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

+      ? 11
+      : ["K", "Q", "J", "10"].includes(rank)
+      ? 10
+      : !isNaN(rank) && rank >= 2 && rank <= 9


Why do you decide to treat "02❤️", "002❤️", and "0x02❤️" the same as "2❤️", but does treat "010❤️" the same as "10❤️"?

yeah i did check and all have got same value :)

cjyuan · 2024-12-01T17:09:23Z

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

+}
+console.log(isValidTriangle(3, 3, 3));
+
+module.exports = isValidTriangle;


@MonaZaresh
Yesterday, while trying to find some non-positive values that could fail the test a + b > c && b + c > a && a + c > b, I realised that in JS checking if a, b, c are all positives is not needed. This is because JS uses only double precision floating point type to represent numbers, so we don't have to worry about "integer overflow".

In other programming languages that support integer types, if a, b, c are integer types, then we would need to check if a, b, c are all positives in addition to checking a + b > c && b + c > a && a + c > b.

cjyuan · 2024-12-01T17:14:21Z

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

+});
+test("Invalid card rank", () => {
+  expect(() => getCardValue("l❤️")).toThrow("Invalid card rank");
+});


Why not test also 9, 10, J, Q, K, A (and maybe some of the wild cases I suggested earlier) to make the test more comprehensive?

Thanks and all working fine :)

cjyuan · 2024-12-01T17:20:34Z

Sprint-3/implement/test/rotate-char.test.js

@@ -0,0 +1,8 @@
+const rotateCharacter = require("../rotate-char");
+test("first argument Invalid Alphabet", () => {
+  expect(rotateCharacter("2", 2)).toBe("2");


Why not test if the function can really rotate a letter (uppercase and lowercase), and also rotate correctly when shift is a large number such as 100?

I wonder what would happen if shift is -1. Can the function rotate a letter in the opposite direction?

Thanks I did add more to check :) and correct

cjyuan · 2024-12-01T17:23:57Z

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

 // And a character char that does not exist within the case-sensitive str,
 // When the function is called with these inputs,
 // Then it should return 0, indicating that no occurrences of the char were found in the case-sensitive str.
+const countChar = (str, char) => {


I think you overdone. The function is expected only to return the value of count as a number, and not as a nicely formatted string. (You don't have to make any changes)

cjyuan · 2024-12-01T17:44:36Z

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

+  }
+  return true;
+};
+test("checking negative number", () => {


Is 1 a prime number? Is 0 a prime number?

wow thanks very much.. i am wrong and i fixed it zero and 1 is not prime

cjyuan · 2024-12-01T17:46:51Z

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

+});
+test("checking not prime number", () => {
+  expect(isPrimeNumber(4)).toBe(false);
+});


Can also test composite numbers in the following manner:
expect(isPrimeNumber(7*11)).toBe(false);

okay i just knew i can check like this as well :)

cjyuan · 2024-12-01T17:52:16Z

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

+  //   return newPassword;
+  //this is for upperCase Regex
+  const upperCaseRegex = /[A-Z]/g;
+  const isCharContainOneCapital = !!newPassword.match(upperCaseRegex);


Can also use the .test() method of a regular expression to check if part of a string matches the regular expression in the following manner:

! /[A-Z]/.test(newPassword)

cjyuan · 2024-12-01T17:57:23Z

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

+    isCharContainOneNumber &&
+    isCharContainOneSpecialLetter &&
+    !isPasswordMatchOld
+    ? "Password Valid"


If the function returns a Boolean value true or false, then the function could be used in the following manner:

if (checkPasswordValidation("aA123$")) ...

instead of

if (checkPasswordValidation("aA123$") == "Password Valid") ...

@cjyuan that is good to know ,i should avoid to use ! true

I meant to say, it is ok to keep const isPasswordMatchOld = previousPassword.includes(newPassword);

but return true/false from your function, as in

return isCharAtLeaseFive && ... && !isPasswordMatchOld ? true : false;

or even

return isCharAtLeaseFive && ... && !isPasswordMatchOld;

cjyuan · 2024-12-01T18:03:14Z

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

+test("Checking Invalid", () => {
+  expect(checkPasswordValidation("abcd")).toBe("Password Invalid");
+});
+


There are more invalid cases to test. Otherwise how can we be sure the implemented function can correctly identify "aA12314" and "A$.$$$A" are an invalid password?

i check it for A$.$$$A :)

cjyuan

password-validator.test.js can use some improvement.

cjyuan · 2024-12-11T12:36:52Z

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

+    isCharContainOneNumber &&
+    isCharContainOneSpecialLetter &&
+    !isPasswordMatchOld
+    ? "Password Valid"


I meant to say, it is ok to keep const isPasswordMatchOld = previousPassword.includes(newPassword);

but return true/false from your function, as in

return isCharAtLeaseFive && ... && !isPasswordMatchOld ? true : false;

or even

return isCharAtLeaseFive && ... && !isPasswordMatchOld;

cjyuan · 2024-12-11T12:56:29Z

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

+test("Checking Invalid", () => {
+  expect(checkPasswordValidation("abcd")).toBe("Password Invalid");
+});


Assuming you have changed your function to return true/false.

expect(checkPasswordValidation("abcd")).toBe(false);
When this test fails (that is, checkPasswordValidation("abcd") returns true), we cannot be certain exactly why the function returns true; it could be one or more of the following reasons:

The function fails to check if the password is long enough.

The function fails to check if the password contains a digit.

The function fails to check if the password contains an non-alphanumeric character.

The function fails to check if the password contains an uppercase letter.

A better test setup would be

test("Expect false when password does not contain any uppercase letter", () => { // "ab$1xyz" meets all requirements except that is does not contain // an uppercase letter. expect(checkPasswordValidation("ab$1xyz")).toBe(false); }); test("Expect false when password length is less than 5", () => { // "aB$1" has all the required characters but too short expect(checkPasswordValidation("aB$1")).toBe(false); }); ...

Mgphone added 10 commits November 12, 2024 21:58

fixed the debug

bd7dae1

error fixed

291b2cd

update extend

ee8c6f7

implement function changes

85c1a02

time-format

e3a68ad

implement done

3752b21

add jest and test the implement and rotate char change spelling variable

cc657c4

half way to jest implement

f45a428

test for revise implement done in jest

a4edfb1

add answer for findjs

1d1b15c

Mgphone added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 21, 2024

MonaZaresh reviewed Nov 24, 2024

View reviewed changes

updated code based on feedback

dd1643e

cjyuan reviewed Dec 1, 2024

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Dec 1, 2024

resolve all the issues with review

0093253

cjyuan reviewed Dec 11, 2024

View reviewed changes

Mgphone added the Complete Volunteer to add when work is complete and all review comments have been addressed. label Dec 16, 2024

illicitonion closed this Sep 1, 2025

		@@ -0,0 +1,4 @@
		const getCardValue = require("../get-card-value");

Uh oh!

London | Phone_Naing |Module-structuring-and-testing-data/ sprint 3 #180

London | Phone_Naing |Module-structuring-and-testing-data/ sprint 3 #180

Uh oh!

Conversation

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

Uh oh!

Learners, PR Template

Changelist

Questions

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!

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!

Mgphone Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Mgphone commented Nov 21, 2024 •

edited

Loading

Mgphone Dec 10, 2024 •

edited

Loading