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-4247 Cleanup sonar warnings from common/common-util #4293

Merged
merged 11 commits into from Nov 7, 2019

Conversation

@Cousjava
Copy link
Member

Cousjava commented Oct 31, 2019

Description

This is a bug fix

Resolves issues in sonarcloud from nucleus/common/common-util

Testing

Test suites executed

  • Payara Microprofile TCKs Runner
  • Cargo Tracker

Testing Environment

Zulu JDK 1.8_222 on Ubuntu 19.04 with Maven 3.6.0
Zulu JDK 11.05 on Ubuntu 19.04 with Maven 3.6.0

@Cousjava Cousjava added this to the 5.194 milestone Oct 31, 2019
@Cousjava

This comment has been minimized.

Copy link
Member Author

Cousjava commented Oct 31, 2019

Jenkins test please

Copy link
Contributor

jbee left a comment

Nice one @Cousjava!
There was only one change that looked odd to me with the file endings replacement. Did not try to hard to understand it though.
Other than that only some code style things that might be my personal preference :D

@@ -180,18 +184,12 @@ public static String replace(String s, String token, String replace) {
public static boolean isRelativePath(String path) {

This comment has been minimized.

Copy link
@jbee

jbee Oct 31, 2019

Contributor

This method wants a return <condition>; badly :D

This comment has been minimized.

Copy link
@Cousjava

Cousjava Nov 4, 2019

Author Member

What do you mean?

This comment has been minimized.

Copy link
@jbee

jbee Nov 5, 2019

Contributor

I mean that the sequence of if-return true/false; else if return true/false is much better expressed as a single return with a merged condition.

This comment has been minimized.

Copy link
@Cousjava

Cousjava Nov 7, 2019

Author Member

As in:

Suggested change
public static boolean isRelativePath(String path) {
public static boolean isRelativePath(String path) {
if (!ok(path)) {
return false;
}
return path.startsWith(".") || (path.contains("/.")) || path.contains("\\.");
}

This comment has been minimized.

Copy link
@jbee

jbee Nov 7, 2019

Contributor

Yes :)

return name != null ? name.replaceAll("__", "/") : filename;
}

private static String addExtentiontoFileName(String filename) {
String extension = "";
if (filename.endsWith("_ear")) {

This comment has been minimized.

Copy link
@jbee

jbee Oct 31, 2019

Contributor

Something does not add up here. The new method addExtentiontoFileName is hard coded for _ear - also above where it is used the endsWith checks for .ear not _ear. Hm...

This comment has been minimized.

Copy link
@Cousjava

Cousjava Nov 4, 2019

Author Member

I didn't change any functionality here

This comment has been minimized.

Copy link
@Cousjava

Cousjava Nov 4, 2019

Author Member

This already existed, not a change of mine.

@@ -180,18 +184,12 @@ public static String replace(String s, String token, String replace) {
public static boolean isRelativePath(String path) {

This comment has been minimized.

Copy link
@jbee

jbee Nov 5, 2019

Contributor

I mean that the sequence of if-return true/false; else if return true/false is much better expressed as a single return with a merged condition.

@Cousjava

This comment has been minimized.

Copy link
Member Author

Cousjava commented Nov 7, 2019

Jenkins test please

@@ -174,7 +174,7 @@ public static boolean deleteFileMaybe(File f) {
///////////////////////////////////////////////////////////////////////////

public static boolean safeIsDirectory(File f) {
return (f != null && f.exists() && f.isDirectory());
return !(f == null || !f.exists() || !f.isDirectory());

This comment has been minimized.

Copy link
@jbee

jbee Nov 7, 2019

Contributor

This is not easier to understand. Does sonar suggest to change it like that?

This comment has been minimized.

Copy link
@jbee

jbee Nov 7, 2019

Contributor

Never mind, didn't see the full diff. I still suggest to express this with && instead.

This comment has been minimized.

Copy link
@dmatej

dmatej Nov 8, 2019

Contributor

I agree. If Sonar did not like it, it will not like this even more.
The original if was better, I would only remove those brackets and also f.exists, because f.isDirectory returns true only if file exists. That is probably why Sonar complained:

     * @return <code>true</code> if and only if the file denoted by this
     *          abstract pathname exists <em>and</em> is a directory;
     *          <code>false</code> otherwise

This comment has been minimized.

Copy link
@dmatej

dmatej Nov 8, 2019

Contributor

Weird ... I'm receiving yesterday e-mails now 🗡

@jbee
jbee approved these changes Nov 7, 2019
@Cousjava Cousjava merged commit 418cd91 into payara:master Nov 7, 2019
58 checks passed
58 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/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
3 participants
You can’t perform that action at this time.