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

CYF-ITP - South Africa | Simphiwe Mabuya | Structuring-and-Testing-Data | Sprint-3 #215

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

Conversation

Simphiwe-Mabuya
Copy link

@Simphiwe-Mabuya Simphiwe-Mabuya commented Dec 2, 2024

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.

@Simphiwe-Mabuya
Copy link
Author

Simphiwe-Mabuya commented Dec 3, 2024 via email

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 let you new comments.

@@ -6,6 +6,21 @@
// When the function getAngleType is called with this angle,
// Then it should:

function getAngleType(degrees) {
if (degrees === 90) return "Right angle";
if (degrees < 90) return "Acute angle";
Copy link

Choose a reason for hiding this comment

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

What do you expect getAngleType(-5) to return?

});


test("Expect 'Reflect angle' when 180 < angle < 360", () => {
Copy link

Choose a reason for hiding this comment

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

I was just making a suggestion so that you can try to keep the test description concise in the future.

Copy link
Author

@Simphiwe-Mabuya Simphiwe-Mabuya Dec 4, 2024

Choose a reason for hiding this comment

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

Thanks for the tip CJ ..I thought it was best I used it.

@@ -2,6 +2,37 @@

// You will need to implement a function getCardValue

function getCardValue(cardRank){
Copy link

Choose a reason for hiding this comment

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

getCardValue("10000♣") would still break your implementation. (sorry)

The card value should always have ONE suite character as the LAST character. So
getCardValue("10") should return 'Invalid card rank'.

Since there are exactly 13 valid ranks but countless possible invalid ranks,
it would be easier to consider what value constitutes a valid rank
instead of considering what values are invalid.
After you have checked all valid cases, everything else would be invalid.

@@ -2,6 +2,37 @@

// You will need to implement a function getCardValue

function getCardValue(cardRank){

if(cardRank === "10"){
Copy link

Choose a reason for hiding this comment

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

If you can remove the suite character from cardRank first, this is a good way to guarantee the return value is correct.

return 10;
}

if(/^0\d/.test(cardRank) || /\.\d/.test(cardRank)){
Copy link

Choose a reason for hiding this comment

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

You can skip checking for possible invalid cases.

return "Invalid card rank";
}

cardRank = cardRank.slice(0, 2) === "10" ? "10" : cardRank.slice(0, 1);
Copy link

Choose a reason for hiding this comment

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

This part should be replaced by a statement that removes the suite character from the string, and should be perform at the beginning of the function.



test("Checks if the number has 16 digits.", () => {
expect(cardValidator("1243578569873476")).toEqual(true)
Copy link

@cjyuan cjyuan Dec 4, 2024

Choose a reason for hiding this comment

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

The function can successfully check if there are 16 digits in the card number does not mean that the function can return false when the card number has 15 or 20 digits.

Similarly, you should also test if the function can return false when

  • the card number do not have "at least two different digits"
  • the sum of the digits is less than or equal 16

In addition to these tests, there are another two tests you could have to make your test more comprehensive.

@@ -0,0 +1,22 @@
function isPrime(num){
Copy link

Choose a reason for hiding this comment

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

Why not design the function to return true or false to indicate if the given number is a prime?


test('Checks if password has at least one English uppercase letter (A-Z)', () => {
const passwords =[];
expect(passwordValidator("Bad@11.#YoZ%", passwords)).toBe(true);
Copy link

Choose a reason for hiding this comment

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

When given a valid password, the function can correctly return true. (This is what your test represents)

When given an invalid password, can the function correctly return false? (This is what I would like you to test)
For examples, a password missing only a digit, a password missing only an alphanumeric character, etc.

If you prepare the tests correctly, you may discover the bug in your code.

}

if(passwords.includes(password)){
return "Password is already used"
Copy link

Choose a reason for hiding this comment

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

Better to return false to make the return value consistent.

@Simphiwe-Mabuya
Copy link
Author

Simphiwe-Mabuya commented Dec 4, 2024 via email

@Simphiwe-Mabuya
Copy link
Author

Simphiwe-Mabuya commented Dec 4, 2024 via email

@cjyuan
Copy link

cjyuan commented Dec 4, 2024

The tests in Sprint-3/revise/implement/password-validator.test.js are still not comprehensive enough to detect the bug in your function.

For a certain kind of invalid passwords, your current function is returning true instead of false. If you cannot figure out which test cases you missed or find the bug in your code, you can test different kinds of invalid passwords to see which kind of invalid passwords your function cannot detect.

@Simphiwe-Mabuya
Copy link
Author

Simphiwe-Mabuya commented Dec 4, 2024 via email

@Simphiwe-Mabuya
Copy link
Author

Simphiwe-Mabuya commented Dec 4, 2024 via email

@Simphiwe-Mabuya
Copy link
Author

Simphiwe-Mabuya commented Dec 9, 2024 via email

@cjyuan
Copy link

cjyuan commented Dec 10, 2024

Yes. All is good.

@Simphiwe-Mabuya
Copy link
Author

Simphiwe-Mabuya commented Dec 10, 2024 via email

@Simphiwe-Mabuya Simphiwe-Mabuya added the Complete Participant to add when work is complete and review comments have been addressed label Dec 10, 2024
@Simphiwe-Mabuya Simphiwe-Mabuya removed the Reviewed Volunteer to add when completing a review label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Participant to add when work is complete and review comments have been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TECH ED] Complete Sprint 3 exercises
2 participants