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.

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

PAYARA-3919 SonarQube - Iterating on entrySet() instead of keySet() when key and value are needed #4038

Merged
merged 1 commit into from Jul 19, 2019

Conversation

@dvmarcilio
Copy link
Contributor

commented Jun 12, 2019

This PR comprises only nucleus classes.

Iterating on a Map using entrySet(), when both key and value are needed, is more performant.

Fix a SonarQube violation of the rule: "entrySet()" should be iterated when both the key and value are needed

for (Serializable key : map.keySet()) {
result.put((String) key, (String) map.get(key));
for (Entry<Serializable, Serializable> entry : map.entrySet()) {
result.put((String) key, (String) entry.getValue());

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Jun 12, 2019

Contributor

Shouldn't this be entry.getKey instead of key?

@@ -144,9 +145,9 @@ public void addMetaData(Object o) {
}

public Object getMetaData(String className) {
for (Class c : metaData.keySet()) {
for (Entry<Class<? extends Object>, Object> entry : metaData.entrySet()) {
if (c.getName().equals(className)) {

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Jun 12, 2019

Contributor

This should be entry.getKey().getName() instead of c.getName() I think.

@jbee
Copy link
Contributor

left a comment

@svendiedrichsen pointed out a number of errors and there were some more I marked. Also I'd like to see the original key name preserved by doing something like KeyType originalKeyName = entry.getKey() before the key is used replacing all occurrences of entry.getKey() with originalKeyName again. In some cases this does not add much but in general there is some context in the key name which I'd like to preserve.

@@ -230,8 +231,8 @@ public synchronized void bootstrapNotifierList() {
}

private void executeTasks() {
for (String registeredTaskKey : registeredTasks.keySet()) {
HealthCheckTask registeredTask = registeredTasks.get(registeredTaskKey);
for (Entry<String, HealthCheckTask> entry : registeredTasks.entrySet()) {

This comment has been minimized.

Copy link
@jbee

jbee Jun 12, 2019

Contributor

This might even be a case where we could just iterate values directly. Otherwise there is a key replacement missing.

@Pandrex247 Pandrex247 changed the title SonarQube - Iterating on entrySet() instead of keySet() when key and value are needed PAYARA-3919 SonarQube - Iterating on entrySet() instead of keySet() when key and value are needed Jun 12, 2019

@dvmarcilio

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Thanks for reviewing @svendiedrichsen and @jbee. I will work on them. Also, I'm sorry for the review overhead, especially the non compiling cases in this PR.

I will fix them ASAP.

@jbee

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

@dvmarcilio any news on the fixes?

@dvmarcilio dvmarcilio force-pushed the dvmarcilio:entrySet_instead_of_keySet branch from 9e0fbe3 to 0979c16 Jul 18, 2019

@dvmarcilio

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

@jbee Thanks for the reminder!

I ended up fixing issues in all modules, not only on nucleus as initially.

I preserved the original key name in almost all cases, except when the original name was not descriptive (like key or c). In such cases, I didn't introduce a variable for the key, just kept it as entry.getKey().

I left out 4 cases where we could have iterated just on the values. I would like to submit fixes for that in the future if you don't mind.

@jbee

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

jenkins test please

1 similar comment
@jbee

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

jenkins test please

@dvmarcilio dvmarcilio force-pushed the dvmarcilio:entrySet_instead_of_keySet branch 2 times, most recently from 0d6105d to b1467ae Jul 18, 2019

@dvmarcilio

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

@jbee My bad! I missed some import java.util.* and should have referenced as Map.Entry instead of just Entry. It should be fine now.

@jbee

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

jenkins test please

@dvmarcilio dvmarcilio force-pushed the dvmarcilio:entrySet_instead_of_keySet branch from b1467ae to 8e76681 Jul 18, 2019

@dvmarcilio

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

@jbee This is starting to get embarrassing — very tricky scenarios to automate. Sorry!

This time my local build is passing, besides Minimal Distribution. (before it was not, but I thought it was something unrelated to the code). Next time I'll be sure only to submit after the build is successful.

I hope it is fine now.

@jbee

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

jenkins test please

@jbee
jbee approved these changes Jul 19, 2019
Copy link
Contributor

left a comment

Thanks @dvmarcilio, all look correct now.

@jbee

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

I left out 4 cases where we could have iterated just on the values. I would like to submit fixes for that in the future if you don't mind.

@dvmarcilio Your help is always welcome.

@jbee jbee merged commit 0b39eef into payara:master Jul 19, 2019

59 checks passed

Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/javaee-api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.