Refactor project for better maintainability#1
Conversation
Major architectural improvements: ## New Structure - Created modular architecture with clear separation of concerns - Added Configuration/, Interfaces/, Services/, and Utils/ namespaces - Split monolithic FileChecker (340 lines) into focused classes ## Key Changes - **FilterConfiguration**: Externalized all filter settings and constants - **IFileChecker & IConsoleWriter**: Added interfaces for testability - **FileFilterService**: Core filtering logic with dependency injection - **GitIgnoreParser**: Dedicated gitignore pattern matching with caching - **ProjectScanner**: Separated scanning logic from I/O concerns - **FileUtilities**: Improved binary detection with better error handling ## Improvements - Removed mutable static state (used Lazy<T> for backward compatibility) - Added comprehensive XML documentation on all public APIs - Enhanced error handling with specific exception types and exit codes - Updated CI/CD to use .NET 9.0 and modern GitHub Actions (v4) - Fixed deprecated actions (create-release@v1 → action-gh-release@v2) - Updated README with current architecture and .NET 9 requirements ## Backward Compatibility - Legacy FileChecker, MyAppsContext, and FileUtils classes preserved as wrappers - Marked obsolete with migration guidance - Existing code continues to work without changes ## Benefits - Testable: Interfaces enable unit testing and mocking - Maintainable: Single Responsibility Principle applied throughout - Extensible: Easy to add new filters or scanners - Documented: XML comments on all public members - Robust: Better error handling and validation
There was a problem hiding this comment.
Pull request overview
This PR represents a major architectural refactoring that transforms a monolithic 340-line FileChecker class into a well-structured, modular codebase following SOLID principles. The refactoring introduces dependency injection, interface-based design, and comprehensive XML documentation while maintaining backward compatibility through obsolete wrapper classes.
Key Changes
- Modular Architecture: Split monolithic code into focused services (FileFilterService, ProjectScanner, GitIgnoreParser) with clear separation of concerns
- Dependency Injection: Introduced interfaces (IFileChecker, IConsoleWriter) and constructor injection for improved testability
- Enhanced Infrastructure: Updated to .NET 9.0, modernized GitHub Actions (v4), and improved error handling with specific exception types and exit codes
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| Configuration/FilterConfiguration.cs | Externalized all filter settings and constants into a dedicated configuration class |
| Interfaces/IFileChecker.cs | Interface for file filtering operations enabling testability |
| Interfaces/IConsoleWriter.cs | Console abstraction interface for dependency injection |
| Services/FileFilterService.cs | Core filtering logic implementing IFileChecker with gitignore support |
| Services/ProjectScanner.cs | Directory scanning and file content extraction separated from I/O concerns |
| Services/GitIgnoreParser.cs | Dedicated gitignore pattern matching with regex caching |
| Services/ConsoleWriter.cs | Standard console implementation of IConsoleWriter interface |
| Utils/FileUtilities.cs | Binary file detection utility with improved error handling |
| FileChecker.cs | Legacy wrapper using Lazy singleton pattern for backward compatibility |
| FileUtils.cs | Legacy wrapper delegating to FileUtilities |
| MyAppsContext.cs | Legacy wrapper delegating to ProjectScanner |
| Program.cs | Enhanced entry point with structured exception handling and comprehensive documentation |
| README.md | Updated documentation reflecting new architecture, .NET 9 requirements, and improved usage examples |
| .github/workflows/build.yml | Modernized CI/CD pipeline with .NET 9.0 and GitHub Actions v4, removed test step |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static string ConvertGitIgnorePatternToRegex(string pattern) | ||
| { | ||
| // Simple conversion - could be enhanced for full gitignore spec | ||
| return pattern | ||
| .Replace(".", "\\.") | ||
| .Replace("*", ".*") | ||
| .Replace("?", "."); | ||
| } |
There was a problem hiding this comment.
The gitignore pattern conversion is overly simplistic and doesn't handle many gitignore features correctly. For example:
- It doesn't handle negation patterns (starting with
!) - It doesn't handle directory-specific patterns (ending with
/) - It doesn't handle patterns starting with
/(root-relative) - It doesn't handle
**(match across directories) - The
*replacement with.*will match directory separators, which is incorrect
This could lead to incorrect filtering behavior. Consider using a proper gitignore parser library or implementing a more complete gitignore spec.
| catch (Exception) | ||
| { | ||
| // If we can't read the file (permissions, etc.), assume it's not binary | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Empty catch block swallows all exceptions without logging. While the comment explains the intent, silently ignoring ALL exceptions (including unexpected ones like OutOfMemoryException) could hide serious issues. Consider catching specific exceptions (IOException, UnauthorizedAccessException) and logging unexpected exceptions for debugging purposes.
| catch (Exception) | |
| { | |
| // If we can't read the file (permissions, etc.), assume it's not binary | |
| return false; | |
| } | |
| catch (IOException) | |
| { | |
| // If we can't read the file (permissions, etc.), assume it's not binary | |
| return false; | |
| } | |
| catch (UnauthorizedAccessException) | |
| { | |
| // If we don't have permission to read the file, assume it's not binary | |
| return false; | |
| } | |
| catch (Exception ex) | |
| { | |
| // Log unexpected exceptions for debugging purposes and rethrow | |
| System.Diagnostics.Debug.WriteLine($"Unexpected exception in IsBinaryFile: {ex}"); | |
| throw; | |
| } |
| var formattedContent = format.ToLower() == "json" | ||
| ? JsonSerializer.Serialize(new { content, timestamp = DateTime.Now }) | ||
| ? JsonSerializer.Serialize(new { content, timestamp = DateTime.Now }, new JsonSerializerOptions { WriteIndented = true }) | ||
| : content; |
There was a problem hiding this comment.
[nitpick] The JSON serialization includes a WriteIndented = true option which is good for readability but wasn't present before. However, this creates inconsistent formatting between text and JSON outputs - JSON will be indented but the serializer itself doesn't add indentation to the nested content string. Consider documenting this behavior or ensuring consistent formatting across output types.
| _patterns.Clear(); | ||
| _regexCache.Clear(); | ||
|
|
||
| var lines = File.ReadAllLines(gitIgnorePath) | ||
| .Where(line => !string.IsNullOrWhiteSpace(line) && !line.StartsWith('#')); | ||
|
|
||
| _patterns.AddRange(lines); | ||
| } |
There was a problem hiding this comment.
[nitpick] The LoadFromFile method clears existing patterns and cache every time it's called. If this is called multiple times (e.g., when scanning different repositories), previously compiled regex patterns are discarded and need to be recompiled. Consider whether this is the intended behavior or if patterns should be cached per file path.
| private bool IsGeneratedCode(string filePath) | ||
| { | ||
| try | ||
| { | ||
| var lines = File.ReadLines(filePath).Take(_config.GeneratedCodeLinesToCheck); | ||
| return lines.Any(line => line.Contains("<auto-generated />")); | ||
| } | ||
| catch (Exception) | ||
| { | ||
| // If we can't read the file, assume it's not generated code | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The IsGeneratedCode check reads file lines even after filtering by extension, size, and binary checks. For large text files under the size limit, this adds an extra I/O operation. Consider moving this check earlier or making it more efficient by only checking if the file extension suggests it could be generated code (e.g., .cs, .designer.cs files).
| _gitRepoRoot ??= FindGitRepoRoot(path); | ||
|
|
||
| if (indent == 0) | ||
| { | ||
| _console.WriteLine("📁 Analyzing directory structure..."); | ||
| } | ||
|
|
||
| var rootPath = _gitRepoRoot ?? path; | ||
| var entries = Directory.EnumerateFileSystemEntries(path) | ||
| .OrderBy(e => e) | ||
| .Where(e => !_fileChecker.ShouldSkip(new FileInfo(e), rootPath)) | ||
| .ToList(); | ||
|
|
||
| var sb = new StringBuilder(); | ||
|
|
||
| for (int i = 0; i < entries.Count; i++) | ||
| { | ||
| WriteProgress(i + 1, entries.Count); | ||
| var entry = entries[i]; | ||
|
|
||
| if (Directory.Exists(entry)) | ||
| { | ||
| var dir = new DirectoryInfo(entry); | ||
| sb.AppendLine($"{new string(' ', indent * 2)}[{dir.Name}/]"); | ||
| sb.Append(GetProjectStructure(entry, indent + 1)); | ||
| } | ||
| else | ||
| { | ||
| var file = new FileInfo(entry); | ||
| sb.AppendLine($"{new string(' ', indent * 2)}[{file.Extension}] {file.Name}"); | ||
| } | ||
| } | ||
|
|
||
| return sb.ToString(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Retrieves the contents of all non-filtered files in the directory tree. | ||
| /// </summary> | ||
| /// <param name="path">The directory path to scan.</param> | ||
| /// <returns>A string containing all file contents with separators.</returns> | ||
| public string GetFileContents(string path) | ||
| { | ||
| if (string.IsNullOrEmpty(path)) | ||
| { | ||
| throw new ArgumentException("Path cannot be null or empty.", nameof(path)); | ||
| } | ||
|
|
||
| if (!Directory.Exists(path)) | ||
| { | ||
| throw new DirectoryNotFoundException($"Directory not found: {path}"); | ||
| } | ||
|
|
||
| _gitRepoRoot ??= FindGitRepoRoot(path); |
There was a problem hiding this comment.
The null-coalescing assignment ??= on line 55 and 108 could cause subtle bugs. If _gitRepoRoot is set to a non-null value on the first call but a different path is passed in subsequent calls, the old git repo root will be used. This is particularly problematic if the scanner is reused across multiple independent directory scans. Consider making FindGitRepoRoot a parameter or determining it fresh for each path.
| private static string? FindGitRepoRoot(string path) | ||
| { | ||
| var currentPath = path; | ||
| while (!string.IsNullOrEmpty(currentPath)) | ||
| { | ||
| if (Directory.Exists(Path.Combine(currentPath, ".git"))) | ||
| { | ||
| return currentPath; | ||
| } | ||
| currentPath = Path.GetDirectoryName(currentPath); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
The FindGitRepoRoot method is duplicated across multiple classes (FileFilterService, ProjectScanner). This violates the DRY principle and makes maintenance harder. Consider extracting this to a shared utility class or service that can be injected.
| var secondLastDotIndex = fileName.LastIndexOf('.', lastDotIndex - 1); | ||
| if (secondLastDotIndex >= 0) | ||
| { | ||
| var compoundExtension = fileName.Substring(secondLastDotIndex); |
There was a problem hiding this comment.
[nitpick] The compound extension check using Substring could throw an ArgumentOutOfRangeException if secondLastDotIndex is negative (when there's only one dot in the filename). While the condition secondLastDotIndex >= 0 guards this on line 105, using Substring with a start index from the beginning to the end of the string is safer. Consider using fileName[secondLastDotIndex..] (range operator) for better readability and safety.
| var compoundExtension = fileName.Substring(secondLastDotIndex); | |
| var compoundExtension = fileName[secondLastDotIndex..]; |
| /// <summary> | ||
| /// Gets the root path of the git repository containing the specified path. | ||
| /// </summary> | ||
| public string? GitRepoRoot => _gitRepoRoot; |
There was a problem hiding this comment.
Inconsistent return type: The GitRepoRoot property returns string? (nullable), but the code in lines 62 and 111 uses the null-coalescing operator with path as fallback. This means rootPath is never null at those points, but the property getter can return null. This inconsistency could confuse users of the API about when null is actually returned.
| public string? GitRepoRoot => _gitRepoRoot; | |
| public string GitRepoRoot => _gitRepoRoot ?? string.Empty; |
| { | ||
| Console.WriteLine("\n💾 Writing output..."); | ||
| string resolvedFilePath = ""; | ||
| string resolvedFilePath; |
There was a problem hiding this comment.
The variable name resolvedFilePath is declared but not initialized before the try block, then assigned inside. If an exception occurs before assignment, the catch block references an uninitialized variable. While the current code won't reach that point due to the initialization on line 217, this pattern is fragile. Consider initializing to string.Empty or restructuring the error handling to avoid this issue.
| string resolvedFilePath; | |
| string resolvedFilePath = string.Empty; |
Major architectural improvements:
New Structure
Key Changes
Improvements
Backward Compatibility
Benefits