Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Millena mesfin/Sprint-3-exercises/structuring and testing data #222

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Millena28
Copy link

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.

Copy link
Author

@Millena28 Millena28 left a comment

Choose a reason for hiding this comment

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

Hi i would appreciate if someone could review my PR?

@Millena28 Millena28 added the Needs Review Participant to add when requesting review label Dec 9, 2024
Copy link

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

I think you missed implementing several functions in revise/implement.

Also, the function in get-card-value.js cannot yet deal with some invalid cases.

@@ -4,6 +4,45 @@

// You need to write assertions for your function to check it works in different cases

function getCardValue(card) {
// Check if the card is a number between 2 and 9
if (Number(card[0]) >= 2 && Number(card[0]) <= 9) {
Copy link

Choose a reason for hiding this comment

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

Since rank can be any invalid value, you cannot just check the first character.
Examples of cards with invalid rank include "23♠", "777♠", "10K♠", "AA♠"


console.log(isProperFraction(2, 3));
console.log(isProperFraction(5, 2));
console.log(isProperFraction(-4, 7));
Copy link

Choose a reason for hiding this comment

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

In mathematics, -4/7 == 4/-7, and -4/-7 == 4/7.
So, ideally isProperFraction() should recognise all of them as proper fractions.

Hint: If you compute the absolute value of both parameters inside the function first, the code can become much simpler.

if (a <= 0 || b <= 0 || c <= 0) {
return false;
}
if (a + b > c && a + c > b && b + c > a) {
Copy link

Choose a reason for hiding this comment

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

If any of a, b, c are negatives, then they will also not pass the triangle inequality check. (You can verify yourself)
So why also include the if statements in lines 39-41?

@@ -41,3 +41,34 @@ console.log(rotateCharacter("7", 5)); // Output: "7" (unchanged, not a letter)
// And the function should return the rotated character as a string (e.g., 'z' rotated by 3 should become 'c', 'Z' rotated by 3 should become 'C').
console.log(rotateCharacter("z", 1)); // Output: "a" (preserves case, but wraps around)
console.log(rotateCharacter("Y", 2)); // Output: "A" (preserves case, but wraps around)

function rotateCharacter(char, n) {
if (!/^[a-zA-Z]$/.test(char)) return "Unchanged, not a letter";
Copy link

Choose a reason for hiding this comment

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

The spec means, if char is not a letter, return char.

newCharCode = newCharCode - 26;
return `Output: ${String.fromCharCode(
newCharCode
)} (preserves case, but wraps around)`;
Copy link

Choose a reason for hiding this comment

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

Why return such a long string?

@@ -33,3 +33,41 @@ These are the requirements your project needs to fulfill:
- Return a boolean from the function to indicate whether the credit card number is valid.

Good luck!


function validateCreditCard(cardNumber) {
Copy link

Choose a reason for hiding this comment

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

Can you implement this in card-validator.js and prepare the test script in card-validator.test.js?

test("handles special characters", () => {
expect(countChar("!@#$$%^^&", "$")).toBe(2);
});
});
Copy link

Choose a reason for hiding this comment

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

Where is the function this test script is testing?

expect(isPrime(100)).toBe(false);
expect(isPrime(150)).toBe(false);
});
});
Copy link

Choose a reason for hiding this comment

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

I think you were also supposed to implement the function isPrime(num) (and not just prepare the tests).

test("should return true if password length is at least 5 characters", () => {
expect(isValidPassword("abcde", [])).toBe(true);
});
});
Copy link

Choose a reason for hiding this comment

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

Can you also implement the function isValidPassword()?

These tests are not comprehensive. For example, it didn't test if the function can correctly check if a password is missing only an uppercase letter.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants