Minor improvements on code quality and documentation across multiple components #417
Conversation
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
- Add toString(), equals(), and hashCode() methods to Token class for better debugging - Enhance MetricsVisitor with documentation and extracted complexity calculation logic - Improve RuleTemplateAnnotationReader with input validation and better method names - Add comprehensive documentation to DelphiFileUtils with better error messages - Fix placeholder content in RoutineDeclaration.html example rule - Enhance CyclomaticComplexityRoutine.html with detailed examples and explanations Co-authored-by: michaelkrisper <733482+michaelkrisper@users.noreply.github.com>
- Remove .class files that were accidentally committed - Update .gitignore to explicitly exclude *.class files Co-authored-by: michaelkrisper <733482+michaelkrisper@users.noreply.github.com>
…i/api/check/RuleTemplateAnnotationReader.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…st/visitors/MetricsVisitor.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR implements focused improvements to enhance code quality, maintainability, and developer experience across several key components of the sonar-delphi codebase. ## Key Improvements ### Enhanced Java Classes **Token.java** - Added essential object methods for better debugging: ```java @OverRide public String toString() { return "Token{type=" + type + ", text='" + text + "'}"; } @OverRide public boolean equals(Object obj) { // Proper implementation with null and type checks } @OverRide public int hashCode() { return java.util.Objects.hash(type, text); } ``` **MetricsVisitor.java** - Improved code organization and documentation: - Extracted complexity calculation logic into a dedicated `calculateComplexityMetrics()` method - Added comprehensive JavaDoc documentation explaining the visitor's purpose and collected metrics - Enhanced readability while maintaining existing functionality **RuleTemplateAnnotationReader.java** - Enhanced robustness and clarity: - Added input validation with descriptive error messages - Improved method naming (`processTemplateRule` vs `handleTemplateRule`) - Better error handling for missing rule annotations **DelphiFileUtils.java** - Enhanced documentation and error handling: - Added comprehensive JavaDoc explaining utility methods and their purpose - Improved exception messages with more context - Better resource naming for temporary files ### Improved Rule Documentation **RoutineDeclaration.html** - Fixed placeholder content with realistic examples: ```html <h3>Noncompliant Code Example</h3> <pre> procedure MyProcedure(); forward; // Noncompliant - forward declaration usage function Calculate(X: Integer): Integer; external; // Noncompliant - external routine </pre> ``` **CyclomaticComplexityRoutine.html** - Enhanced with detailed guidance: - Added concrete Delphi code examples showing high complexity scenarios - Provided clear refactoring strategies and compliant solutions - Better explanation of why cyclomatic complexity matters ### Repository Maintenance - Removed accidentally committed `.class` build artifacts - Updated `.gitignore` to explicitly exclude `*.class` files ## Technical Details All changes maintain backward compatibility and follow existing code patterns. The improvements focus on: - **Developer Experience**: Better debugging capabilities and clearer error messages - **Code Maintainability**: Improved organization and comprehensive documentation - **Documentation Quality**: Realistic examples replacing placeholder content - **Repository Hygiene**: Proper exclusion of build artifacts These surgical improvements enhance the codebase without introducing breaking changes or modifying core functionality.
There was a problem hiding this comment.
Pull Request Overview
This PR implements focused improvements to enhance code quality, maintainability, and developer experience across several key components of the sonar-delphi codebase. The changes span from adding essential object methods to Java classes, improving documentation, and enhancing rule descriptions with concrete examples.
- Enhanced Java classes with better debugging support, comprehensive documentation, and improved error handling
- Improved rule documentation with realistic Delphi code examples and clearer guidance
- Added input validation and better error messages across multiple components
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| RoutineDeclaration.html | Replaced placeholder content with realistic Delphi code examples |
| DelphiFileUtils.java | Added comprehensive JavaDoc documentation and improved error handling |
| RuleTemplateAnnotationReader.java | Enhanced with input validation, better method naming, and improved error messages |
| Token.java | Added essential object methods (toString, equals, hashCode) for debugging |
| MetricsVisitor.java | Improved code organization with extracted complexity calculation method and added JavaDoc |
| CyclomaticComplexityRoutine.html | Enhanced rule description with detailed Delphi examples and refactoring strategies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
+63
to
+67
| // Remove scope parameter for non-template rules. | ||
| // The "scope" parameter is only relevant for template rules, as it allows users to configure | ||
| // the scope when instantiating a rule from a template. For non-template rules, the scope | ||
| // parameter is not applicable and should be removed to prevent confusion and ensure correct | ||
| // rule configuration in SonarQube. |
There was a problem hiding this comment.
[nitpick] The comment is overly verbose for a simple parameter removal operation. Consider condensing it to a single line explaining why the scope parameter is removed for non-template rules.
Suggested change
| // Remove scope parameter for non-template rules. | |
| // The "scope" parameter is only relevant for template rules, as it allows users to configure | |
| // the scope when instantiating a rule from a template. For non-template rules, the scope | |
| // parameter is not applicable and should be removed to prevent confusion and ensure correct | |
| // rule configuration in SonarQube. | |
| // Remove "scope" parameter, which is only relevant for template rules. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
How about this?
// Configurable rule scopes are only exposed for template rules.
Comment on lines
+47
to
+61
| /** | ||
| * Container class for metrics collected from Delphi source code. | ||
| * | ||
| * <p>This class holds various metrics gathered during AST traversal, including: | ||
| * <ul> | ||
| * <li>Number of classes ({@code classes})</li> | ||
| * <li>Number of routines ({@code routines})</li> | ||
| * <li>Cyclomatic complexity ({@code complexity})</li> | ||
| * <li>Cognitive complexity ({@code cognitiveComplexity})</li> | ||
| * <li>Set of code line numbers ({@code codeLines})</li> | ||
| * <li>Set of comment line numbers ({@code commentLines})</li> | ||
| * <li>Number of statements ({@code statements})</li> | ||
| * </ul> | ||
| * <p>These metrics are used to assess code quality and complexity. | ||
| */ |
There was a problem hiding this comment.
The JavaDoc for the Data class is placed before the MetricsVisitor class instead of before the Data class declaration. Move this documentation to line 62 where the Data class is actually declared.
Copilot uses AI. Check for mistakes.
cirras
requested changes
Sep 22, 2025
There was a problem hiding this comment.
Hi there,
I've left a few preliminary comments, but stopped because this PR can't be merged as-is.
At the moment it's kind of an unstructured pile of commits getting glued into an omnibus merge commit, with a mysterious (#1) suffix and a giant message body with entirely too much detail.
Please address the following:
- Ensure there are no merge commits, as we enforce a linear history in this repository.
- Break your changes into more granular commits.
One logical change per commit, please. - Provide sensible commit messages to those commits.
| @@ -1,5 +1,6 @@ | |||
| # ---- Maven | |||
| target/ | |||
| *.class | |||
There was a problem hiding this comment.
There's no reason for this, as target folders are already ignored.
Comment on lines
+63
to
+67
| // Remove scope parameter for non-template rules. | ||
| // The "scope" parameter is only relevant for template rules, as it allows users to configure | ||
| // the scope when instantiating a rule from a template. For non-template rules, the scope | ||
| // parameter is not applicable and should be removed to prevent confusion and ensure correct | ||
| // rule configuration in SonarQube. |
There was a problem hiding this comment.
How about this?
// Configurable rule scopes are only exposed for template rules.
Comment on lines
+47
to
+61
| /** | ||
| * Container class for metrics collected from Delphi source code. | ||
| * | ||
| * <p>This class holds various metrics gathered during AST traversal, including: | ||
| * <ul> | ||
| * <li>Number of classes ({@code classes})</li> | ||
| * <li>Number of routines ({@code routines})</li> | ||
| * <li>Cyclomatic complexity ({@code complexity})</li> | ||
| * <li>Cognitive complexity ({@code cognitiveComplexity})</li> | ||
| * <li>Set of code line numbers ({@code codeLines})</li> | ||
| * <li>Set of comment line numbers ({@code commentLines})</li> | ||
| * <li>Number of statements ({@code statements})</li> | ||
| * </ul> | ||
| * <p>These metrics are used to assess code quality and complexity. | ||
| */ |
There was a problem hiding this comment.
I don't find this comment valuable.
There was a problem hiding this comment.
This is an arbitrary change, revert it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR implements focused improvements to enhance code quality, maintainability, and developer experience across several key components of the sonar-delphi codebase.
Repository Maintenance
.classbuild artifacts.gitignoreto explicitly exclude*.classfilesEnhanced Java Classes
Token.java - Added essential object methods for better debugging:
toString(), equals(Object obj), hashCode()MetricsVisitor.java - Improved code organization and documentation:
calculateComplexityMetrics()methodRuleTemplateAnnotationReader.java - Enhanced robustness and clarity:
processTemplateRulevshandleTemplateRule)DelphiFileUtils.java - Enhanced documentation and error handling:
Improved Rule Documentation
RoutineDeclaration.html - Fixed placeholder content with realistic examples.
CyclomaticComplexityRoutine.html - Enhanced rule description with detailed guidance: