-
Notifications
You must be signed in to change notification settings - Fork 25
Minor improvements on code quality and documentation across multiple components #417
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
Changes from all commits
a94b3eb
e9ed59c
e0a3000
6c604dc
fe5c638
411f533
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
# ---- Maven | ||
target/ | ||
*.class | ||
|
||
# ---- IntelliJ IDEA | ||
*.iws | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,9 +30,35 @@ | |
import org.sonar.plugins.communitydelphi.api.ast.StatementNode; | ||
import org.sonar.plugins.communitydelphi.api.token.DelphiToken; | ||
|
||
/** | ||
* Visitor that collects various code metrics from Delphi source code. | ||
* | ||
* <p>This visitor traverses the AST and collects metrics including: | ||
* | ||
* <ul> | ||
* <li>Number of classes, routines, and statements | ||
* <li>Cyclomatic and cognitive complexity | ||
* <li>Code and comment line counts | ||
* </ul> | ||
*/ | ||
public class MetricsVisitor implements DelphiParserVisitor<Data> { | ||
private static final Pattern NEW_LINE_PATTERN = Pattern.compile("\r\n|\n|\r"); | ||
|
||
/** | ||
* 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. | ||
*/ | ||
Comment on lines
+47
to
+61
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. 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. Positive FeedbackNegative Feedback
Comment on lines
+47
to
+61
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. I don't find this comment valuable. |
||
public static class Data { | ||
private int classes; | ||
private int routines; | ||
|
@@ -73,15 +99,24 @@ public Set<Integer> getCodeLines() { | |
|
||
@Override | ||
public Data visit(DelphiAst ast, Data data) { | ||
calculateComplexityMetrics(ast, data); | ||
return DelphiParserVisitor.super.visit(ast, data); | ||
} | ||
|
||
/** | ||
* Calculates both cyclomatic and cognitive complexity metrics for the given AST. | ||
* | ||
* @param ast the AST to analyze | ||
* @param data the data container to populate with complexity metrics | ||
*/ | ||
private void calculateComplexityMetrics(DelphiAst ast, Data data) { | ||
var cyclomaticVisitor = new CyclomaticComplexityVisitor(); | ||
var cyclomaticComplexity = cyclomaticVisitor.visit(ast, new CyclomaticComplexityVisitor.Data()); | ||
data.complexity = cyclomaticComplexity.getComplexity(); | ||
|
||
var cognitiveVisitor = new CognitiveComplexityVisitor(); | ||
var cognitiveComplexity = cognitiveVisitor.visit(ast, new CognitiveComplexityVisitor.Data()); | ||
data.cognitiveComplexity = cognitiveComplexity.getComplexity(); | ||
|
||
return DelphiParserVisitor.super.visit(ast, data); | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -26,26 +26,59 @@ | |||||||||||||
import org.sonar.api.utils.AnnotationUtils; | ||||||||||||||
import org.sonar.check.Rule; | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* Utility class for reading and processing rule template annotations. | ||||||||||||||
* | ||||||||||||||
* <p>This class handles the configuration of SonarQube rules based on annotations found on rule | ||||||||||||||
* classes, particularly {@link RuleTemplate} annotations. | ||||||||||||||
*/ | ||||||||||||||
public final class RuleTemplateAnnotationReader { | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* Updates rule definitions in the repository based on rule template annotations. | ||||||||||||||
* | ||||||||||||||
* @param repository the rules repository to update | ||||||||||||||
* @param ruleClasses the list of rule classes to process | ||||||||||||||
* @throws IllegalArgumentException if repository or ruleClasses is null | ||||||||||||||
*/ | ||||||||||||||
public void updateRulesByAnnotatedClass( | ||||||||||||||
RulesDefinition.NewRepository repository, List<Class<?>> ruleClasses) { | ||||||||||||||
ruleClasses.forEach(ruleClass -> handleTemplateRule(repository, ruleClass)); | ||||||||||||||
} | ||||||||||||||
if (repository == null) { | ||||||||||||||
throw new IllegalArgumentException("Repository cannot be null"); | ||||||||||||||
} | ||||||||||||||
if (ruleClasses == null) { | ||||||||||||||
throw new IllegalArgumentException("Rule classes cannot be null"); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
private static String ruleKey(Class<?> ruleClass) { | ||||||||||||||
return AnnotationUtils.getAnnotation(ruleClass, Rule.class).key(); | ||||||||||||||
ruleClasses.forEach(ruleClass -> processTemplateRule(repository, ruleClass)); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
private static boolean isTemplateRule(Class<?> ruleClass) { | ||||||||||||||
return AnnotationUtils.getAnnotation(ruleClass, RuleTemplate.class) != null; | ||||||||||||||
} | ||||||||||||||
private static void processTemplateRule(NewRepository repository, Class<?> ruleClass) { | ||||||||||||||
String key = extractRuleKey(ruleClass); | ||||||||||||||
NewRule rule = Objects.requireNonNull(repository.rule(key), "Rule not found for key: " + key); | ||||||||||||||
|
||||||||||||||
private static void handleTemplateRule(NewRepository repository, Class<?> ruleClass) { | ||||||||||||||
NewRule rule = Objects.requireNonNull(repository.rule(ruleKey(ruleClass))); | ||||||||||||||
if (isTemplateRule(ruleClass)) { | ||||||||||||||
rule.setTemplate(true); | ||||||||||||||
} else { | ||||||||||||||
rule.params().removeIf(param -> param.key().equals("scope")); | ||||||||||||||
// 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. | ||||||||||||||
Comment on lines
+63
to
+67
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. [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
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback 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. How about this? // Configurable rule scopes are only exposed for template rules. |
||||||||||||||
rule.params().removeIf(param -> "scope".equals(param.key())); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
private static String extractRuleKey(Class<?> ruleClass) { | ||||||||||||||
Rule ruleAnnotation = AnnotationUtils.getAnnotation(ruleClass, Rule.class); | ||||||||||||||
if (ruleAnnotation == null) { | ||||||||||||||
throw new IllegalArgumentException( | ||||||||||||||
"Rule class must have @Rule annotation: " + ruleClass.getName()); | ||||||||||||||
} | ||||||||||||||
return ruleAnnotation.key(); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
private static boolean isTemplateRule(Class<?> ruleClass) { | ||||||||||||||
return AnnotationUtils.getAnnotation(ruleClass, RuleTemplate.class) != null; | ||||||||||||||
} | ||||||||||||||
} |
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. This is an arbitrary change, revert it. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,16 @@ | ||
<h2>Why is this an issue?</h2> | ||
<p>My stupid rule to avoid routine declaration</p> | ||
<p>This rule detects routine declarations that should be avoided according to coding standards. | ||
Routine declarations in certain contexts can lead to maintenance issues and reduced code readability.</p> | ||
<h3>Noncompliant Code Example</h3> | ||
<pre> | ||
TO DO | ||
procedure MyProcedure(); forward; // Noncompliant - forward declaration usage | ||
|
||
function Calculate(X: Integer): Integer; external; // Noncompliant - external routine | ||
</pre> | ||
<h3>Compliant Solution</h3> | ||
<pre> | ||
TO DO | ||
function Calculate(X: Integer): Integer; // Compliant - proper implementation | ||
begin | ||
Result := X * 2; | ||
end; | ||
</pre> |
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.
There's no reason for this, as target folders are already ignored.