feature: Implemented ConsecutiveLiteralAppends#915
Conversation
| return false; | ||
| } | ||
|
|
||
| if (!(scope.get() instanceof MethodCallExpr) || !isAppendableScope(expression, scope)) { |
There was a problem hiding this comment.
code-snippet highlighting the typical concerned code would help ;).
…-consecutiveliteralappends
| @CompareMethods | ||
| public static class TwoIntegers { | ||
| public Object pre(StringBuilder builder) { | ||
| return builder.append(1).append(2); |
There was a problem hiding this comment.
What about :
builder.append(1).append(2);builder.append(123).append(456);builder.append(2147483647).append(1);builder.append(1).append(-2);builder.append(0x1).append(0x2);
Co-authored-by: Benoit Chatain Lacelle <benoit.chatain.lacelle@gmail.com>
| return false; | ||
| } | ||
|
|
||
| String argument = getStringValue(methodCall.getArgument(SMALLEST_SINGLE_DIGIT_NUMBER)); |
There was a problem hiding this comment.
I believe we really lack documentation to read through the main method (e.g. processExpression).
Typically, here, I have to:
- Scroll up to see what's
SMALLEST_SINGLE_DIGIT_NUMBER - Interprets we're parsing something like
.append(12)
....
WAIT NO.
SMALLEST_SINGLE_DIGIT_NUMBER is irrelevant here, it is just some unexpected refactoring of 0 into SMALLEST_SINGLE_DIGIT_NUMBER.
This is my whole point about documentation: having to interpret the code due to lack of comments, I end interpreting an irrelevant code, while it is correct code. Having a snippet like extract 'someString' from '.append("someString")' would have helped.
There was a problem hiding this comment.
This one was unintentional, I’ve restored the zeros.
As for the documentation, I prefer to avoid “what” comments unless they’re absolutely necessary. Instead, I’d rather extract intent into well-named helper methods (e.g. extractArgumentFromMethodCall() in this case), so the code is self-explanatory without additional comments.
I'll try to come up with a solution that I gladly commit and you gladly merge.
| @UnmodifiedMethod | ||
| public static class NonSingleDigitIntegers { | ||
| public Object pre(StringBuilder builder) { | ||
| return builder.append(123).append(456); |
There was a problem hiding this comment.
This case is UnmodifiedMethod. Is it for specific reason, or just it feels like a rare/not-very relevant case?
There was a problem hiding this comment.
No, I think I just didn't think about it, but thanks for pointing out, I'll try to make it work!
There was a problem hiding this comment.
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.
2 participants
No description provided.