Challenge/1 even odd range rouaa #52
Conversation
There was a problem hiding this comment.
Overall, the function and the test file are really well done, and the code is easy to follow because of the clear docstrings and explanations you’ve included. There are a few things that could be improved to make it even better, though. For example, adding a bit more about the strategy in the function docstring would help others understand how the function works before diving into the code. Also, including some doctests would show examples of how the function behaves, which could be helpful.
In the test file, it might be a good idea to add some tests for negative numbers and larger ranges to cover more cases. Also, I noticed that two tests check for the same thing (when start is greater than end), so maybe you can remove one to make things clearer and less redundant. I’d suggest keeping the first test, as it explains the situation a bit better.
Once these changes are made, the code will be even more polished, and it will be easier for anyone else to understand what’s going on. Keep up the great work!
solutions/even_odd.py
Outdated
|
|
||
| Returns: | ||
| A tuple of two lists: the first list contains even numbers, the second contains odd numbers. | ||
| """ |
There was a problem hiding this comment.
The function docstring is well written and shows clearly how the function is used but it can be improved even more. Consider adding the strategy to help others better understand how the function works before even going into the details of the code.
Also, make sure to add doctests to the function docstring to provide examples on the output of the function
| @@ -0,0 +1,44 @@ | |||
| import unittest | |||
There was a problem hiding this comment.
Your test file is also well written, and I liked how each function has a descriptive docstring and that me it easier for my to understand the purpose of each test. Overall, the quality is high and it can be even better with some changes.
- Add a module docstring to describe the purpose of the tests, what is being tested and what types of test cases are included in the file. This will make the test file even more easy to understand.
- Consider adding more tests for large ranges
- Consider adding a test for negative numbers to ensure your cover most of the cases
- I also noticed that two tests serve the same purpose:
def test_even_odd_range_invalid_order(self):
"""
Tests that ValueError is raised if start is greater than end.
"""
with self.assertRaises(ValueError):
even_odd_range(5, 3)def test_even_odd_range_empty(self):
"""
Tests that the function raises an error for an empty range where start > end.
"""
with self.assertRaises(ValueError):
even_odd_range(5, 4)They both check when the start is greater than the end and raise a ValueError and this seems redundant, and you can consider dropping one of the tests. I suggest keeping the first function since it describes the scenario better.
There was a problem hiding this comment.
Thank you so much, Abdo, for your detailed feedback and constructive suggestions!
I really appreciate you pointing out the strengths in the code and offering ways to improve it further. Here’s how I plan to address your comments:
1. Function Docstring and Doctests: I’ll update the function docstring to include more details about the strategy, making it easier for others to understand how it works at a glance. I’ll also add doctests to show examples of the function’s behavior.
2. Test File Improvements:
• I’ll add a module docstring to explain the purpose of the test file and summarize the types of cases it covers.
• I’ll include tests for larger ranges and negative numbers to make the test cases more comprehensive.
• Regarding the redundant tests, I’ll remove the test_even_odd_range_empty test as you suggested and keep the test_even_odd_range_invalid_order since it’s clearer.
Once I make these changes, I hope the code will be more polished and easier to understand. Thanks again for taking the time to review and for your helpful insights—it really means a lot! 😃👌.
|
The code and the test are really good, well done Rouaa! |
|
@MuhannadGTR thank you for your review , I appreciate it 🤩 |
Rouaa93
force-pushed
the
challenge/1-even_odd_range-Rouaa
branch
from
17ab504 to
a103249
Compare
|
@Rouaa93 |
|
@Rouaa93, after getting permission from you, I deleted the extra files and kept only two: one in the solutions folder and another in the tests folder. Now, we have reduced the CI check errors from 8 to 4. Ci check Errors:
|
|
@Rouaa93 1. Issue with the Test Expectation: The failure occurs because the test expects the even_odd_range function to return even numbers in the order [0, -4], while the current implementation returns them in ascending order [-4, -2, 0]. This is because Python's range() function generates numbers in ascending order by default. Solution: Update the test case as follows: def test_even_odd_range_negative_numbers(self): 2. Issue with Relative Import: The error occurs because the relative import (from ..even_odd import even_odd_range) fails when the script is run as a standalone file, outside of the package context. Python treats test_even_odd.py as a top-level script, making the relative import invalid. Solution: Modify the import logic to handle both package execution and standalone execution: try: This way, the import will work in both contexts. |
|
Hi Evan, I did some research and found that the parent repository might hold a solution to this issue. However, I’m unsure how to access .gitmodules to remove it directly on GitHub. Additionally, I searched for the d44145 commit in the history but couldn’t locate it. Could you please assist me with this? |
|
Hi Aseel, @Aseel-AbuKmail Regarding the py_tests issue, your proposed solution for adjusting the test expectations makes sense. I'll review and test the changes to ensure compatibility. For the relative import issue, the solution with the try-except block to handle both execution contexts is a practical approach. I'll implement and verify it. Thanks again for your collaboration—we’re getting closer to resolving everything! 😊 |
|
To resolve the Is_linting error: After conducting an online search and consulting with peers in the program on Slack, we believe the issue may be related to the ET6-foundations-group-17 submodule that was created in your branch for this challenge. I kindly ask for your permission to delete this submodule in order to attempt fixing the error, or feel free to delete it yourself. Thank you!
|
You're welcome! Just pray for us and for an end to the war in Gaza. |
|
@Aseel-AbuKmail . However, before proceeding, I would like to understand the impact of deleting the submodule on the current challenge and the overall project structure. Could you clarify if this will affect other parts of the repository or any dependencies?” “If deleting the submodule is safe and won’t disrupt other work, I agree to proceed with your suggestion. Please keep me updated on the steps you’re taking and any changes made.” |
Of course, you and your family are in my prayers. May there be peace and an end to the suffering in Gaza soon. And may your parents always be blessed for their love and efforts—you’re truly a reflection of their hard work. Stay strong, and may goodness follow you and your loved ones always. ❤️ |
Thank you for your thoughtful consideration. I believe it won’t affect other parts of the repository, as the submodule is only present in this branch and not in others. So, any changes we make in this branch should not impact the repository overall—this is my assumption, as I’m still a beginner with GitHub! |
|
@Rouaa93, I believe your pull request is incorrect. Please save these files on your local machine, delete this pull request, and then create a new one. |
name: solution review for even odd challenge
about: A template PR for code review with a checklist
Behavior
Files
/tests/test_file_name.pyUnit Tests
Function Docstring
Raises:The Function
Strategy
Do's
Don'ts
Implementation
when it's too restricting.
printstatements anywhere