mbien commented on code in PR #5332:
URL: https://github.com/apache/netbeans/pull/5332#discussion_r1103562622
##########
enterprise/maven.jaxws/src/org/netbeans/modules/maven/jaxws/MavenJaxWsSupportProvider.java:
##########
@@ -381,15 +381,14 @@ private void doUpdateJaxWs( Map<String, ServiceInfo>
newServices ) {
commonServices.add(key);
}
}
- for (String key : commonServices) {
- oldServices.remove(key);
- newServices.remove(key);
- }
+
+ oldServices.keySet().removeAll(commonServices);
+ newServices.keySet().removeAll(commonServices);
Review Comment:
i think the original version is better:
- it iterates only once over all services
- it doesn't create two keySets which maps create lazily on demand
##########
apisupport/apisupport.ant/test/unit/src/org/netbeans/modules/apisupport/project/ui/customizer/SingleModulePropertiesTest.java:
##########
@@ -187,10 +187,11 @@ public void testThatPropertiesAreRefreshedInOSGiMode()
throws Exception {
Manifest mf = new Manifest(new
FileInputStream(props.getManifestFile()));
for (Object s : mf.getMainAttributes().keySet()) {
- if (s.toString().equals("OpenIDE-Module-Layer")) {
+ String s1 = s.toString();
+ if (s1.equals("OpenIDE-Module-Layer")) {
continue;
}
- if (s.toString().startsWith("OpenIDE")) {
+ if (s1.startsWith("OpenIDE")) {
Review Comment:
this doesn't have anything to do with the PR description. Its also test code
and the PR is large enough -> please revert
##########
platform/openide.util/test/unit/src/org/openide/util/NbCollectionsTest.java:
##########
@@ -295,20 +295,20 @@ public void testCheckedMapByFilter() throws Exception {
assertEquals(2, m2.entrySet().size());
assertEquals(2, m2.keySet().size());
assertEquals(2, m2.values().size());
- assertTrue(m2.keySet().contains(1));
- assertFalse(m2.keySet().contains(5));
+ assertTrue(m2.containsKey(1));
+ assertFalse(m2.containsKey(5));
try {
- m2.keySet().contains("three");
+ m2.containsKey("three");
fail();
} catch (ClassCastException e) {/*OK*/}
- assertFalse(m2.keySet().contains(4));
- assertTrue(m2.values().contains("one"));
- assertFalse(m2.values().contains("five"));
+ assertFalse(m2.containsKey(4));
+ assertTrue(m2.containsValue("one"));
+ assertFalse(m2.containsValue("five"));
try {
- m2.values().contains(4);
+ m2.containsValue(4);
fail();
} catch (ClassCastException e) {/*OK*/}
- assertFalse(m2.values().contains("three"));
+ assertFalse(m2.containsValue("three"));
Review Comment:
this is a NbCollectionsTest! Lets revert this.
##########
nbi/engine/src/org/netbeans/installer/product/Registry.java:
##########
@@ -21,13 +21,7 @@
import java.io.File;
import java.io.IOException;
-import java.util.ArrayList;
-import java.util.HashSet;
-import java.util.LinkedList;
-import java.util.List;
-import java.util.Properties;
-import java.util.Queue;
-import java.util.Set;
+import java.util.*;
Review Comment:
i think we try to avoid star imports.
##########
webcommon/lib.v8debug/src/org/netbeans/lib/v8debug/client/cmdline/V8Debug.java:
##########
@@ -1020,12 +1020,12 @@ private String printStr(V8Value value) {
if (o.getProperties() != null) {
Map<String, V8Object.Property> properties =
o.getProperties();
String newLine = System.getProperty("line.separator");
- for (String propName : properties.keySet()) {
+ for (Map.Entry<String, V8Object.Property> entry :
properties.entrySet()) {
sb.append(newLine);
sb.append(" ");
- sb.append(propName);
+ sb.append(entry.getKey());
Review Comment:
use `propName` variable for key like it was before
##########
apisupport/timers/src/org/netbeans/modules/timers/TimeComponentPanel.java:
##########
@@ -439,18 +440,18 @@ public void stateChanged(ChangeEvent arg0) {
inner.paintImmediately(inner.getBounds());
}
});
- Map/*<Object,Path>*/ traces = LiveReferences.fromRoots(objects, null,
bar.getModel());
+ Map<Object, Path> traces = LiveReferences.fromRoots(objects, null,
bar.getModel());
if (traces == null) {
return "";
}
StringBuffer sb = new StringBuffer();
-
- for (Object inst : traces.keySet()) {
+
+ traces.forEach((inst, v) -> {
sb.append(inst);
sb.append(":\n"); // NOI18N
sb.append(traces.get(inst));
sb.append("\n\n"); // NOI18N
- }
+ });
Review Comment:
this is half finished. You are not using the value `v` here. You could also
rename `v` to `path`.
##########
extide/gradle.editor/src/org/netbeans/modules/gradle/editor/hints/GradleHintsProvider.java:
##########
@@ -130,10 +130,14 @@ void updateProjectProblems() {
return;
}
List<ErrorDescription> hints = new ArrayList<>();
- for (LineDocument doc : documentReports.keySet()) {
+ for (Map.Entry<LineDocument, List<GradleReport>> it :
documentReports.entrySet()) {
+ LineDocument doc = it.getKey();
+
doc.render(() -> {
- for (GradleReport r : documentReports.get(doc)) {
-
hints.add(ErrorDescriptionFactory.createErrorDescription(Severity.ERROR,
r.formatReportForHintOrProblem(false, null), doc, r.getLine()));
+ for (GradleReport r : it.getValue()) {
+
hints.add(ErrorDescriptionFactory.createErrorDescription(Severity.ERROR,
+ r.formatReportForHintOrProblem(false, null), doc,
+ r.getLine()));
}
Review Comment:
revert formatting of the loop body to the original please.
##########
enterprise/websvc.core/src/org/netbeans/modules/websvc/core/jaxws/projects/JavaEEWSOpenHook.java:
##########
@@ -217,15 +217,13 @@ private void updateWsModel(Map<String, String> services){
}
}
- for (String key : commonServices) {
- oldServices.remove(key);
- services.remove(key);
- }
+ oldServices.keySet().removeAll(commonServices);
+ services.keySet().removeAll(commonServices);
Review Comment:
same here, original version is likely better
##########
webcommon/javascript2.editor/test/unit/src/org/netbeans/modules/javascript2/editor/formatter/JsFormatterTestBase.java:
##########
@@ -139,19 +139,22 @@ protected void reformatFileContents(String file,
Map<String, Object> options, St
prefs.clear();
prefs = CodeStylePreferences.get(doc).getPreferences();
- for (String option : options.keySet()) {
- assertTrue(FmtOptions.tabSize.equals(option)
- || FmtOptions.expandTabToSpaces.equals(option)
- || FmtOptions.indentSize.equals(option)
- || FmtOptions.rightMargin.equals(option)
- || prefs.get(option, null) == null);
- Object value = options.get(option);
+ for (Map.Entry<String, Object> entry : options.entrySet()) {
+ String key = entry.getKey();
Review Comment:
rename to `option` like it was before.
##########
webcommon/javascript2.editor/test/unit/src/org/netbeans/modules/javascript2/editor/formatter/PackageFormatterTest.java:
##########
@@ -109,19 +109,23 @@ protected void reformatFileContents(String file,
Map<String, Object> options, St
prefs.clear();
prefs = CodeStylePreferences.get(doc).getPreferences();
- for (String option : options.keySet()) {
- assertTrue(FmtOptions.tabSize.equals(option)
- || FmtOptions.expandTabToSpaces.equals(option)
- || FmtOptions.indentSize.equals(option)
- || FmtOptions.rightMargin.equals(option)
- || prefs.get(option, null) == null);
- Object value = options.get(option);
+ for (Map.Entry<String, Object> entry : options.entrySet()) {
+ String key = entry.getKey();
Review Comment:
same here -> rename to option
##########
enterprise/websvc.rest/src/org/netbeans/modules/websvc/rest/client/ClientGenerationStrategy.java:
##########
@@ -492,11 +492,15 @@ protected void addQueryParams(TreeMaker maker, HttpParams
httpParams,
commentBuffer.append("<LI>"+otherParam+" [OPTIONAL]\n");
//NOI18N
}
// add default params
- Map<String,String> defaultParams =
httpParams.getDefaultQueryParams();
- for (String key : defaultParams.keySet()) {
- commentBuffer.append("<LI>"+key+" [OPTIONAL, DEFAULT
VALUE: \""+
- defaultParams.get(key)+"\"]\n"); //NOI18N
- }
+ Map<String, String> defaultParams =
httpParams.getDefaultQueryParams();
+ defaultParams.forEach((k, v) -> {
+ commentBuffer
+ .append("<LI>")
+ .append(k)
+ .append(" [OPTIONAL, DEFAULT VALUE: \"")
+ .append(v)
+ .append("\"]\n"); //NOI18N
+ });
Review Comment:
can you revert the `append` part? Concatenation of short Strings with `+` is
already as fast as it gets.
```java
commentBuffer.append("<LI>"+k+" [OPTIONAL, DEFAULT VALUE:
\""+v+"\"]\n"); //NOI18N
```
##########
platform/core.windows/src/org/netbeans/core/windows/view/ViewHierarchy.java:
##########
@@ -998,8 +999,8 @@ private static String createIndentString(int indent) {
private String dumpAccessors() {
StringBuffer sb = new StringBuffer();
- for(ElementAccessor accessor: accessor2view.keySet()) {
- sb.append("accessor="+accessor +
"\tview="+accessor2view.get(accessor) + "\n"); // NOI18N
+ for (Map.Entry<ElementAccessor, ViewElement> entry :
accessor2view.entrySet()) {
+
sb.append("accessor=").append(entry.getKey()).append("\tview=").append(entry.getValue()).append("\n");
// NOI18N
Review Comment:
revert `append()` change. `+` is fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
