-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
apply preferred style to com.jme3.renderer #1653
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
Conversation
+ use curly braces where optional (4.1.1) + indent continuation lines by +8 (4.5.2) + no single space after an open paren or before a close paren (4.6.2) + no single space before an unary operator (4.6.2) + no single space before a square bracket (4.6.2) + single space before "else" (4.6.2 #2) + single space before an open curly brace (4.6.2 #3) + single space around a binary or ternary operator (4.6.2 #4) + single space after a comma (4.6.2 #5)
+ paragraphs separated by a blank line (7.1.2) + successive paragraphs start with "<p>" (7.1.2) + javadoc blocks begin with a summary fragment, not a sentence (7.2) + summary not in imperative mood (7.2) + summary capitalized and punctuated like a sentence (7.2)
|
|
||
| /** | ||
| * <code>getFrustumBottom</code> returns the value of the bottom frustum | ||
| * Returns the value of the bottom frustum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely aside from the format/styling concerns, I question the value of this, and other "Trivial" Javadoc comments in this file. It adds no details that cannot be gleaned from the method signature, and therefore does not add value above what the javadoc tool would create on it's own. Why does it exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style guide says that javadoc is optional for "simple, obvious" methods like getFoo, in cases where there really and truly is nothing else worthwhile to say but "Returns the foo".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR shows minimal changes. Deleting the entire javadoc seemed like a bigger change than deleting one word.
I agree that the current javadoc isn't valuable. Honestly, I haven't studied the frustum math to figure out what "the value of the bottom frustum" really is. Is it the y/z slope, the y-coordinate at the near plane, or the y-coordinate at the far plane? So I believe getFrustumBottom() needs some javadoc, but I don't yet know enough to write it myself. Writing javadoc for someone else's code can be very frustrating!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, though in this case the code literally passes through the value of a field.
I hear you on documenting other people's code. In the case of the frustrum stuff here, I'd think that the javadoc should go on the field declarations, as they are protected and could, in theory, be overridden by extension code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, there's a proposed change (to the style guide) that would remove the option of omitting javadoc for "simple, obvious" methods. Discussion of this proposed change is at #1098.
|
I believe this PR's usefulness as a conversation piece is over. I'll revert the 2 API changes and make another pass over the files. |
|
I plan to integrate this in 24 hours. |
This PR illustrates minimal changes to make the
com.jme3.rendererpackage comply with the coding style proposed at issue #1098.Marked as "draft" because the PR includes 2 API changes that should be integrated separately, as discussed at #1098.