Add code complexity assessor for AI understandability metrics#4
Conversation
…dependency analysis Co-authored-by: webreidi <55603905+webreidi@users.noreply.github.com>
…calculation Co-authored-by: webreidi <55603905+webreidi@users.noreply.github.com>
- Detects when multiple instruction files exist (e.g., both .github/copilot-instructions.md and .copilot-instructions.md) - Flags conflicts as a weakness with clear recommendation - Applies score penalty (2 points) when conflicts are detected - Adds heuristic check for contradicting content between files - Addresses feedback about conflicting instructions causing Copilot issues
There was a problem hiding this comment.
Pull request overview
This PR adds a new CodeComplexityAssessor to measure code complexity metrics that impact AI's ability to understand codebases. The addition addresses a gap where repositories with good documentation could score well despite having complex, tightly-coupled code that confounds AI assistants. The PR also enhances the CustomInstructionsAssessor to detect and warn about conflicting instruction files.
Changes:
- Adds new
CodeComplexityAssessor(25 points) measuring cyclomatic complexity, file coupling, circular dependencies, and dependency depth - Updates max base score from 150 to 175 points across configuration, scoring, and documentation
- Enhances
CustomInstructionsAssessorto detect multiple/conflicting instruction files with a 2-point penalty - Updates all assessor progress indicators from "[X/10]" to "[X/11]" format
- Bumps tool version from 3.0 to 3.1
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| Assessors/CodeComplexityAssessor.cs | New assessor analyzing cyclomatic complexity, file coupling, circular dependencies, and dependency depth using regex-based parsing |
| Configuration/AssessmentConfig.cs | Adds CodeComplexity to Scores and Findings dictionaries, updates max score comment to 175 |
| Services/ReportGenerator.cs | Updates version to 3.1, adds CodeComplexity to max scores (25 pts), adds DisplayCodeComplexityMetrics method, updates max score comment |
| Program.cs | Adds CodeComplexityAssessor to assessors array, updates version display and usage text to reflect 175 max score |
| Assessors/CustomInstructionsAssessor.cs | Adds conflict detection for multiple instruction files with CheckForContradictions method, applies 2-point penalty for conflicts |
| Assessors/*.cs (8 files) | Updates progress indicators from [X/10] to [X/11] to reflect addition of new assessor |
Comments suppressed due to low confidence (6)
Assessors/CodeComplexityAssessor.cs:365
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var node in graph.Keys)
{
if (!visited.Contains(node))
{
var path = new List<string>();
FindCycles(node, graph, visited, recStack, path, cycles);
}
}
Assessors/CodeComplexityAssessor.cs:456
- This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
foreach (var neighbor in graph[node])
{
int depth = CalculateMaxDepth(neighbor, graph, visited);
maxDepth = Math.Max(maxDepth, depth);
}
Assessors/CodeComplexityAssessor.cs:89
- Poor error handling: empty catch block.
catch { }
Assessors/CodeComplexityAssessor.cs:205
- Poor error handling: empty catch block.
catch { }
Assessors/CodeComplexityAssessor.cs:318
- Poor error handling: empty catch block.
catch { }
Assessors/CodeComplexityAssessor.cs:446
- Inefficient use of 'ContainsKey' and indexer.
if (!graph.ContainsKey(node) || visited.Contains(node))
| ".js" or ".ts" or ".jsx" or ".tsx" => new[] { @"^import\s+.*from\s+['""]", @"require\s*\(['""]" }, | ||
| ".py" => new[] { @"^import\s+[\w.]+", @"^from\s+[\w.]+\s+import" }, | ||
| ".java" => new[] { @"^import\s+[\w.]+;" }, | ||
| ".go" => new[] { @"^import\s+\(", @"^import\s+""" }, |
There was a problem hiding this comment.
The Go import pattern "@"^import\s+("" only matches the opening parenthesis of multi-line import blocks but doesn't count the individual imports within the block. For example, "import (\n "fmt"\n "os"\n)" would count as 1 dependency instead of 2. This underestimates the file coupling for Go files with multi-line import blocks.
| // Extract method body (simplified brace matching) | ||
| int braceCount = 1; | ||
| int start = match.Index + match.Length; | ||
| int end = start; | ||
|
|
||
| for (int i = start; i < content.Length && braceCount > 0; i++) | ||
| { | ||
| if (content[i] == '{') braceCount++; | ||
| if (content[i] == '}') braceCount--; | ||
| end = i; | ||
| } | ||
|
|
||
| if (end > start && end < content.Length - 1) | ||
| { | ||
| methods.Add(content.Substring(match.Index, end - match.Index + 1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The brace counting logic for extracting method bodies doesn't handle string literals or comments containing braces, which can lead to incorrect method body extraction. For example, a string like "function() { return true; }" or a comment with braces would incorrectly affect the brace count. This could result in incomplete or incorrect method bodies being analyzed, leading to inaccurate cyclomatic complexity calculations.
| private int CountFileDependencies(string content, string extension) | ||
| { | ||
| int count = 0; | ||
|
|
||
| // Count import/using statements | ||
| var patterns = extension switch | ||
| { | ||
| ".cs" => new[] { @"^using\s+[\w.]+;", @"^using\s+static\s+[\w.]+;" }, | ||
| ".js" or ".ts" or ".jsx" or ".tsx" => new[] { @"^import\s+.*from\s+['""]", @"require\s*\(['""]" }, | ||
| ".py" => new[] { @"^import\s+[\w.]+", @"^from\s+[\w.]+\s+import" }, | ||
| ".java" => new[] { @"^import\s+[\w.]+;" }, | ||
| ".go" => new[] { @"^import\s+\(", @"^import\s+""" }, | ||
| _ => new[] { @"^import\s+", @"^#include\s*[<""]" } | ||
| }; | ||
|
|
||
| foreach (var pattern in patterns) | ||
| { | ||
| count += Regex.Matches(content, pattern, RegexOptions.Multiline).Count; | ||
| } | ||
|
|
||
| return count; | ||
| } |
There was a problem hiding this comment.
The import pattern for C# (@"using\s+([\w.]+);") will match all using statements including framework namespaces like System, System.IO, etc. This inflates the file coupling metric with external dependencies rather than measuring only internal project coupling. The coupling analysis should focus on internal dependencies within the codebase to accurately measure project complexity, not dependencies on standard libraries or third-party packages.
| Regex methodPattern = extension switch | ||
| { | ||
| ".cs" => new Regex(@"(public|private|protected|internal)\s+(?:static\s+)?(?:async\s+)?\w+(?:<[\w,\s]+>)?\s+\w+\s*\([^)]*\)\s*\{", RegexOptions.Multiline), | ||
| ".js" or ".ts" or ".jsx" or ".tsx" => new Regex(@"(function\s+\w+\s*\([^)]*\)\s*\{|(?:const|let|var)\s+\w+\s*=\s*(?:async\s*)?\([^)]*\)\s*=>\s*\{|\w+\s*\([^)]*\)\s*\{)", RegexOptions.Multiline), |
There was a problem hiding this comment.
The JavaScript/TypeScript method pattern includes a catch-all "\w+\s*([^)])\s{" that will match object methods and other structures that aren't actually function definitions. For example, it could match method calls like "someFunction() {" in certain formatting contexts. This pattern is too broad and may incorrectly identify non-method code as methods, leading to inaccurate method counts for complexity analysis.
| ".js" or ".ts" or ".jsx" or ".tsx" => new Regex(@"(function\s+\w+\s*\([^)]*\)\s*\{|(?:const|let|var)\s+\w+\s*=\s*(?:async\s*)?\([^)]*\)\s*=>\s*\{|\w+\s*\([^)]*\)\s*\{)", RegexOptions.Multiline), | |
| ".js" or ".ts" or ".jsx" or ".tsx" => new Regex(@"(function\s+\w+\s*\([^)]*\)\s*\{|(?:const|let|var)\s+\w+\s*=\s*(?:async\s*)?\([^)]*\)\s*=>\s*\{)", RegexOptions.Multiline), |
| // Count decision points: if, else if, while, for, foreach, case, catch, &&, ||, ? | ||
| var patterns = new[] | ||
| { | ||
| @"\bif\s*\(", | ||
| @"\belse\s+if\s*\(", |
There was a problem hiding this comment.
The pattern @"\belse\s+if\s*(" counts "else if" as a separate decision point, but this is already counted by the base complexity of 1 plus the "if" pattern. In cyclomatic complexity calculation, "else if" should not add an extra decision point beyond what the "if" already contributes. This double-counts certain control flow paths and inflates complexity scores.
| // Count decision points: if, else if, while, for, foreach, case, catch, &&, ||, ? | |
| var patterns = new[] | |
| { | |
| @"\bif\s*\(", | |
| @"\belse\s+if\s*\(", | |
| // Count decision points: if, while, for, foreach, case, catch, &&, ||, ? | |
| var patterns = new[] | |
| { | |
| @"\bif\s*\(", |
| foreach (Match match in matches) | ||
| { | ||
| if (match.Groups.Count > 1) | ||
| { | ||
| imports.Add(match.Groups[1].Value); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
This issue also appears on line 358 of the same file.
| foreach (var method in methods) | ||
| { | ||
| int complexity = CalculateCyclomaticComplexity(method); | ||
| complexityScores.Add(complexity); | ||
| analyzedMethods++; | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
This issue also appears on line 452 of the same file.
| foreach (var method in methods) | |
| { | |
| int complexity = CalculateCyclomaticComplexity(method); | |
| complexityScores.Add(complexity); | |
| analyzedMethods++; | |
| } | |
| var methodComplexities = methods | |
| .Select(method => CalculateCyclomaticComplexity(method)) | |
| .ToList(); | |
| complexityScores.AddRange(methodComplexities); | |
| analyzedMethods += methodComplexities.Count; |
| !f.Contains(".min.")); | ||
| codeFiles.AddRange(files); | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
Poor error handling: empty catch block.
This issue also appears in the following locations of the same file:
- line 89
- line 205
- line 318
| catch { } | |
| catch (Exception ex) | |
| { | |
| AssessmentConfig.Findings["CodeComplexity"].Weaknesses.Add( | |
| $"Failed to scan files with extension '{ext}' in repository path '{AssessmentConfig.RepoPath}': {ex.Message}"); | |
| } |
| if (graph.ContainsKey(node)) | ||
| { | ||
| foreach (var neighbor in graph[node]) |
There was a problem hiding this comment.
Inefficient use of 'ContainsKey' and indexer.
This issue also appears on line 446 of the same file.
| if (graph.ContainsKey(node)) | |
| { | |
| foreach (var neighbor in graph[node]) | |
| if (graph.TryGetValue(node, out var neighbors)) | |
| { | |
| foreach (var neighbor in neighbors) |
Current assessment evaluates documentation and tooling but not whether code structure allows AI to reason about changes. A repository with comprehensive README and build config can score A+ despite having deeply coupled spaghetti code that confounds AI context windows.
Changes
New CodeComplexityAssessor (25 points)
if,while,for,&&,||, etc). Scores based on avg: <5 excellent, <10 manageable, ≥15 fails.Scoring adjustments
Example output
Version bumped to 3.1.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.