aibolit/patterns/bidirect_index/bidirect_index.py (4)

42-42: Fix quote style for consistency.

The codebase prefers single quotes based on the pipeline failures.

-        with open(filename, encoding="utf-8") as f:
+        with open(filename, encoding='utf-8') as f:

---

59-62: Simplify nested if statements.

Combine the conditions to improve readability.

-            if re.search(r"\bvoid\b\s+\w+\s*\(.*\)\s*{", line):
-                if mstart is None:
-                    mstart = idx
-                    brace = 0
+            if re.search(r'\bvoid\b\s+\w+\s*\(.*\)\s*{', line) and mstart is None:
+                mstart = idx
+                brace = 0

---

63-64: Fix quote style for consistency.

-            brace += line.count("{")
-            brace -= line.count("}")
+            brace += line.count('{')
+            brace -= line.count('}')

---

99-100: Fix quote style for consistency.

-                    brace += lines[i].count("{")
-                    brace -= lines[i].count("}")
+                    brace += lines[i].count('{')
+                    brace -= lines[i].count('}')

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

aibolit/patterns/bidirect_index/bidirect_index.py (2)

116-119: Replace variable name 'line_' with a more descriptive name.

For consistency and clarity, use a more descriptive variable name.

Apply this diff:

         k = start
         while k < end:
-            line_ = lines[k]
-            for_decl = re.match(r'\s*for\s*\(\s*int\s+' + re.escape(var) + r'\s*=', line_)
+            current_line = lines[k]
+            for_decl = re.match(r'\s*for\s*\(\s*int\s+' + re.escape(var) + r'\s*=', current_line)
             if for_decl:
-                brace = line_.count('{') - line_.count('}')
+                brace = current_line.count('{') - current_line.count('}')

---

141-151: Replace variable name 'line_' with a more descriptive name.

For consistency with the rest of the codebase, use a more descriptive variable name.

Apply this diff:

             if not in_for:
-                line_ = lines[k]
+                current_line = lines[k]
                 if re.search(
                         r'(\+\+' + re.escape(var) + r'|' + re.escape(var) + r'\+\+|' +
                         re.escape(var) + r'\s*\+=\s*1\b|' +
-                        re.escape(var) + r'\s*=\s*' + re.escape(var) + r'\s*\+\s*1\b)', line_):
+                        re.escape(var) + r'\s*=\s*' + re.escape(var) + r'\s*\+\s*1\b)', current_line):
                     inc_outside += 1
                 if re.search(
                         r'(--' + re.escape(var) + r'|' + re.escape(var) + r'--|' +
                         re.escape(var) + r'\s*-=\s*1\b|' +
-                        re.escape(var) + r'\s*=\s*' + re.escape(var) + r'\s*-\s*1\b)', line_):
+                        re.escape(var) + r'\s*=\s*' + re.escape(var) + r'\s*-\s*1\b)', current_line):
                     dec_outside += 1

aibolit/patterns/bidirect_index/bidirect_index.py (2)

59-62: Simplify nested if statements.

The nested if statements can be combined into a single condition for better readability.

Apply this diff to simplify:

-            if re.search(r'\bvoid\b\s+\w+\s*\(.*\)\s*{', line):
-                if mstart is None:
-                    mstart = idx
-                    brace = 0
+            if re.search(r'\bvoid\b\s+\w+\s*\(.*\)\s*{', line) and mstart is None:
+                mstart = idx
+                brace = 0

---

86-90: Use consistent variable naming.

The variable name line_ is used here while line is used elsewhere. Consider using a more descriptive name like next_line or current_line for clarity and consistency.

Apply this diff:

                 j = i + 1
                 while j <= end:
