Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary boxing #541

Merged
merged 2 commits into from
Nov 11, 2019
Merged

Remove unnecessary boxing #541

merged 2 commits into from
Nov 11, 2019

Conversation

vlsi
Copy link
Collaborator

@vlsi vlsi commented Oct 18, 2019

Motivation and Context

Removal of boxing and unboxing makes the code easier to read in most of the cases.

The replacements were performed automatically by IDEA.

@FSchumacher
Copy link
Contributor

FSchumacher commented Oct 19, 2019 via email

@vlsi
Copy link
Collaborator Author

vlsi commented Oct 19, 2019

Sample from JMeterToolbar:

        buttonStates.put(ActionNames.REMOTE_START_ALL, Boolean.valueOf(!started));
        buttonStates.put(ActionNames.REMOTE_STOP_ALL, Boolean.valueOf(started));
        buttonStates.put(ActionNames.REMOTE_SHUT_ALL, Boolean.valueOf(started));

"valueof" adds nothing here, and it triggered the refactoring.

Note: in certain cases, the change is a performance improvement (when Double.valueOf was used instead of Double.parseDouble)

ham1
ham1 reviewed Oct 20, 2019
View reviewed changes
src/core/src/main/java/org/apache/jmeter/control/LoopController.java
Comment on lines 85 to 86
// in this case as resetLoopCount will not be called,
// it leads to no further evaluations if we don't evaluate, see BUG 56276
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth fixing this indentation (or moving it to a form which doesn't require constant manual formatting?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be nice to use consistent spacing around == provided Spotless or something like that can auto-format the code.

Copy link
Contributor

@ham1 ham1 Oct 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was referring to the block of // comments, but now you mention it, the spacing around == should at least be consistently wrong or correct ;)

Copy link
Collaborator Author

@vlsi vlsi Oct 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. As discussed earlier, there's no point in maintaining horizontal alignment.
Frankly speaking, I don't like "line comments within expression", and it would probably look better if the comment is placed before if or if nbLoops ==0 was extracted to a separate method.

I'm not sure it makes sense to manually inspect this change and fix spacing around == because it would take time and it add no value.
I would rather spend the time on implementing Async HTTP client rather than dealing with spaces.

src/core/src/main/java/org/apache/jmeter/gui/util/PowerTableModel.java
Byte.valueOf(Byte.MIN_VALUE), Short.valueOf(Short.MIN_VALUE)));
private static final List<Object> DEFAULT_ARGS = Collections.unmodifiableList(Arrays.asList("", 0,
0L, Boolean.FALSE, 0F, 0D, ' ',
Byte.MIN_VALUE, Short.MIN_VALUE));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably be better formatted now the arguments are much shorter e.g.

 Collections.unmodifiableList(Arrays.asList(
        "", 0, 0L, false, 0F, 0D, ' ', Byte.MIN_VALUE, Short.MIN_VALUE));

src/core/src/main/java/org/apache/jmeter/report/core/CsvSampleReader.java
Comment on lines +203 to 204
row + 2, file, data.length, columnCount,
String.join(", ", data));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Join these two short lines?

src/core/src/main/java/org/apache/jmeter/report/processor/RequestsSummaryConsumer.java
result.setResult("KoPercent", new ValueResultData(Double.valueOf((double) errorCount
* 100 / count)));
result.setResult("KoPercent", new ValueResultData((double) errorCount
* 100 / count));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Join these lines?

src/core/src/main/java/org/apache/jmeter/report/processor/graph/AbstractGraphConsumer.java
Comment on lines +222 to +223
valueResult.setValue(Math.min((Double) valueResult.getValue(),
value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Join these lines here and for the change below in this file?

src/core/src/main/java/org/apache/jmeter/report/processor/graph/AbstractGraphConsumer.java
@@ -342,15 +340,15 @@ private void addKeyData(MapResultData result, String group, String series,
ListResultData coordResult = new ListResultData();
coordResult.addResult(new ValueResultData(value));
coordResult.addResult(new ValueResultData(
Double.valueOf(percentile)));
percentile));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Join lines? :)

@vlsi vlsi merged commit 1da45dd into apache:master Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ham1 ham1 ham1 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vlsi @FSchumacher @ham1