mbien commented on code in PR #4454:
URL: https://github.com/apache/netbeans/pull/4454#discussion_r933973890
##########
enterprise/tomcat5/src/org/netbeans/modules/tomcat5/j2ee/JaxWsStack.java:
##########
@@ -169,21 +169,17 @@ private String[] getDetectedMetroLibs() {
private boolean isKeystore() {
- if (new File(catalinaHome, KEYSTORE_LOCATION).exists()) return true;
- else return false;
+ return new File(catalinaHome, KEYSTORE_LOCATION).exists();
Review Comment:
nitpick: could be `Files.exists(Paths.get(catalinaHome, KEYSTORE_LOCATION))`
but we could do this later, going to add this pattern to the list here:
https://github.com/mbien/jackpot-inspections :)
##########
enterprise/tomcat5/src/org/netbeans/modules/tomcat5/TomcatFactory.java:
##########
@@ -227,15 +227,7 @@ public static String getTomcatVersionString(File
catalinaHome) throws IllegalSta
return version.substring(idx + 1);
}
throw new IllegalStateException("Cannot identify the version of
the server."); // NOI18N
- } catch (MalformedURLException e) {
- throw new IllegalStateException(e);
- } catch (ClassNotFoundException e) {
- throw new IllegalStateException(e);
- } catch (NoSuchMethodException e) {
- throw new IllegalStateException(e);
- } catch (InvocationTargetException e) {
- throw new IllegalStateException(e);
- } catch (IllegalAccessException e) {
+ } catch (MalformedURLException | ClassNotFoundException |
NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
Review Comment:
nitpick: this could be shortened by using `ReflectiveOperationException` as
common super type of all reflection exceptions.
##########
enterprise/tomcat5/src/org/netbeans/modules/tomcat5/ui/wizard/AddInstanceIterator.java:
##########
@@ -224,14 +224,14 @@ private void checkStartupScript(File homeDir) {
FileUtil.copyFile(
FileUtil.toFileObject(new
File(bundledHome, "bin/" + CATALINA)), // NOI18N
FileUtil.toFileObject(new File(homeDir,
"bin")), // NOI18N
- CATALINA.substring(0,
CATALINA.indexOf(".")) // NOI18N
+ CATALINA.substring(0,
CATALINA.indexOf('.')) // NOI18N
Review Comment:
`indexOf` is the last remaining String method which has the char variant
outperforming the single-char-string variant (under some circumstances).
But this is more of a performance bug of the JDK/JVM and highly depends of
the length of the string which is being checked (String variant can be
sometimes faster! The correct behavior would be identical performance).
I don't think we should do those kind of refactoring since:
- firstly, they should perform the same if the JVM does its job correctly,
- secondly, right now, String going to outperform the char variant in some
situations, so its a de-optimization ;)
- thirdly, the difference is so tiny that it would only show up in
measurements when used in inner loops without any other bottle necks
We should probably add a note to the description of the code inspection that
it is a bit outdated (just like when IDEs suggested the wrong version of
toArray() for so long).
I tried to fix it in the JDK but ended up running out of time when I noticed
that the initial assumption that char variants are faster was wrong due to
another performance bug:
https://github.com/openjdk/jdk/pull/6509
https://github.com/openjdk/jdk/pull/6509#issuecomment-980681069 (string
variant faster than char bug)
next winter maybe
--
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
