From d6355995d58a7a4a77d799512bf4f8590e76dcd3 Mon Sep 17 00:00:00 2001 From: Bora Ciner Date: Sun, 2 Nov 2025 16:25:22 +0100 Subject: [PATCH 1/8] add recipe to replace magic numbers with named constants Introduce `ReplaceMagicNumbersWithConstants` recipe implementing Sonar rule java:S109. This recipe extracts numeric literals from method bodies into private static final constants to improve code readability and maintainability. Also add `ReplaceMagicNumbersWithConstantsTest` with test cases verifying: - constants are generated for numeric literals - literals already assigned to variables or fields are ignored - replacements preserve correctness and type consistency --- .../ReplaceMagicNumbersWithConstants.java | 136 ++++++++++++++++++ .../ReplaceMagicNumbersWithConstantsTest.java | 105 ++++++++++++++ 2 files changed, 241 insertions(+) create mode 100644 src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java create mode 100644 src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java diff --git a/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java b/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java new file mode 100644 index 0000000000..7bdc9b8c1e --- /dev/null +++ b/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java @@ -0,0 +1,136 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.staticanalysis; + +import org.openrewrite.*; +import org.openrewrite.java.JavaTemplate; +import org.openrewrite.java.JavaVisitor; +import org.openrewrite.java.tree.J; + +import java.time.Duration; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Set; + +/** + * This recipe replaces magic number literals in method bodies with named constants following the Sonar java:S109 rule. + *
+ * All detected magic numbers (excluding those explicitly assigned to variables or fields) will be extracted as + * private static final constants at the top of the class. + * The original numeric usages are replaced with the new constant name to improve code readability and maintainability. + *
+ * Currently, unsupported: + * - The recipe will not create constants for literals already declared as field/variable assignments. + * - The recipe does not ignore typical non-magic numbers (like 0, 1, -1); this can be extended for full Sonar parity. + * - Only basic type literals (e.g., int, double, float) are handled. String and char literals are not affected. + *
+ * Note: The constant name is generated based on the type and value, and may contain underscores or "NEGATIVE_" for negative values. + */ +public class ReplaceMagicNumbersWithConstants extends Recipe { + private static final String CUSTOM_MODIFIERS = "private static final"; + + @Override + public @NlsRewrite.DisplayName String getDisplayName() { + return "How to use Visitors"; + } + + @Override + public @NlsRewrite.Description String getDescription() { + return "Replaces magic number literals in method bodies with named constants to improve code readability and maintainability. " + + "Magic numbers are replaced by private static final constants declared at the top of the class, following Sonar's java:S109 rule. " + + "The recipe does not create constants for literals that are already assigned to fields or variables, nor for typical non-magic numbers (such as 0, 1, or -1). " + + "Currently, only numeric primitive literals are handled; string and character literals are unaffected. " + + "If a constant for a value already exists, or the constant name would conflict with an existing symbol, the recipe will skip that value."; + } + @Override + public TreeVisitor getVisitor() { + return new JavaVisitor(){ + @Override + public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) { + J.ClassDeclaration cd = (J.ClassDeclaration) super.visitClassDeclaration(classDecl, ctx); + + List literals = new ArrayList<>(); + new JavaVisitor() { + @Override + public J visitLiteral(J.Literal literal, ExecutionContext ctx2) { + Cursor cursor = getCursor(); + if(!(cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable)){ + literals.add(literal); + } + return literal; + } + }.visit(classDecl, ctx); + + List newFieldSources = new ArrayList<>(); + for (J.Literal literal : literals) { + String constantName = getStrValFromLiteral(literal); + boolean alreadyExists = cd.getBody().getStatements().stream() + .filter(J.VariableDeclarations.class::isInstance) + .map(J.VariableDeclarations.class::cast) + .flatMap(vars -> vars.getVariables().stream()) + .anyMatch(var -> var.getSimpleName().equals(constantName)); + if (!alreadyExists) { + String modifiers = CUSTOM_MODIFIERS; // this is "private static final" + String typeName = literal.getType() == null ? "Object" : literal.getType().toString(); + String fieldSource = modifiers + " " + typeName + " " + constantName + " = " + literal.getValueSource() + ";"; + newFieldSources.add(fieldSource); + } + } + if (newFieldSources.isEmpty()) { + return cd; + } + + String templateStr = String.join("\n", newFieldSources); + JavaTemplate template = JavaTemplate.builder(templateStr) + .contextSensitive() + .build(); + Cursor bodyCursor = new Cursor(getCursor(), cd.getBody()); + J.Block updatedBody = template.apply(bodyCursor, cd.getBody().getCoordinates().firstStatement()); + + return cd.withBody(updatedBody); + } + + + @Override + public J visitLiteral(J.Literal literal, ExecutionContext ctx) { + Cursor cursor = getCursor(); + if(cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable){ + return super.visitLiteral(literal, ctx); + } + String valueSource = literal.getValueSource(); + String constantName = getStrValFromLiteral(literal); // e.g., "DOUBLE_51_0" + if (constantName != null) { + JavaTemplate template = JavaTemplate.builder(constantName).build(); + return template.apply(getCursor(), literal.getCoordinates().replace()); + } + return super.visitLiteral(literal, ctx); + } + }; + } + + @Override + public Set getTags() { + return Collections.singleton("RSPEC-109"); + } + @Override + public Duration getEstimatedEffortPerOccurrence() { + return Duration.ofSeconds(10); + } + private String getStrValFromLiteral(J.Literal literal) { + return literal.getType().toString().toUpperCase() + "_" + literal.getValueSource().replace(".", "_").replace("-", "NEGATIVE_"); + } +} diff --git a/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java b/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java new file mode 100644 index 0000000000..f1c0472dd7 --- /dev/null +++ b/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java @@ -0,0 +1,105 @@ +package org.openrewrite.staticanalysis; +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.java.JavaParser; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; +import org.openrewrite.test.TypeValidation; + +import static org.openrewrite.java.Assertions.java; + +public class ReplaceMagicNumbersWithConstantsTest implements RewriteTest { + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new ReplaceMagicNumbersWithConstants()); + } + + @DocumentExample + @Test + void assignMagicNumbersToConstantsTest() { + rewriteRun( + spec -> spec + .typeValidationOptions(TypeValidation.none()), + java( +""" +public class OrderProcessor { + private static final double myVariable = 99.99; + public double calculateShippingCost(double orderTotal) { + int localVar = 5; + orderTotal = localVar + 10; + if (orderTotal < 51.0) { + return 7.99; + } else { + return 0.0; + } + } +} +""", + """ +public class OrderProcessor { + private static final int INT_10 = 10; + private static final double DOUBLE_51_0 = 51.0; + private static final double DOUBLE_7_99 = 7.99; + private static final double DOUBLE_0_0 = 0.0; + private static final double myVariable = 99.99; + public double calculateShippingCost(double orderTotal) { + int localVar = 5; + orderTotal = localVar + INT_10; + if (orderTotal < DOUBLE_51_0) { + return DOUBLE_7_99; + } else { + return DOUBLE_0_0; + } + } +} + """ + ) + ); + } + + @DocumentExample + @Test + void assignMagicNumbersToConstantsBasicTest() { + rewriteRun( + spec -> spec + .typeValidationOptions(TypeValidation.none()), + java( + """ + public class OrderProcessor { + public double calculateShippingCost(double orderTotal) { + if (orderTotal < 51.0) { + return 7.99; + } + } + } + """, + """ +public class OrderProcessor { + private static final double DOUBLE_51_0 = 51.0; + private static final double DOUBLE_7_99 = 7.99; + public double calculateShippingCost(double orderTotal) { + if (orderTotal < DOUBLE_51_0) { + return DOUBLE_7_99; + } + } +} + """ + ) + ); + } +} From 13256ea86b562e82ab42e8514d479a4e7f699825 Mon Sep 17 00:00:00 2001 From: Bora Ciner Date: Sun, 2 Nov 2025 19:20:10 +0100 Subject: [PATCH 2/8] Update ReplaceMagicNumbersWithConstants.java -1, 0, and 1 are not considered magic numbers. --- .../ReplaceMagicNumbersWithConstants.java | 213 ++++++++++-------- 1 file changed, 118 insertions(+), 95 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java b/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java index 7bdc9b8c1e..90223c4e4d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java +++ b/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java @@ -19,6 +19,7 @@ import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaType; import java.time.Duration; import java.util.ArrayList; @@ -28,109 +29,131 @@ /** * This recipe replaces magic number literals in method bodies with named constants following the Sonar java:S109 rule. - *
- * All detected magic numbers (excluding those explicitly assigned to variables or fields) will be extracted as + * All detected magic numbers (excluding those explicitly assigned to variables or fields, and -1, 0, 1) will be extracted as * private static final constants at the top of the class. * The original numeric usages are replaced with the new constant name to improve code readability and maintainability. - *
- * Currently, unsupported: - * - The recipe will not create constants for literals already declared as field/variable assignments. - * - The recipe does not ignore typical non-magic numbers (like 0, 1, -1); this can be extended for full Sonar parity. - * - Only basic type literals (e.g., int, double, float) are handled. String and char literals are not affected. - *
- * Note: The constant name is generated based on the type and value, and may contain underscores or "NEGATIVE_" for negative values. */ public class ReplaceMagicNumbersWithConstants extends Recipe { - private static final String CUSTOM_MODIFIERS = "private static final"; - - @Override - public @NlsRewrite.DisplayName String getDisplayName() { - return "How to use Visitors"; - } - - @Override - public @NlsRewrite.Description String getDescription() { - return "Replaces magic number literals in method bodies with named constants to improve code readability and maintainability. " + - "Magic numbers are replaced by private static final constants declared at the top of the class, following Sonar's java:S109 rule. " + - "The recipe does not create constants for literals that are already assigned to fields or variables, nor for typical non-magic numbers (such as 0, 1, or -1). " + - "Currently, only numeric primitive literals are handled; string and character literals are unaffected. " + - "If a constant for a value already exists, or the constant name would conflict with an existing symbol, the recipe will skip that value."; - } - @Override - public TreeVisitor getVisitor() { - return new JavaVisitor(){ + private static final String CUSTOM_MODIFIERS = "private static final"; + @Override - public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) { - J.ClassDeclaration cd = (J.ClassDeclaration) super.visitClassDeclaration(classDecl, ctx); - - List literals = new ArrayList<>(); - new JavaVisitor() { - @Override - public J visitLiteral(J.Literal literal, ExecutionContext ctx2) { - Cursor cursor = getCursor(); - if(!(cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable)){ - literals.add(literal); - } - return literal; - } - }.visit(classDecl, ctx); - - List newFieldSources = new ArrayList<>(); - for (J.Literal literal : literals) { - String constantName = getStrValFromLiteral(literal); - boolean alreadyExists = cd.getBody().getStatements().stream() - .filter(J.VariableDeclarations.class::isInstance) - .map(J.VariableDeclarations.class::cast) - .flatMap(vars -> vars.getVariables().stream()) - .anyMatch(var -> var.getSimpleName().equals(constantName)); - if (!alreadyExists) { - String modifiers = CUSTOM_MODIFIERS; // this is "private static final" - String typeName = literal.getType() == null ? "Object" : literal.getType().toString(); - String fieldSource = modifiers + " " + typeName + " " + constantName + " = " + literal.getValueSource() + ";"; - newFieldSources.add(fieldSource); - } - } - if (newFieldSources.isEmpty()) { - return cd; - } - - String templateStr = String.join("\n", newFieldSources); - JavaTemplate template = JavaTemplate.builder(templateStr) - .contextSensitive() - .build(); - Cursor bodyCursor = new Cursor(getCursor(), cd.getBody()); - J.Block updatedBody = template.apply(bodyCursor, cd.getBody().getCoordinates().firstStatement()); - - return cd.withBody(updatedBody); + public @NlsRewrite.DisplayName String getDisplayName() { + return "Replace magic numbers with constants"; } + @Override + public @NlsRewrite.Description String getDescription() { + return "Replaces magic number literals in method bodies with named constants to improve code readability and maintainability. " + + "Magic numbers are replaced by private static final constants declared at the top of the class, following Sonar's java:S109 rule. " + + "The recipe does not create constants for literals that are already assigned to fields or variables, nor for typical non-magic numbers (such as 0, 1, or -1). " + + "Currently, only numeric primitive literals are handled; string and character literals are unaffected. " + + "If a constant for a value already exists, or the constant name would conflict with an existing symbol, the recipe will skip that value."; + } - @Override - public J visitLiteral(J.Literal literal, ExecutionContext ctx) { - Cursor cursor = getCursor(); - if(cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable){ - return super.visitLiteral(literal, ctx); - } + @Override + public TreeVisitor getVisitor() { + return new JavaVisitor() { + @Override + public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) { + J.ClassDeclaration cd = (J.ClassDeclaration) super.visitClassDeclaration(classDecl, ctx); + + List literals = new ArrayList<>(); + // COLLECT magic numbers not in variable initializers and not -1/0/1 + new JavaVisitor() { + @Override + public J visitLiteral(J.Literal literal, ExecutionContext ctx2) { + Cursor cursor = getCursor(); + if (!(cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable) + && !isIgnoredMagicNumber(literal)) { + literals.add(literal); + } + return literal; + } + }.visit(classDecl, ctx); + + List newFieldSources = new ArrayList<>(); + for (J.Literal literal : literals) { + String constantName = getStrValFromLiteral(literal); + boolean alreadyExists = cd.getBody().getStatements().stream() + .filter(J.VariableDeclarations.class::isInstance) + .map(J.VariableDeclarations.class::cast) + .flatMap(vars -> vars.getVariables().stream()) + .anyMatch(var -> var.getSimpleName().equals(constantName)); + if (!alreadyExists) { + String modifiers = CUSTOM_MODIFIERS; + String typeName = getTypeName(literal); + String fieldSource = modifiers + " " + typeName + " " + constantName + " = " + literal.getValueSource() + ";"; + newFieldSources.add(fieldSource); + } + } + if (newFieldSources.isEmpty()) { + return cd; + } + + String templateStr = String.join("\n", newFieldSources); + JavaTemplate template = JavaTemplate.builder(templateStr) + .contextSensitive() + .build(); + Cursor bodyCursor = new Cursor(getCursor(), cd.getBody()); + J.Block updatedBody = template.apply(bodyCursor, cd.getBody().getCoordinates().firstStatement()); + + return cd.withBody(updatedBody); + } + + @Override + public J visitLiteral(J.Literal literal, ExecutionContext ctx) { + Cursor cursor = getCursor(); + // Do NOT replace in variable/field initializers or for ignored numbers + if (cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable + || isIgnoredMagicNumber(literal)) { + return super.visitLiteral(literal, ctx); + } + String constantName = getStrValFromLiteral(literal); + if (constantName != null) { + JavaTemplate template = JavaTemplate.builder(constantName).build(); + return template.apply(getCursor(), literal.getCoordinates().replace()); + } + return super.visitLiteral(literal, ctx); + } + }; + } + + @Override + public Set getTags() { + return Collections.singleton("RSPEC-109"); + } + + @Override + public Duration getEstimatedEffortPerOccurrence() { + return Duration.ofSeconds(10); + } + + private String getStrValFromLiteral(J.Literal literal) { + String type = getTypeName(literal).toUpperCase(); String valueSource = literal.getValueSource(); - String constantName = getStrValFromLiteral(literal); // e.g., "DOUBLE_51_0" - if (constantName != null) { - JavaTemplate template = JavaTemplate.builder(constantName).build(); - return template.apply(getCursor(), literal.getCoordinates().replace()); + if (valueSource == null) return null; + if (valueSource.startsWith("-")) { + valueSource = "NEGATIVE_" + valueSource.substring(1); } - return super.visitLiteral(literal, ctx); - } - }; - } - - @Override - public Set getTags() { - return Collections.singleton("RSPEC-109"); - } - @Override - public Duration getEstimatedEffortPerOccurrence() { - return Duration.ofSeconds(10); - } - private String getStrValFromLiteral(J.Literal literal) { - return literal.getType().toString().toUpperCase() + "_" + literal.getValueSource().replace(".", "_").replace("-", "NEGATIVE_"); - } + return type + "_" + valueSource.replace(".", "_"); + } + + private String getTypeName(J.Literal literal) { + if (literal.getType() == null) return "Object"; + JavaType type = literal.getType(); + if (type instanceof JavaType.Primitive) { + return ((JavaType.Primitive) type).getKeyword(); + } + return type.toString(); // fallback + } + + private boolean isIgnoredMagicNumber(J.Literal literal) { + Object value = literal.getValue(); + if (value instanceof Number) { + double d = ((Number) value).doubleValue(); + // Only ignore -1, 0, 1 for all numeric types. + return d == -1.0 || d == 0.0 || d == 1.0; + } + return false; + } } From 85d0fa6e36b11ccdb31316f5c3932bf3f4bb85fc Mon Sep 17 00:00:00 2001 From: Bora Ciner Date: Sun, 2 Nov 2025 19:20:53 +0100 Subject: [PATCH 3/8] Update ReplaceMagicNumbersWithConstantsTest.java -1, 0, and 1 are not considered magic numbers. --- .../ReplaceMagicNumbersWithConstantsTest.java | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java b/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java index f1c0472dd7..7877685802 100644 --- a/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java @@ -97,6 +97,50 @@ public double calculateShippingCost(double orderTotal) { return DOUBLE_7_99; } } +} + """ + ) + ); + } + @DocumentExample + @Test + void assignMagicNumbersToConstantsM1_0_1_AreIgnoredTest() { + rewriteRun( + spec -> spec + .typeValidationOptions(TypeValidation.none()), + java( + """ + public class OrderProcessor { + private static final double myVariable = 99.99; + public double calculateShippingCost(double orderTotal) { + int localVar0 = 0; + orderTotal = localVar0 - 1; + orderTotal = localVar0 + 0; + orderTotal = localVar0 + 1; + if (orderTotal < 51.0) { + return 7.99; + } else { + return 0.0; + } + } + } + """, + """ +public class OrderProcessor { + private static final double DOUBLE_51_0 = 51.0; + private static final double DOUBLE_7_99 = 7.99; + private static final double myVariable = 99.99; + public double calculateShippingCost(double orderTotal) { + int localVar0 = 0; + orderTotal = localVar0 - 1; + orderTotal = localVar0 + 0; + orderTotal = localVar0 + 1; + if (orderTotal < DOUBLE_51_0) { + return DOUBLE_7_99; + } else { + return 0.0; + } + } } """ ) From 9c9ed78481f0862253761254f0d5d15a5a968de5 Mon Sep 17 00:00:00 2001 From: Bora Ciner Date: Sun, 2 Nov 2025 19:39:58 +0100 Subject: [PATCH 4/8] Update ReplaceMagicNumbersWithConstantsTest.java test fix --- .../ReplaceMagicNumbersWithConstantsTest.java | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java b/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java index 7877685802..738a11c90a 100644 --- a/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java @@ -1,3 +1,18 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.openrewrite.staticanalysis; /* * Copyright 2024 the original author or authors. @@ -55,7 +70,6 @@ public class OrderProcessor { private static final int INT_10 = 10; private static final double DOUBLE_51_0 = 51.0; private static final double DOUBLE_7_99 = 7.99; - private static final double DOUBLE_0_0 = 0.0; private static final double myVariable = 99.99; public double calculateShippingCost(double orderTotal) { int localVar = 5; @@ -63,7 +77,7 @@ public double calculateShippingCost(double orderTotal) { if (orderTotal < DOUBLE_51_0) { return DOUBLE_7_99; } else { - return DOUBLE_0_0; + return 0.0; } } } From 31bbeb7ac9ac022929783a8b7242f83f0c47b6ff Mon Sep 17 00:00:00 2001 From: Bora Ciner Date: Mon, 3 Nov 2025 07:15:41 +0100 Subject: [PATCH 5/8] Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../ReplaceMagicNumbersWithConstants.java | 40 +++++++++++-------- .../ReplaceMagicNumbersWithConstantsTest.java | 5 +-- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java b/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java index 90223c4e4d..efb94c9c01 100644 --- a/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java +++ b/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java @@ -15,7 +15,10 @@ */ package org.openrewrite.staticanalysis; -import org.openrewrite.*; +import org.openrewrite.Cursor; +import org.openrewrite.ExecutionContext; +import org.openrewrite.Recipe; +import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.tree.J; @@ -23,7 +26,8 @@ import java.time.Duration; import java.util.ArrayList; -import java.util.Collections; +import static java.util.Collections.singleton; + import java.util.List; import java.util.Set; @@ -37,13 +41,13 @@ public class ReplaceMagicNumbersWithConstants extends Recipe { private static final String CUSTOM_MODIFIERS = "private static final"; @Override - public @NlsRewrite.DisplayName String getDisplayName() { - return "Replace magic numbers with constants"; - } - - @Override - public @NlsRewrite.Description String getDescription() { - return "Replaces magic number literals in method bodies with named constants to improve code readability and maintainability. " + public String getDisplayName() { + public String getDescription() { + return "Replaces magic number literals in method bodies with named constants to improve code readability and maintainability. " + + "Magic numbers are replaced by private static final constants declared at the top of the class, following Sonar's java:S109 rule. " + + "The recipe does not create constants for literals that are already assigned to fields or variables, nor for typical non-magic numbers (such as 0, 1, or -1). " + + "Currently, only numeric primitive literals are handled; string and character literals are unaffected. " + + "If a constant for a value already exists, or the constant name would conflict with an existing symbol, the recipe will skip that value."; + "Magic numbers are replaced by private static final constants declared at the top of the class, following Sonar's java:S109 rule. " + "The recipe does not create constants for literals that are already assigned to fields or variables, nor for typical non-magic numbers (such as 0, 1, or -1). " + "Currently, only numeric primitive literals are handled; string and character literals are unaffected. " @@ -63,8 +67,8 @@ public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ct @Override public J visitLiteral(J.Literal literal, ExecutionContext ctx2) { Cursor cursor = getCursor(); - if (!(cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable) - && !isIgnoredMagicNumber(literal)) { + if (!(cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable) && + !isIgnoredMagicNumber(literal)) { literals.add(literal); } return literal; @@ -104,8 +108,8 @@ public J visitLiteral(J.Literal literal, ExecutionContext ctx2) { public J visitLiteral(J.Literal literal, ExecutionContext ctx) { Cursor cursor = getCursor(); // Do NOT replace in variable/field initializers or for ignored numbers - if (cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable - || isIgnoredMagicNumber(literal)) { + if (cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable || + isIgnoredMagicNumber(literal)) { return super.visitLiteral(literal, ctx); } String constantName = getStrValFromLiteral(literal); @@ -120,7 +124,7 @@ public J visitLiteral(J.Literal literal, ExecutionContext ctx) { @Override public Set getTags() { - return Collections.singleton("RSPEC-109"); + return singleton("RSPEC-109"); } @Override @@ -131,7 +135,9 @@ public Duration getEstimatedEffortPerOccurrence() { private String getStrValFromLiteral(J.Literal literal) { String type = getTypeName(literal).toUpperCase(); String valueSource = literal.getValueSource(); - if (valueSource == null) return null; + if (valueSource == null) { + return null; + } if (valueSource.startsWith("-")) { valueSource = "NEGATIVE_" + valueSource.substring(1); } @@ -139,7 +145,9 @@ private String getStrValFromLiteral(J.Literal literal) { } private String getTypeName(J.Literal literal) { - if (literal.getType() == null) return "Object"; + if (literal.getType() == null) { + return "Object"; + } JavaType type = literal.getType(); if (type instanceof JavaType.Primitive) { return ((JavaType.Primitive) type).getKeyword(); diff --git a/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java b/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java index 738a11c90a..78ebd1f05f 100644 --- a/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java @@ -31,8 +31,7 @@ */ import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample; -import org.openrewrite.java.JavaParser; -import org.openrewrite.test.RecipeSpec; +class ReplaceMagicNumbersWithConstantsTest implements RewriteTest { import org.openrewrite.test.RewriteTest; import org.openrewrite.test.TypeValidation; @@ -86,7 +85,6 @@ public double calculateShippingCost(double orderTotal) { ); } - @DocumentExample @Test void assignMagicNumbersToConstantsBasicTest() { rewriteRun( @@ -116,7 +114,6 @@ public double calculateShippingCost(double orderTotal) { ) ); } - @DocumentExample @Test void assignMagicNumbersToConstantsM1_0_1_AreIgnoredTest() { rewriteRun( From 3daf20c68b4831e9902832de0270222b1eba8bfa Mon Sep 17 00:00:00 2001 From: Bora Ciner Date: Mon, 3 Nov 2025 11:05:32 +0100 Subject: [PATCH 6/8] Update ReplaceMagicNumbersWithConstants.java --- .../ReplaceMagicNumbersWithConstants.java | 118 +++++++++++------- 1 file changed, 76 insertions(+), 42 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java b/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java index efb94c9c01..8f855d45ff 100644 --- a/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java +++ b/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java @@ -15,21 +15,14 @@ */ package org.openrewrite.staticanalysis; -import org.openrewrite.Cursor; -import org.openrewrite.ExecutionContext; -import org.openrewrite.Recipe; -import org.openrewrite.TreeVisitor; +import org.openrewrite.*; import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; import java.time.Duration; -import java.util.ArrayList; -import static java.util.Collections.singleton; - -import java.util.List; -import java.util.Set; +import java.util.*; /** * This recipe replaces magic number literals in method bodies with named constants following the Sonar java:S109 rule. @@ -41,13 +34,13 @@ public class ReplaceMagicNumbersWithConstants extends Recipe { private static final String CUSTOM_MODIFIERS = "private static final"; @Override - public String getDisplayName() { - public String getDescription() { - return "Replaces magic number literals in method bodies with named constants to improve code readability and maintainability. " + - "Magic numbers are replaced by private static final constants declared at the top of the class, following Sonar's java:S109 rule. " + - "The recipe does not create constants for literals that are already assigned to fields or variables, nor for typical non-magic numbers (such as 0, 1, or -1). " + - "Currently, only numeric primitive literals are handled; string and character literals are unaffected. " + - "If a constant for a value already exists, or the constant name would conflict with an existing symbol, the recipe will skip that value."; + public @NlsRewrite.DisplayName String getDisplayName() { + return "Replace magic numbers with constants"; + } + + @Override + public @NlsRewrite.Description String getDescription() { + return "Replaces magic number literals in method bodies with named constants to improve code readability and maintainability. " + "Magic numbers are replaced by private static final constants declared at the top of the class, following Sonar's java:S109 rule. " + "The recipe does not create constants for literals that are already assigned to fields or variables, nor for typical non-magic numbers (such as 0, 1, or -1). " + "Currently, only numeric primitive literals are handled; string and character literals are unaffected. " @@ -62,32 +55,48 @@ public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ct J.ClassDeclaration cd = (J.ClassDeclaration) super.visitClassDeclaration(classDecl, ctx); List literals = new ArrayList<>(); - // COLLECT magic numbers not in variable initializers and not -1/0/1 + // COLLECT magic numbers not in variable initializers and not -1/0/1, ONLY numbers new JavaVisitor() { @Override public J visitLiteral(J.Literal literal, ExecutionContext ctx2) { - Cursor cursor = getCursor(); - if (!(cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable) && - !isIgnoredMagicNumber(literal)) { - literals.add(literal); + try { + Object value = literal.getValue(); + if (!(value instanceof Number)) { + return literal; + } + Cursor cursor = getCursor(); + if (!(cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable) + && !isIgnoredMagicNumber(literal)) { + literals.add(literal); + } + return literal; + } catch (Exception e) { + System.out.println("Exception in visitLiteral 1: " + e.getMessage() + e.getStackTrace() + e); + return literal; } - return literal; } }.visit(classDecl, ctx); + // Deduplicate by constant name! List newFieldSources = new ArrayList<>(); + Set alreadyCreated = new HashSet<>(); for (J.Literal literal : literals) { String constantName = getStrValFromLiteral(literal); - boolean alreadyExists = cd.getBody().getStatements().stream() + + if (alreadyCreated.contains(constantName)) { + continue; + } + boolean existsInCode = cd.getBody().getStatements().stream() .filter(J.VariableDeclarations.class::isInstance) .map(J.VariableDeclarations.class::cast) .flatMap(vars -> vars.getVariables().stream()) .anyMatch(var -> var.getSimpleName().equals(constantName)); - if (!alreadyExists) { + if (!existsInCode) { String modifiers = CUSTOM_MODIFIERS; String typeName = getTypeName(literal); String fieldSource = modifiers + " " + typeName + " " + constantName + " = " + literal.getValueSource() + ";"; newFieldSources.add(fieldSource); + alreadyCreated.add(constantName); } } if (newFieldSources.isEmpty()) { @@ -106,25 +115,54 @@ public J visitLiteral(J.Literal literal, ExecutionContext ctx2) { @Override public J visitLiteral(J.Literal literal, ExecutionContext ctx) { - Cursor cursor = getCursor(); - // Do NOT replace in variable/field initializers or for ignored numbers - if (cursor.getParent().getParent().getValue() instanceof J.VariableDeclarations.NamedVariable || - isIgnoredMagicNumber(literal)) { + try { + Object value = literal.getValue(); + if (!(value instanceof Number)) { + return literal; + } + Cursor cursor = getCursor(); + Cursor parent = cursor != null ? cursor.getParent() : null; + Cursor grandparent = parent != null ? parent.getParent() : null; + + // Do NOT replace in variable/field initializers or for ignored numbers + if ((grandparent != null && + grandparent.getValue() instanceof J.VariableDeclarations.NamedVariable) + || isIgnoredMagicNumber(literal)) { + return super.visitLiteral(literal, ctx); + } + String constantName = getStrValFromLiteral(literal); + if (constantName != null) { + JavaTemplate template = JavaTemplate.builder(constantName).build(); + return template.apply(getCursor(), literal.getCoordinates().replace()); + } return super.visitLiteral(literal, ctx); + } catch (Exception e) { + System.err.println("Exception in visitLiteral 3:"); + System.err.println(" Exception Type: " + e.getClass().getName()); + System.err.println(" Message: " + e.getMessage()); + System.err.println(" Literal: " + literal); + System.err.println(" Literal value: " + literal.getValueSource()); + System.err.println(" Cursor path: " + printCursorPath(getCursor())); + e.printStackTrace(System.err); + return literal; } - String constantName = getStrValFromLiteral(literal); - if (constantName != null) { - JavaTemplate template = JavaTemplate.builder(constantName).build(); - return template.apply(getCursor(), literal.getCoordinates().replace()); - } - return super.visitLiteral(literal, ctx); } }; } - + private String printCursorPath(Cursor cursor) { + StringBuilder sb = new StringBuilder(); + while (cursor != null) { + Object val = cursor.getValue(); + sb.append(val == null ? "null" : val.getClass().getSimpleName()); + sb.append(" > "); + cursor = cursor.getParent(); + } + sb.append("ROOT"); + return sb.toString(); + } @Override public Set getTags() { - return singleton("RSPEC-109"); + return Collections.singleton("RSPEC-109"); } @Override @@ -135,9 +173,7 @@ public Duration getEstimatedEffortPerOccurrence() { private String getStrValFromLiteral(J.Literal literal) { String type = getTypeName(literal).toUpperCase(); String valueSource = literal.getValueSource(); - if (valueSource == null) { - return null; - } + if (valueSource == null) return null; if (valueSource.startsWith("-")) { valueSource = "NEGATIVE_" + valueSource.substring(1); } @@ -145,9 +181,7 @@ private String getStrValFromLiteral(J.Literal literal) { } private String getTypeName(J.Literal literal) { - if (literal.getType() == null) { - return "Object"; - } + if (literal.getType() == null) return "Object"; JavaType type = literal.getType(); if (type instanceof JavaType.Primitive) { return ((JavaType.Primitive) type).getKeyword(); From 836ee9a3b6a1a291f3ca6a5fb933505fe22bfbd9 Mon Sep 17 00:00:00 2001 From: Bora Ciner Date: Mon, 3 Nov 2025 11:05:49 +0100 Subject: [PATCH 7/8] Update ReplaceMagicNumbersWithConstantsTest.java --- .../ReplaceMagicNumbersWithConstantsTest.java | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java b/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java index 78ebd1f05f..0dc3ff9fac 100644 --- a/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstantsTest.java @@ -31,7 +31,8 @@ */ import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample; -class ReplaceMagicNumbersWithConstantsTest implements RewriteTest { +import org.openrewrite.java.JavaParser; +import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; import org.openrewrite.test.TypeValidation; @@ -56,6 +57,15 @@ public class OrderProcessor { public double calculateShippingCost(double orderTotal) { int localVar = 5; orderTotal = localVar + 10; + if (orderTotal < 51.0) { + System.out.println("Order total is less than 51.0"); + } + if (orderTotal < 51.0) { + System.out.println("Order total is less than 51.0"); + } + if (orderTotal < 51.0) { + System.out.println("Order total is less than 51.0"); + } if (orderTotal < 51.0) { return 7.99; } else { @@ -73,6 +83,15 @@ public class OrderProcessor { public double calculateShippingCost(double orderTotal) { int localVar = 5; orderTotal = localVar + INT_10; + if (orderTotal < DOUBLE_51_0) { + System.out.println("Order total is less than 51.0"); + } + if (orderTotal < DOUBLE_51_0) { + System.out.println("Order total is less than 51.0"); + } + if (orderTotal < DOUBLE_51_0) { + System.out.println("Order total is less than 51.0"); + } if (orderTotal < DOUBLE_51_0) { return DOUBLE_7_99; } else { @@ -85,6 +104,7 @@ public double calculateShippingCost(double orderTotal) { ); } + @DocumentExample @Test void assignMagicNumbersToConstantsBasicTest() { rewriteRun( @@ -114,6 +134,7 @@ public double calculateShippingCost(double orderTotal) { ) ); } + @DocumentExample @Test void assignMagicNumbersToConstantsM1_0_1_AreIgnoredTest() { rewriteRun( From 74e2ae00e194555235899b2464a688881237a101 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 3 Nov 2025 14:23:07 +0100 Subject: [PATCH 8/8] Update src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java --- .../staticanalysis/ReplaceMagicNumbersWithConstants.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java b/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java index 8f855d45ff..5e332c52aa 100644 --- a/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java +++ b/src/main/java/org/openrewrite/staticanalysis/ReplaceMagicNumbersWithConstants.java @@ -137,13 +137,6 @@ public J visitLiteral(J.Literal literal, ExecutionContext ctx) { } return super.visitLiteral(literal, ctx); } catch (Exception e) { - System.err.println("Exception in visitLiteral 3:"); - System.err.println(" Exception Type: " + e.getClass().getName()); - System.err.println(" Message: " + e.getMessage()); - System.err.println(" Literal: " + literal); - System.err.println(" Literal value: " + literal.getValueSource()); - System.err.println(" Cursor path: " + printCursorPath(getCursor())); - e.printStackTrace(System.err); return literal; } }