-                    line_ = lines[j]
-                    if re.match(r'\s*(int|long|byte|short)\s+' + re.escape(var) + r'\s*=', line_):
+                    next_line = lines[j]
+                    if re.match(r'\s*(int|long|byte|short)\s+' + re.escape(var) + r'\s*=', next_line):
                         break
                     j += 1

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: SinbadTheSailor2005
PR: cqfn/aibolit#816
File: aibolit/patterns/loop_outsider/loop_outsider.py:0-0
Timestamp: 2025-06-08T11:50:22.673Z
Learning: In the aibolit codebase, ASTNodeType.ASSIGNMENT nodes have an attribute called `expressionl` (with 'l' suffix), not `expression`. This is confirmed in `aibolit/ast_framework/_auxiliary_data.py` and used consistently throughout existing patterns like mutable_index and many_primary_ctors.

aibolit/patterns/bidirect_index/bidirect_index.py (2)

42-46: Improve exception chaining for better error context.

The error handling is good, but consider using exception chaining to preserve the original exception context.

-        except (FileNotFoundError, IOError) as e:
-            raise ValueError(f'Failed to read file {filename}: {e}')
+        except (FileNotFoundError, IOError) as e:
+            raise ValueError(f'Failed to read file {filename}: {e}') from e

---

68-73: Simplify nested if statements for better readability.

The regex pattern effectively captures Java method signatures with various modifiers and return types. However, the nested if statement can be simplified.

-            if (re.search(
-                    r'(public|private|protected|static|\s)*([\w<>\[\]]+)\s+\w+\s*\([^)]*\)\s*{',
-                    line)):
-                if mstart is None:
-                    mstart = idx
-                    brace = 0
+            if (re.search(
+                    r'(public|private|protected|static|\s)*([\w<>\[\]]+)\s+\w+\s*\([^)]*\)\s*{',
+                    line) and mstart is None):
+                mstart = idx
+                brace = 0

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: SinbadTheSailor2005
PR: cqfn/aibolit#816
File: aibolit/patterns/loop_outsider/loop_outsider.py:0-0
Timestamp: 2025-06-08T11:50:22.673Z
Learning: In the aibolit codebase, ASTNodeType.ASSIGNMENT nodes have an attribute called `expressionl` (with 'l' suffix), not `expression`. This is confirmed in `aibolit/ast_framework/_auxiliary_data.py` and used consistently throughout existing patterns like mutable_index and many_primary_ctors.

aibolit/patterns/bidirect_index/bidirect_index.py (1)

13-14: Fix indentation to match the parent item.

The indentation is inconsistent with the parent bullet point.

-        - increments/decrements inside a for-loop with a local variable of the same name
-    are ignored (to avoid "fake" cases).
+        - increments/decrements inside a for-loop with a local variable of the same name
+          are ignored (to avoid "fake" cases).

aibolit/patterns/bidirect_index/bidirect_index.py (4)

61-66: Simplify nested if statements for better readability.

The nested if statement can be combined into a single condition as suggested by the static analysis tool.

-            if (re.search(
-                    r'(public|private|protected|static|\s)*([\w<>\[\]]+)\s+\w+\s*\([^)]*\)\s*{',
-                    line)):
-                if mstart is None:
-                    mstart = idx
-                    brace = 0
+            if (re.search(
+                    r'(public|private|protected|static|\s)*([\w<>\[\]]+)\s+\w+\s*\([^)]*\)\s*{',
+                    line) and mstart is None):
+                mstart = idx
+                brace = 0

---

82-86: Consider extracting variable declaration detection logic.

The variable declaration detection logic is complex and could benefit from extraction into a separate method for better readability and testability.

+    @staticmethod
+    def _extract_declared_variable(line):
+        """Extract variable name from declaration line if present."""
+        typed_decl = re.match(r'\s*(int|long|byte|short)\s+(\w+)\s*=', line)
+        if typed_decl:
+            return typed_decl.group(2)
+        
+        untyped_decl = re.match(r'^\s*(\w+)\s*=\s*[^;]+;', line)
+        if untyped_decl:
+            return untyped_decl.group(1)
+        
+        return None

Then use it in the main method:

