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

Cleanup documentation and modifiers #123

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

holgerfriedrich
Copy link
Contributor

@holgerfriedrich holgerfriedrich commented Oct 12, 2023

  • Cleanup Javadoc annotations (removing brackets from {@return statements})
  • Change modifiers according to JLS (mainly removing final from static methods, as static methods are implicitly final, but also order of modifiers)
  • Minor other improvements

Copy link
Collaborator

@bmalinowsky bmalinowsky left a comment

Choose a reason for hiding this comment

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

Thanks, I added some comments.

@@ -207,7 +207,7 @@ public static void addressStyle(final Presentation style)
}

/**
* {@return the address presentation style preset for all KNX group addresses}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's an inline return tag, it should work fine (as well as all the others).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inline javadoc return statements were not properly recognized by my tooling (though technically fine) and I had not realized that you just changed it to be this way not too long ago.

@@ -506,7 +506,7 @@ final String removeUnit(final String value)
return value.trim();
}

static final short ubyte(final int value)
static short ubyte(final int value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Putting final on static methods prevents hiding the method, its usage is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rolled back as requested. AFAIR there are methods only declared static without the final keyword. Not sure if this is intended then.

@@ -982,7 +982,7 @@ else if (++day == 1) {
int dow = DAYS.length - 1;
while (dow >= 0 && !DAYS[dow].toLowerCase().startsWith(prefix))
--dow;
final boolean anyday = dow == 0 && t.hasMoreTokens() && t.nextToken().equalsIgnoreCase("day");
final boolean anyday = dow == 0 && t.hasMoreTokens() && "day".equalsIgnoreCase(t.nextToken());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Switching the operands in cases like these seems unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using "constant".eqals(somestringvariable) has the benefit that the string constant is never null and running equals() cannot cause a NPE. It cannot harm to write it in this order.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's true; the thing is, if nextToken returns null something is really really wrong. So a reversal like this basically hardens against the langspec/compiler/interpreter.

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
@holgerfriedrich
Copy link
Contributor Author

Hi @bmalinowsky, sorry for replying late.

I followed your advice and rolled back accordingly. Seems this PR is down to two minor changes....

@bmalinowsky bmalinowsky merged commit ab1c7f4 into calimero-project:master Oct 24, 2023
0 of 2 checks passed
@bmalinowsky
Copy link
Collaborator

Thanks for the PRs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants