-
-
Notifications
You must be signed in to change notification settings - Fork 0
Add Code Complexity and Maintainability Audit Report #58
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
Open
Snider
wants to merge
3
commits into
dev
Choose a base branch
from
feat/add-code-complexity-audit-5935141500850508550
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| # Code Complexity & Maintainability Audit | ||
|
|
||
| This audit analyzes the code quality of the Core PHP Framework, identifies maintainability issues, and provides recommendations for improvement. The findings are prioritized by their impact on the maintenance burden. | ||
|
|
||
| ## High Impact Findings | ||
|
|
||
| None. The codebase is generally well-structured and follows modern best practices. | ||
|
Comment on lines
+5
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hyphenate compound adjectives in headings. ✏️ Suggested edit-## High Impact Findings
+## High-Impact Findings
@@
-## Low Impact Findings
+## Low-Impact FindingsAlso applies to: 63-65 🧰 Tools🪛 LanguageTool[grammar] ~5-~5: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) 🤖 Prompt for AI Agents |
||
|
|
||
| ## Medium Impact Findings | ||
|
|
||
| ### 1. Rigid Namespace Detection in `ModuleScanner` | ||
|
|
||
| **File:** `src/Core/ModuleScanner.php` | ||
| **Method:** `classFromFile()` | ||
|
|
||
| **Issue:** | ||
| The `classFromFile()` method uses a series of `if (str_contains(...))` checks to determine the root namespace for a module based on its file path. This approach is rigid and violates the Open/Closed Principle. Adding a new module type (e.g., `app/Addon/`) would require modifying this core file, which increases the risk of regressions. | ||
|
|
||
| **Recommendation:** | ||
| Refactor the `if` chain into a more flexible, data-driven approach. A mapping array can be used to associate base paths with their corresponding root namespaces. This would allow new module types to be registered through configuration rather than code changes, improving extensibility and maintainability. | ||
|
|
||
| **Code Example of Improvement:** | ||
|
|
||
| ```php | ||
| // Before | ||
| private function classFromFile(string $file, string $basePath): ?string | ||
| { | ||
| // ... namespace derivation logic ... | ||
|
|
||
| if (str_contains($basePath, '/Core')) { | ||
| return "Core\\{$namespace}"; | ||
| } | ||
| if (str_contains($basePath, '/Mod')) { | ||
| return "Mod\\{$namespace}"; | ||
| } | ||
| // ... more if statements ... | ||
| } | ||
|
|
||
| // After | ||
| private function classFromFile(string $file, string $basePath): ?string | ||
| { | ||
| // A configurable mapping of base paths to namespaces | ||
| $namespaceMapping = [ | ||
| '/Core' => 'Core\\', | ||
| '/Mod' => 'Mod\\', | ||
| '/Website' => 'Website\\', | ||
| '/Plug' => 'Plug\\', | ||
| ]; | ||
|
|
||
| // ... namespace derivation logic ... | ||
|
|
||
| foreach ($namespaceMapping as $pathFragment => $rootNamespace) { | ||
| if (str_contains($basePath, $pathFragment)) { | ||
| return "{$rootNamespace}{$namespace}"; | ||
| } | ||
| } | ||
|
|
||
| // Fallback or error handling | ||
| return null; | ||
| } | ||
| ``` | ||
|
|
||
| ## Low Impact Findings | ||
|
|
||
| ### 2. Minor Code Duplication in `ModuleRegistry` | ||
|
|
||
| **File:** `src/Core/ModuleRegistry.php` | ||
| **Methods:** `register()`, `addPaths()` | ||
|
|
||
| **Issue:** | ||
| The `register()` and `addPaths()` methods share similar logic for iterating through newly discovered mappings, sorting them by priority, and registering them with Laravel's event listener. This represents a minor case of code duplication. | ||
|
|
||
| **Recommendation:** | ||
| Extract the common logic into a private helper method to reduce duplication and improve clarity. This would centralize the listener registration process, making the code more DRY (Don't Repeat Yourself). | ||
|
|
||
| **Code Example of Improvement:** | ||
|
|
||
| ```php | ||
| // Before | ||
| public function register(array $paths): void | ||
| { | ||
| // ... | ||
| foreach ($this->mappings as $event => $listeners) { | ||
| $sorted = $this->sortByPriority($listeners); | ||
| foreach ($sorted as $moduleClass => $config) { | ||
| Event::listen($event, new LazyModuleListener($moduleClass, $config['method'])); | ||
| } | ||
| } | ||
| // ... | ||
| } | ||
|
|
||
| public function addPaths(array $paths): void | ||
| { | ||
| // ... | ||
| foreach ($newMappings as $event => $listeners) { | ||
| $sorted = $this->sortByPriority($listeners); | ||
| foreach ($sorted as $moduleClass => $config) { | ||
| // ... logic to skip if already registered ... | ||
| $this->mappings[$event][$moduleClass] = $config; | ||
| Event::listen($event, new LazyModuleListener($moduleClass, $config['method'])); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // After | ||
| public function register(array $paths): void | ||
| { | ||
| // ... | ||
| $this->registerListenersFromMappings($this->mappings); | ||
| // ... | ||
| } | ||
|
|
||
| public function addPaths(array $paths): void | ||
| { | ||
| // ... | ||
| $this->registerListenersFromMappings($newMappings, true); // true to check for existing | ||
| // ... | ||
| } | ||
|
|
||
| private function registerListenersFromMappings(array $mappings, bool $checkExisting = false): void | ||
| { | ||
| foreach ($mappings as $event => $listeners) { | ||
| $sorted = $this->sortByPriority($listeners); | ||
| foreach ($sorted as $moduleClass => $config) { | ||
| if ($checkExisting && isset($this->mappings[$event][$moduleClass])) { | ||
| continue; | ||
| } | ||
| if ($checkExisting) { | ||
| $this->mappings[$event][$moduleClass] = $config; | ||
| } | ||
| Event::listen($event, new LazyModuleListener($moduleClass, $config['method'])); | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Restore failing-test signal (avoid unconditional
exit 0).Exiting 0 makes the Tests job green even when PHPUnit fails, which removes the quality gate. Since the report job already uses
if: always(), you can preserve the failure signal without blocking the summary generation.🔧 Proposed fix
🤖 Prompt for AI Agents