-            typed_decl = re.match(r'\s*(int|long|byte|short)\s+(\w+)\s*=', line)
-            untyped_decl = re.match(r'^\s*(\w+)\s*=\s*[^;]+;', line) \
-                if not typed_decl else None
-            var = typed_decl.group(2) if typed_decl else (untyped_decl.group(1)
-                                                          if untyped_decl else None)
+            var = BidirectIndex._extract_declared_variable(line)

---

146-155: Consider using verbose regex for better readability.

The complex regex patterns for increment/decrement detection are hard to read and maintain. Consider using the re.VERBOSE flag as suggested in past comments.

+    @staticmethod
+    def _create_increment_pattern(var):
+        """Create verbose regex pattern for increment detection."""
+        return re.compile(rf"""
+            (?:
+                \+\+{re.escape(var)}          |  # Pre-increment: ++var
+                {re.escape(var)}\+\+          |  # Post-increment: var++
+                {re.escape(var)}\s*\+=\s*1\b  |  # Addition assignment: var += 1
+                {re.escape(var)}\s*=\s*{re.escape(var)}\s*\+\s*1\b  # Assignment: var = var + 1
+            )
+        """, re.VERBOSE)
+    
+    @staticmethod
+    def _create_decrement_pattern(var):
+        """Create verbose regex pattern for decrement detection."""
+        return re.compile(rf"""
+            (?:
+                --{re.escape(var)}            |  # Pre-decrement: --var
+                {re.escape(var)}--            |  # Post-decrement: var--
+                {re.escape(var)}\s*-=\s*1\b   |  # Subtraction assignment: var -= 1
+                {re.escape(var)}\s*=\s*{re.escape(var)}\s*-\s*1\b  # Assignment: var = var - 1
+            )
+        """, re.VERBOSE)

---

143-156: Extract range checking logic for better readability.

The range checking logic for determining if a line is within a for-block could be extracted for clarity.

+    @staticmethod
+    def _is_in_for_block(line_idx, for_blocks):
+        """Check if line index is within any for-block range."""
+        return any(bstart <= line_idx <= bend for bstart, bend in for_blocks)

Then use it in the main method:

-            in_for = any(bstart <= k <= bend for (bstart, bend) in for_blocks)
+            in_for = BidirectIndex._is_in_for_block(k, for_blocks)

test/patterns/bidirect_index/test_bidirect_index.py (1)

11-13: Consider adding error handling to the helper method.

The _lines_from_file method should handle potential file reading errors gracefully to provide better test failure messages.

     def _lines_from_file(self, filename):
-        with open(filename, encoding='utf-8') as f:
-            return f.readlines()
+        try:
+            with open(filename, encoding='utf-8') as f:
+                return f.readlines()
+        except FileNotFoundError:
+            self.fail(f"Test file not found: {filename}")
+        except IOError as e:
+            self.fail(f"Error reading test file {filename}: {e}")

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: SinbadTheSailor2005
PR: cqfn/aibolit#816
File: aibolit/patterns/loop_outsider/loop_outsider.py:0-0
Timestamp: 2025-06-08T11:50:22.673Z
Learning: In the aibolit codebase, ASTNodeType.ASSIGNMENT nodes have an attribute called `expressionl` (with 'l' suffix), not `expression`. This is confirmed in `aibolit/ast_framework/_auxiliary_data.py` and used consistently throughout existing patterns like mutable_index and many_primary_ctors.

aibolit/patterns/bidirect_index/bidirect_index.py (4)

41-43: Use re.VERBOSE flag for complex regex patterns to improve readability.

The regex patterns are difficult to understand. Consider using the re.VERBOSE flag with comments:

typed_decl = re.match(
    r'''
    \s*                     # Optional whitespace
    (int|long|byte|short)   # Java primitive types
    \s+                     # Required whitespace
    (\w+)                   # Variable name (captured)
    \s*                     # Optional whitespace
    =                       # Assignment operator
    ''', 
    line, 
    re.VERBOSE
)

