feature: Implemented BigIntegerInstantiation by gbq6 · Pull Request #921 · solven-eu/cleanthat

gbq6 · 2025-09-30T20:01:38Z

No description provided.

gbq6 · 2025-09-30T20:54:36Z

@blacelle is there a way to test it against multiple java versions? In that case I would try to make a solution that handles TWO as well.

blacelle · 2025-10-01T07:25:04Z

+	}
+
+	/**
+	 * {@link BigIntegerInstantiation} may turn NumberFormatException into a BigInteger.


Could you provide a specific case?

blacelle · 2025-10-01T07:25:52Z

+
+	@Override
+	protected boolean processExpression(NodeAndSymbolSolver<Expression> expression) {
+		Expression node = expression.getNode();


Please add code-snippet through the steps, to know what's going on.

blacelle · 2025-10-01T07:27:38Z

+
+	private static final String NOTHING = "";
+
+	private static final String NUMERIC_LITERAL_SUFFIX_REGEX = "(?<=\\d)(\\.0?)?([ldfLDF])?$";


Please document regex. One should not have to parse the reges to know what's it achieving.

Supposing what's doing this regex, I feel it does not manage cases like: double d = 1.0e0;. I suggest either finding another way (doing a plain Double.parse maybe, and then comparing with given values?), or relying on a solid regex, with a solid reference (e.g., some stackoverflow with many votes).

blacelle · 2025-10-01T07:29:02Z

+
+	private static final String NUMERIC_LITERAL_SUFFIX_REGEX = "(?<=\\d)(\\.0?)?([ldfLDF])?$";
+
+	private static final Map<String, String> CONSTANTS = Map.of("0", "ZERO", "1", "ONE", "10", "TEN");


Is this a INTEGER_AS_STRING_TO_FIELD map? We need documentation not to have to re-interpret the whole thing to know how it work.

If my interpretation of INTEGER_AS_STRING_TO_FIELD is right, I wonder (not having interpreted the whole thing yet), how it will manage new BigDecimal(1.0D)

I'll add comments, but this map would be immediately understandable if the formatter did not inline it.

One rule I'd love, but I did not implement (or only partially), is turning Map.of(...) into ImmutableMap.builder().put(...).put(...). build() which will tend to have a single entry per LOC.

Still, it would not be immediately understandable. Typically, it would suggest it manage 0->ZERO but not 0D->ZERO. Which is unclear to me if it is true or not (given the rest of the code).

(I'm no believer in comments are just code pollution, the code should be 100% self-expressive. It should be true (in a perfect world), we should code in a way comments are general useless/pollution... but regularly, just code is not enough to express what's going on, what's tricky, what's our plan, what's our responsability ).

One rule I'd love, but I did not implement (or only partially), is turning Map.of(...) into ImmutableMap.builder().put(...).put(...). build() which will tend to have a single entry per LOC.

There is GuavaImmutableMapBuilderOverVarargs somewhere. It got stuck on managing types in <S, T> diamond operator.

(Which may be another case of partially migrated code, following our discussion in #170)

blacelle · 2025-10-01T07:31:55Z

+				|| argument.isDoubleLiteralExpr();
+	}
+
+	private static String getValue(LiteralStringValueExpr argument) {


This method feels dangerous
-> it needs dedicated unit-testing.
-> it can not be private.

I mean, we could have standard mutator test-case. But I feel, given this design, simpler to unit-test some cases with getValue unit-cases. As I'd like to like things like 1, 1.0, 1.0D, 1.0000, etc

Can I move it to a helper class? (If yes, where do you suggest?)

WHy a helper class? Isn't this relevant only in the context of BigIntegerInstanciation?

Also, you'll not like it (:D), but I lack documentation to know the responsability of this method (which is a pre-requisites to answer where would I suggest). And its resposnability shall not be defined by its implementation (which may be faulty). And a method name may not be eough to described precisaly the responsability.

It relies on a regex, which is itself quite non-expressive. (And a regex shall never be self-expressive, most of us do not how how to interpret precisaly and correctly a regex).

getAsStringRepresentingInteger feels like a poor naming, which might provide some hints about the responsability of this method.

I completely agree that the regex should be documented and am working on it.

I'm not sure if it's limited to BigInteger/BigDecimal, but I'll leave it here then - unless I can find a better way to parse the numbers and completely get rid of this.

I mean, my original idea was to call the BigInteger/BigDecimal constructors with the given value and compare the result with the available constants, but that doesn't feel clean either. But definitely would require less extra testing than the regex.

my original idea was to call the BigInteger/BigDecimal constructors with the given value and compare the result with the available constants, but that doesn't feel clean either.

I feel it a very good approach, and similar to my suggestion of relying on Double.parse.

But definitely would require less extra testing than the regex.

That's a huge argument in my perspective. In this project, it is difficult to foresee all actual edge-cases we'll have to digest. (Some say Java is a simple language... Well... I do not beleive so xD).

I'll update the implementation!

blacelle · 2025-10-01T07:33:11Z

+
+    @Override
+    public IJavaparserAstMutator getTransformer() {
+        return new BigIntegerInstantiation();


Generally speaking, I would expect this rule to also cover cases like BigDecimal.valueOf(1)

Me too, I just didn't have enough energy to implement that one as well, but will give it a try.

You could add additional test-case, with the @NotYetImplemented annotation. I feel it's a nice why to document future improvments, and what's not implemented yet.

~~I have adjusted it to support numbers, but only non-negatives yet. Planning to extend it with those as well, but in the meantime if you have any test cases that you suggest I'd appreciate it.'~~

Sorry, meant to write this on the other PR.

feature: Implemented BigIntegerInstantiation

b343d4f

gbq6 requested a review from blacelle as a code owner September 30, 2025 20:01

Extracted node

034a3bc

blacelle reviewed Oct 1, 2025

View reviewed changes

Comment thread ...c/main/java/eu/solven/cleanthat/engine/java/refactorer/mutators/BigIntegerInstantiation.java Outdated

blacelle reviewed Oct 1, 2025

View reviewed changes

Added test case

a6a4e2e

gbq6 marked this pull request as draft October 1, 2025 21:16

Adjusted implementation

d0fdb40


		private static final String NOTHING = "";

		private static final String NUMERIC_LITERAL_SUFFIX_REGEX = "(?<=\\d)(\\.0?)?([ldfLDF])?$";


		private static final String NUMERIC_LITERAL_SUFFIX_REGEX = "(?<=\\d)(\\.0?)?([ldfLDF])?$";

		private static final Map<String, String> CONSTANTS = Map.of("0", "ZERO", "1", "ONE", "10", "TEN");

Conversation

gbq6 commented Sep 30, 2025

Uh oh!

gbq6 commented Sep 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blacelle Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blacelle Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blacelle Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blacelle Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbq6 Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

blacelle Oct 1, 2025 •

edited

Loading

blacelle Oct 1, 2025 •

edited

Loading

blacelle Oct 1, 2025 •

edited

Loading

blacelle Oct 1, 2025 •

edited

Loading

gbq6 Oct 1, 2025 •

edited

Loading