-
-
Notifications
You must be signed in to change notification settings - Fork 195
LONDON | MAY2025 | JESUS DEL MORAL | STRUCTURING AND TESTING DATA SPRINT 3 #581
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
base: main
Are you sure you want to change the base?
LONDON | MAY2025 | JESUS DEL MORAL | STRUCTURING AND TESTING DATA SPRINT 3 #581
Conversation
a2a857e
to
91085e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your PR is on the right track. There are a few functional requirements we still need to address - I've left comments in your PR to help you about these. 🙂
There are 3 files that shouldn't be included in this PR since they aren't modified for this exercise:
- pull_request_template.md
- package-lock.json
- package.json
I don't see the tests for Sprint-3/3-mandatory-practice. Is it possible that you forgot these?
const right = getAngleType(90); | ||
console.log(right); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 It's a good idea to remove these console logs before submitting your PR since they are not required for the exercise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed all the console logs
@@ -27,27 +30,37 @@ function assertEquals(actualOutput, targetOutput) { | |||
// Explanation: The fraction 2/3 is a proper fraction, where the numerator is less than the denominator. The function should return true. | |||
const properFraction = isProperFraction(2, 3); | |||
assertEquals(properFraction, true); | |||
console.log(properFraction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may have forgotten to remove these console logs before submitting your PR. Can you remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the console logs has been removed
if (numerator < denominator) return true; | ||
} | ||
|
||
if (numerator < denominator) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌱 Consider this: you can return a boolean value of true or false by doing a simple check of the values themselves.
i.e.
return numerator < denominator;
This will return true if numerator is less than denominator. It will return false if numerator greater than or equal to the denominator
You can add a check for zero before any other checks. If the denominator is zero, you can return false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have simplified the function
function isProperFraction(numerator, denominator) {
if (denominator == 0){
return false
}
return numerator < denominator;
}
// Case 4: Identify Equal Numerator and Denominator: | ||
|
||
|
||
test("should return True for a proper fraction", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you review the test assertion text here? You are saying you expect this proper fraction to return true but you are checking for 'false'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified the test and the function
test("should return True for a proper fraction", () => {
expect(isProperFraction(3, 3)).toEqual(true);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check the instruction explanations for a proper fraction again. I see this:
Explanation: The fraction 3/3 is not a proper fraction because the numerator is equal to the denominator. The function should return false.
Your test is not validating the correct thing since 3/3 should return an improper fraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test has been modified
test("should return False for a proper fraction", () => {
expect(isProperFraction(3, 3)).toEqual(false);
});
const invalid = getCardValue("-1"); | ||
expect(invalid).toEqual('Invalid card rank.'); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get the expected value when I tested these scenarios:
test("should return the value 10 for 10 of hearts ", () => {
const tenOfHearts = getCardValue("10♥");
expect(tenOfHearts).toEqual(10);
});
test("should return the value Invalid card rank for value of 22", () => {
const twentyTwo = getCardValue("22");
expect(twentyTwo).toEqual("Invalid card rank.");
});
Can you investigate and see if you can find the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have included these two scenarios and tested them. All of them working fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@delmorallopez - can you review your function again? The change you made (returning the value of the test within the function is not the right approach here.
Here are some tips to help you out:
- Get the suit: the last character of the string will be the suit (♥, ♠, etc).
- Get the rank: the part of the string before the suit is going to be either a number or 'A', 'J', 'Q', or 'K'.
- if the rank is 'A', the value is always 11.
- if the rank is any of these: 'J', 'Q', or 'K', the value of the card is 10.
- if the rank value is 2,3,4,5,6,7,8,9,10 -> then the card value is the rank.
- anything else is an 'invalid card value'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified the getCardValue function considering all the scenarios
All the tests have been passed
function getCardValue(card) {
if (typeof card !== 'string' || card.length < 2) {
return 'Invalid card rank.';
}
const validSuits = ['♠', '♥', '♦', '♣'];
const suit = card.slice(-1); // Last character
const rank = card.slice(0, -1); // Everything before last character
if (!validSuits.includes(suit)) {
return 'Invalid card rank.';
}
if (rank === 'A') {
return 11;
} else if (['J', 'Q', 'K'].includes(rank)) {
return 10;
} else if (!isNaN(rank)) {
const numericRank = Number(rank);
if (numericRank >= 2 && numericRank <= 10) {
return numericRank;
}
}
return 'Invalid card rank.';
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@delmorallopez - Can you please review all of the comments in the PR? There were quite a few that were not addressed.
If you're struggling with a specific part of this exercise, don't forget that there is a community of people on Slack who can help. 😃
A good tip is to give yourself about 1 hour and then reach out for help! You want to give yourself enough time to see if you can find the solution - but you don't want to get frustrated by getting stuck on a problem for too long.
return 11; | ||
} | ||
let rank = card.charAt(0); | ||
console.log(rank); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This console.log should be removed from the PR. Console logs can:
- impact performance
- clutter the browser console
- look unprofessional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console log is still in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jenny-alexander
The console logs has been removed
@@ -8,7 +8,22 @@ | |||
// write one test at a time, and make it pass, build your solution up methodically | |||
// just make one change at a time -- don't rush -- programmers are deep and careful thinkers | |||
function getCardValue(card) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you review the above☝️failing tests and implement something within your function to ensure that they don't fail!
@@ -8,7 +8,22 @@ | |||
// write one test at a time, and make it pass, build your solution up methodically | |||
// just make one change at a time -- don't rush -- programmers are deep and careful thinkers | |||
function getCardValue(card) { | |||
if (rank === "A") return 11; | |||
let rank = card.charAt(0); | |||
console.log(rank); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@delmorallopez - console log (line 12) is still in your function. Please review.
// Case 4: Identify Equal Numerator and Denominator: | ||
|
||
|
||
test("should return True for a proper fraction", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check the instruction explanations for a proper fraction again. I see this:
Explanation: The fraction 3/3 is not a proper fraction because the numerator is equal to the denominator. The function should return false.
Your test is not validating the correct thing since 3/3 should return an improper fraction.
const invalid = getCardValue("-1"); | ||
expect(invalid).toEqual('Invalid card rank.'); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@delmorallopez - can you review your function again? The change you made (returning the value of the test within the function is not the right approach here.
Here are some tips to help you out:
- Get the suit: the last character of the string will be the suit (♥, ♠, etc).
- Get the rank: the part of the string before the suit is going to be either a number or 'A', 'J', 'Q', or 'K'.
- if the rank is 'A', the value is always 11.
- if the rank is any of these: 'J', 'Q', or 'K', the value of the card is 10.
- if the rank value is 2,3,4,5,6,7,8,9,10 -> then the card value is the rank.
- anything else is an 'invalid card value'.
I have added the test for Wrong Angle // Case 6: Identify Wrong Angles: const wrong = getAngleType(400); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review this requested change:
https://github.com/CodeYourFuture/Module-Structuring-and-Testing-Data/pull/581/files#r2216678978
@jenny-alexander return Math.abs(numerator) < Math.abs(denominator); All test passed |
LONDON | MAY2025 | JESUS DEL MORAL | STRUCTURING AND TESTING DATA SPRINT 3
I have committed my files one by one, on purpose, and for a reason
I have titled my PR with REGION | COHORT_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
I have tested my changes
My changes follow the style guide
My changes meet the requirements of this task