---

127-127: Fix indentation to match the list item structure.

-        - increments/decrements inside a for-loop with a local variable of the same name
-    are ignored (to avoid "fake" cases).
+        - increments/decrements inside a for-loop with a local variable of the same name
+          are ignored (to avoid "fake" cases).

---

143-157: Consider following the unified Pattern interface using AST.

Based on the PR objectives and past comments, the Pattern interface should accept an AST object rather than source lines. The current implementation using source lines goes against the unified interface direction.

Consider refactoring to:

def value(self, ast: AST) -> list[LineNumber]:
    """Analyze the AST and return line numbers with bidirectional indices."""
    # Implementation using AST traversal

This would align with the codebase's direction towards AST-based analysis rather than text-based parsing.

---

105-114: Refactor complex regex patterns using re.VERBOSE for better readability.

These regex patterns are extremely difficult to parse. Use re.VERBOSE with comments:

inc_pattern = re.compile(
    rf'''
    (
        \+\+{re.escape(var)}              # Pre-increment: ++var
        |{re.escape(var)}\+\+             # Post-increment: var++
        |{re.escape(var)}\s*\+=\s*1\b     # Compound assignment: var += 1
        |{re.escape(var)}\s*=\s*          # Assignment: var = var + 1
         {re.escape(var)}\s*\+\s*1\b
    )
    ''',
    re.VERBOSE
)
if inc_pattern.search(line_):
    inc_outside += 1

aibolit/patterns/bidirect_index/bidirect_index.py (1)

6-31: Consider making this a private class method or keeping functions consistently outside the class.

The function is defined outside the class but used inside. For consistency, either make all helper functions class methods (prefixed with _ for internal use) or keep them all as standalone functions.

-def find_methods(lines):
+class BidirectIndex:
+    # ... existing code ...
+    
+    def _find_methods(self, lines):

Also update the call in line 155:

-        for m_start, m_end in find_methods(lines):
+        for m_start, m_end in self._find_methods(lines):

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: SinbadTheSailor2005
PR: cqfn/aibolit#816
File: aibolit/patterns/loop_outsider/loop_outsider.py:0-0
Timestamp: 2025-06-08T11:50:22.673Z
Learning: In the aibolit codebase, ASTNodeType.ASSIGNMENT nodes have an attribute called `expressionl` (with 'l' suffix), not `expression`. This is confirmed in `aibolit/ast_framework/_auxiliary_data.py` and used consistently throughout existing patterns like mutable_index and many_primary_ctors.

aibolit/patterns/bidirect_index/bidirect_index.py (6)

9-34: Convert standalone function to instance method.

Based on past review feedback, standalone functions should be converted to instance methods of the BidirectIndex class for better encapsulation and consistency with the codebase design patterns.

-def find_methods(lines):
+class BidirectIndex:
+    def _find_methods(self, lines):

---

23-25: Improve regex readability using re.VERBOSE flag.

The complex regex pattern is difficult to read and maintain. Based on past review feedback, use the re.VERBOSE flag to make it more readable with comments and proper formatting.

-        if (re.search(
-                r'(public|private|protected|static|\s)*([\w<>\[\]]+)\s+\w+\s*\([^)]*\)\s*{',
-                line)):
+        method_pattern = re.compile(r'''
+            (public|private|protected|static|\s)*  # Access modifiers
+            ([\w<>\[\]]+)                          # Return type
+            \s+\w+                                 # Method name
+            \s*\([^)]*\)                          # Parameters
+            \s*{                                   # Opening brace
+        ''', re.VERBOSE)
+        if method_pattern.search(line):

---

37-73: Convert to instance method and improve structure.

This function should be an instance method for consistency with the class design pattern. Also consider breaking down the complex logic into smaller, more focused methods.

-def analyze_block(lines, start, end, result):
+    def _analyze_block(self, lines, start, end, result):

---

75-95: Convert to instance method.

This function should be an instance method for consistency with the class design pattern.

