-
Notifications
You must be signed in to change notification settings - Fork 6
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
Valid Parentheses - Merge pull request #543 from iKostanOrg/master #544
Conversation
Merge from master
Reviewer's Guide by SourceryThis pull request adds a new kata solution for "Valid Parentheses" problem and updates the README.md file to include it in the list of completed katas. The implementation includes a solution file and comprehensive test cases to verify the functionality. Class diagram for Valid Parentheses solutionclassDiagram
class ValidParenthesesTestCase {
+test_valid_parentheses_true()
+test_valid_parentheses_false()
+test_valid_parentheses_empty_string()
+test_valid_parentheses_large_valid()
+test_valid_parentheses_large_invalid()
}
class valid_parentheses {
+valid_parentheses(paren_str: str) bool
}
class update_str {
+update_str(paren_str: str, i: int) str
}
ValidParenthesesTestCase --> valid_parentheses
valid_parentheses --> update_str
note for ValidParenthesesTestCase "Unit tests for the valid_parentheses function"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
We have skipped reviewing this pull request. Here's why:
- We don't review packaging changes - Let us know if you'd like us to change this.
- All of the files are larger than we can process. We're working on it!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #544 +/- ##
=======================================
Coverage 90.01% 90.01%
=======================================
Files 171 171
Lines 2574 2574
=======================================
Hits 2317 2317
Misses 257 257 ☔ View full report in Codecov by Sentry. |
Code Climate has analyzed commit d87e55d and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 90.0% (0.0% change). View more on Code Climate. |
Missing module docstring
Trailing newlines
Wrong continued indentation (add 1 space).
@sourcery-ai review |
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.
Hey @ikostan - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
""" | ||
|
||
|
||
def valid_parentheses(paren_str: str) -> bool: |
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.
issue (complexity): Consider using a counter-based approach instead of string manipulation to validate parentheses
The current implementation unnecessarily rebuilds strings and uses nested loops, leading to O(n²) complexity. This can be simplified to O(n) using a counter approach that better expresses the intent:
def valid_parentheses(paren_str: str) -> bool:
count = 0
for char in paren_str:
if char == '(':
count += 1
else:
count -= 1
if count < 0: # Too many closing brackets
return False
return count == 0
This solution:
- Eliminates string rebuilding and nested loops
- Maintains the same functionality with better performance
- Is more readable as it directly expresses the logic of matching pairs
for test_str in test_data: | ||
with allure.step(f"Enter test string {test_data}" | ||
f"and verify that the function returns True."): | ||
result: bool = valid_parentheses(test_str) | ||
print_log(test_data=test_data, result=result) | ||
self.assertTrue(result) |
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.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests
)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
kyu_7/valid_parentheses/solution.py:53:19: R1736: Unnecessary list index lookup, use 'char' instead (unnecessary-list-index-lookup)
kyu_7/valid_parentheses/test_valid_parentheses.py:108:25: W1309: Using an f-string that does not have any interpolated variables (f-string-without-interpolation) kyu_7/valid_parentheses/test_valid_parentheses.py:130:26: W1309: Using an f-string that does not have any interpolated variables (f-string-without-interpolation) kyu_7/valid_parentheses/test_valid_parentheses.py:132:29: W1309: Using an f-string that does not have any interpolated variables (f-string-without-interpolation) kyu_7/valid_parentheses/test_valid_parentheses.py:154:26: W1309: Using an f-string that does not have any interpolated variables (f-string-without-interpolation) kyu_7/valid_parentheses/test_valid_parentheses.py:156:29: W1309: Using an f-string that does not have any interpolated variables (f-string-without-interpolation)
************* Module kyu_7.valid_parentheses.test_valid_parentheses kyu_7/valid_parentheses/test_valid_parentheses.py:130:26: W1309: Using an f-string that does not have any interpolated variables (f-string-without-interpolation) kyu_7/valid_parentheses/test_valid_parentheses.py:154:26: W1309: Using an f-string that does not have any interpolated variables (f-string-without-interpolation)
""" | ||
|
||
|
||
def valid_parentheses(paren_str: str) -> bool: |
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.
Function valid_parentheses
has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.
issue (bug_risk): Fix incorrect parameter in print_log call The print_log is using test_data instead of test_str which could cause confusing log output. Should be print_log(test_data=test_str, result=result)
Merge from master
Summary by Sourcery
Add a new solution for the 'Valid Parentheses' problem, including comprehensive unit tests and documentation.
New Features:
Documentation:
Tests: