Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job getting the core tests for the ifElse function in place! The tests for the true and false conditions are working correctly.
However, this will require a small change to be more complete. The current tests don't cover what happens when the condition function returns a "truthy" value that isn't strictly true (e.g., 1 or 'hello'). Because the implementation uses a strict === true check, these truthy values would incorrectly trigger the second callback.
Please add a new test case to verify this behavior and ensure your test suite is robust enough to catch this edge case.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| }); | ||
|
|
||
| // write tests here | ||
| it(`the second function must be called`, () => { |
There was a problem hiding this comment.
This test correctly handles the false case. To make your tests more robust, consider adding another test case that checks what happens when the condition function returns a 'truthy' value that isn't strictly true (e.g., 1 or 'hello'). According to the implementation's === true check, the second callback should still be invoked, and it's important to verify this behavior.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job adding the new test case for truthy values! However, a small but critical adjustment is needed. Your current test uses const condition = () => Boolean(1);, which evaluates to the strict boolean true. This makes the test a duplicate of the one checking for a true condition, and it doesn't verify how the function handles other truthy values.
To fix this, please update the test so the condition returns a non-boolean truthy value directly, such as () => 1 or () => 'hello'. This change will likely cause the test to fail, which will correctly point to a small bug in your ifElse.js implementation that also needs to be addressed.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/ifElse.test.js
Outdated
| }); | ||
|
|
||
| it(`if function returns a 'truthy' the first function must be called`, () => { | ||
| const condition = () => Boolean(1); |
There was a problem hiding this comment.
This test is a great start, but Boolean(1) evaluates to the strict boolean true, so it's not testing for a "truthy" value that isn't true. To properly test this behavior, the function should return a truthy value directly, like () => 1 or () => 'hello'. This will help verify that the ifElse implementation handles all truthy values, not just true.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job updating the test case to use a non-boolean truthy value—you're very close to the solution! I am providing an educational approval to help you move forward, but there's a key concept to address.
Your test for the 'truthy' case currently expects the second function to run. However, standard JavaScript if/else logic executes the first block for any truthy value (like 'hello'), not just the boolean true.
To fix this, first adjust your test in src/ifElse.test.js to assert that the first function is called. This will correctly cause the test to fail, revealing the bug. Then, you can fix the implementation in ifElse.js by changing the condition from a strict === true check to a general truthiness check (if (condition())), which will make your corrected test pass. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| it(`if function returns a 'truthy' | ||
| the second function must be called`, () => { |
There was a problem hiding this comment.
The title of this test is misleading. In JavaScript, it's standard for conditions to check for 'truthiness', not just strict equality with true. Therefore, if the condition function returns any truthy value (like 'hello'), the first function should be called.
| }; | ||
|
|
||
| ifElse(condition, first, second); | ||
| expect(count).toBe(2); |
There was a problem hiding this comment.
Based on the standard behavior for truthy values, this should expect count to be 1. Making this change will correctly cause the test to fail, highlighting a small bug in the ifElse function's implementation where it's checking for === true instead of just the truthiness of the result.
3 participants
No description provided.