-def find_for_blocks(lines, var, start, end):
+    def _find_for_blocks(self, lines, var, start, end):

---

97-120: Convert to instance method and improve regex readability.

This function should be an instance method, and the complex regex patterns should use the re.VERBOSE flag for better readability.

-def count_inc_dec(lines, var, for_blocks, start, end):
+    def _count_inc_dec(self, lines, var, for_blocks, start, end):

For the regex patterns, consider using re.VERBOSE:

-            if re.search(
-                    r'(\+\+' + re.escape(var) + r'|' + re.escape(var) + r'\+\+|' +
-                    re.escape(var) + r'\s*\+=\s*1\b|' +
-                    re.escape(var) + r'\s*=\s*' + re.escape(var) + r'\s*\+\s*1\b)', line_):
+            increment_pattern = re.compile(rf'''
+                (\+\+{re.escape(var)}          |    # Pre-increment
+                 {re.escape(var)}\+\+          |    # Post-increment
+                 {re.escape(var)}\s*\+=\s*1\b  |    # += 1
+                 {re.escape(var)}\s*=\s*{re.escape(var)}\s*\+\s*1\b)  # = var + 1
+            ''', re.VERBOSE)
+            if increment_pattern.search(line_):

---

139-157: Address interface consistency and add error handling.

Based on past review feedback, the interface should be updated to match the established pattern in the codebase. Also, file operations need proper error handling.

-    def value(self, filename: str | os.PathLike) -> list[LineNumber]:
+    def value(self, lines: list[str]) -> list[LineNumber]:
         """
-        Analyze the given Java file and return a sorted list of line numbers where a variable
+        Analyze the given Java source lines and return a sorted list of line numbers where a variable
         is used as a bidirectional index.

         Args:
-            filename (str | os.PathLike): Path to the Java source file.
+            lines (list[str]): Lines of Java source code to analyze.

         Returns:
             list[LineNumber]: Sorted list of line numbers where bidirectional indices are found.
         """
-
-        with open(filename, encoding='utf-8') as f:
-            lines = f.readlines()
         result: list[int] = []
-        for m_start, m_end in find_methods(lines):
-            analyze_block(lines, m_start + 1, m_end, result)
+        for m_start, m_end in self._find_methods(lines):
+            self._analyze_block(lines, m_start + 1, m_end, result)
         return [LineNumber(n) for n in sorted(result)]

aibolit/patterns/bidirect_index/bidirect_index.py (1)

44-48: Simplify variable declaration detection logic.

The current logic for detecting typed and untyped declarations is complex and could be simplified for better readability.

-        typed_decl = re.match(r'\s*(int|long|byte|short)\s+(\w+)\s*=', line)
-        untyped_decl = re.match(r'^\s*(\w+)\s*=\s*[^;]+;', line) \
-            if not typed_decl else None
-        var = typed_decl.group(2) if typed_decl else (untyped_decl.group(1)
-                                                      if untyped_decl else None)
+        var = None
+        typed_decl = re.match(r'\s*(int|long|byte|short)\s+(\w+)\s*=', line)
+        if typed_decl:
+            var = typed_decl.group(2)
+        else:
+            untyped_decl = re.match(r'^\s*(\w+)\s*=\s*[^;]+;', line)
+            if untyped_decl:
+                var = untyped_decl.group(1)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: SinbadTheSailor2005
PR: cqfn/aibolit#816
File: aibolit/patterns/loop_outsider/loop_outsider.py:0-0
Timestamp: 2025-06-08T11:50:22.673Z
Learning: In the aibolit codebase, ASTNodeType.ASSIGNMENT nodes have an attribute called `expressionl` (with 'l' suffix), not `expression`. This is confirmed in `aibolit/ast_framework/_auxiliary_data.py` and used consistently throughout existing patterns like mutable_index and many_primary_ctors.

aibolit/patterns/bidirect_index/bidirect_index.py (4)

16-16: Fix indentation to match the list format.

The continuation line should be properly indented to align with the list items above.

-      - increments/decrements inside a for-loop with a local variable of the same name are ignored.
+      - increments/decrements inside a for-loop with a local variable of the same name
+        are ignored (to avoid "fake" cases).

---

135-151: Consider refactoring to use AST interface for consistency.

As mentioned in previous reviews, this implementation uses filename as input while the unified Pattern interface expects AST. This goes against the project's direction for interface unification.

Consider creating a puzzle to track the refactoring of this implementation to use ast_framework.AST rather than filename as suggested in the past review comments.

---

122-131: Use re.VERBOSE flag to improve regex readability.

These complex regex patterns are difficult to read and maintain. Consider using the re.VERBOSE flag with comments as previously suggested.

# Define patterns with re.VERBOSE for better readability
INCREMENT_PATTERN = re.compile(rf'''
    \+\+{re.escape(var)}           |  # prefix increment
    {re.escape(var)}\+\+           |  # postfix increment  
    {re.escape(var)}\s*\+=\s*1\b   |  # compound assignment
    {re.escape(var)}\s*=\s*{re.escape(var)}\s*\+\s*1\b  # assignment
''', re.VERBOSE)

DECREMENT_PATTERN = re.compile(rf'''
    --{re.escape(var)}             |  # prefix decrement
    {re.escape(var)}--             |  # postfix decrement
    {re.escape(var)}\s*-=\s*1\b    |  # compound assignment
    {re.escape(var)}\s*=\s*{re.escape(var)}\s*-\s*1\b  # assignment
''', re.VERBOSE)

# Then use:
if INCREMENT_PATTERN.search(line_):
    inc_outside += 1
if DECREMENT_PATTERN.search(line_):
    dec_outside += 1

---

146-147: Add error handling for file operations.

The file operations should handle potential errors gracefully as previously suggested.

-        with open(filename, encoding='utf-8') as f:
-            lines = f.readlines()
+        try:
+            with open(filename, encoding='utf-8') as f:
+                lines = f.readlines()
+        except (FileNotFoundError, IOError) as e:
+            raise ValueError(f"Failed to read file {filename}: {e}")

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: SinbadTheSailor2005
PR: cqfn/aibolit#816
File: aibolit/patterns/loop_outsider/loop_outsider.py:0-0
Timestamp: 2025-06-08T11:50:22.673Z
Learning: In the aibolit codebase, ASTNodeType.ASSIGNMENT nodes have an attribute called `expressionl` (with 'l' suffix), not `expression`. This is confirmed in `aibolit/ast_framework/_auxiliary_data.py` and used consistently throughout existing patterns like mutable_index and many_primary_ctors.

aibolit/patterns/bidirect_index/bidirect_index.py (1)

149-150: Add error handling for file operations.

The method should handle potential file reading errors gracefully.

-        with open(filename, encoding='utf-8') as f:
-            lines = f.readlines()
+        try:
+            with open(filename, encoding='utf-8') as f:
+                lines = f.readlines()
+        except (FileNotFoundError, IOError) as e:
+            raise ValueError(f"Failed to read file {filename}: {e}")

aibolit/patterns/bidirect_index/bidirect_index.py (2)

40-43: Consider handling methods with opening braces on separate lines.

The current regex assumes the opening brace { is on the same line as the method signature. In Java, it's valid to have:

public void myMethod()
{
    // code
}

Consider updating the logic to handle this case:

-            if (re.search(
-                    r'(public|private|protected|static|\s)*([\w<>\[\]]+)\s+\w+\s*\([^)]*\)\s*{',
-                    line)):
+            # First check for method signature
+            method_sig = re.search(
+                r'(public|private|protected|static|\s)*([\w<>\[\]]+)\s+\w+\s*\([^)]*\)\s*$',
+                line)
+            # Then check if it has brace on same line or next line
+            if method_sig or (re.search(
+                    r'(public|private|protected|static|\s)*([\w<>\[\]]+)\s+\w+\s*\([^)]*\)\s*{',
+                    line)):

---

125-134: Consider improving regex readability.

The increment/decrement detection regexes are comprehensive but difficult to read.

Consider using the re.VERBOSE flag for better readability:

inc_pattern = re.compile(rf'''
    \+\+{re.escape(var)}       |  # pre-increment
    {re.escape(var)}\+\+       |  # post-increment
    {re.escape(var)}\s*\+=\s*1\b |  # compound assignment
    {re.escape(var)}\s*=\s*{re.escape(var)}\s*\+\s*1\b  # reassignment
''', re.VERBOSE)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: SinbadTheSailor2005
PR: cqfn/aibolit#816
File: aibolit/patterns/loop_outsider/loop_outsider.py:0-0
Timestamp: 2025-06-08T11:50:22.673Z
Learning: In the aibolit codebase, ASTNodeType.ASSIGNMENT nodes have an attribute called `expressionl` (with 'l' suffix), not `expression`. This is confirmed in `aibolit/ast_framework/_auxiliary_data.py` and used consistently throughout existing patterns like mutable_index and many_primary_ctors.

aibolit/patterns/bidirect_index/bidirect_index.py (4)

16-16: Fix docstring indentation for consistency.

The list item indentation doesn't match the other items.

-      - increments/decrements inside a for-loop with a local variable of the same name are ignored.
+        - increments/decrements inside a for-loop with a local variable of the same name
+          are ignored (to avoid "fake" cases).

---

23-39: Consider migrating to AST-based interface.

Per previous discussions, the preferred interface should use ast_framework.AST rather than filename as input parameter to align with the unified Pattern interface direction.

-    def value(self, filename: str | os.PathLike) -> list[LineNumber]:
+    def value(self, ast: AST) -> list[LineNumber]:

Additionally, add error handling for file operations:

-        with open(filename, encoding='utf-8') as f:
-            lines = f.readlines()
+        try:
+            with open(filename, encoding='utf-8') as f:
+                lines = f.readlines()
+        except (FileNotFoundError, IOError) as e:
+            raise ValueError(f"Failed to read file {filename}: {e}")

---

87-90: Improve untyped variable detection to avoid false positives.

The current regex for untyped declarations could match invalid patterns like method calls. Consider a more restrictive pattern:

-            untyped_decl = re.match(
-                r'^\s*(\w+)\s*=\s*[^=;]+;', line) if not typed_decl else None
+            # Only match simple reassignments, not method calls or field access
+            untyped_decl = re.match(
+                r'^\s*(\w+)\s*=\s*[^=;]+;', line) if not typed_decl else None

---

95-99: Include untyped reassignments when finding variable scope.

The current implementation only checks for typed redeclarations, missing cases where variables are reassigned without type:

-                    # Check for re-declaration of variable with type
-                    if re.match(
-                            r'\s*(int|long|byte|short)\s+' + re.escape(var) + r'\s*=', line_):
+                    # Check for both typed declarations and simple reassignments
+                    if (re.match(
+                            r'\s*(int|long|byte|short)\s+' + re.escape(var) + r'\s*=', line_) or
+                        re.match(r'^\s*' + re.escape(var) + r'\s*=\s*[^=;]+;', line_)):

aibolit/patterns/bidirect_index/bidirect_index.py (2)

41-74: Well-structured method detection with readable regex.

Good use of re.VERBOSE for regex readability. Minor typo in comment:

-            ([\w<>\[\]]+)                          # # return type
+            ([\w<>\[\]]+)                          # return type

---

1-175: Consider future refactoring to AST-based implementation.

While this regex-based implementation is functional and covers all test cases, it would benefit from being refactored to use the ast_framework.AST for better maintainability and consistency with other patterns in the codebase. This could be tracked as a technical debt item.

Would you like me to create an issue to track the refactoring from regex-based to AST-based implementation?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro