-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Added some javadocs #3217
Added some javadocs #3217
Conversation
Can one of the admins please verify this patch? |
/** | ||
* {@inheritDoc} | ||
*/ | ||
|
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.
It's great that you decided to remove these redundant javadocs.
However, note that every method now has 2 newlines (in this case, 20 and 21) perceding it. We usually like 1 newline. Can you please remove the extra newline for all the javadocs you deleted?
@@ -19,6 +19,7 @@ | |||
import org.terasology.module.sandbox.API; | |||
|
|||
/** | |||
* This interface bundles together all the methods chunk contains. |
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 name of the interface is Chunk, so obviously it will bundle the methods of chunk - doesn't really help the reader. I would like to see a more concrete description of this (explain what a chunk is).
@@ -37,56 +37,211 @@ | |||
*/ | |||
@API | |||
public interface CoreChunk { | |||
/** | |||
* @return Position of the chunk in blocks |
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.
"position of the chunk in blocks"
This is not entirely clear. What do you mean? Do you mean it returns the local position of the chunk within a block, or blocks is the unit (like 3 blocks away from origin) ?
@@ -20,6 +20,9 @@ | |||
import org.terasology.world.generator.WorldConfigurator; | |||
import org.terasology.world.generator.WorldGenerator; | |||
|
|||
/** | |||
* The most common implementation of {@link WorldGenerator} based on the idea of Facets |
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 most common implementation"
The word common seems strange in this context. Common in what sense? Most commonly implemented (as in multiple people implement it, and most people implement this) or most commonly used implementation (implemented by one person, and used by most users) or what? Can you replace it with something else?
A basic implementation
or something.
@@ -83,6 +89,10 @@ public World getWorld() { | |||
return world; | |||
} | |||
|
|||
/** | |||
* Returns current {@link WorldBuilder} or creates new if none exists |
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 way your sentence is structured, it will
- Return existing one if it exists
- Or create a new one (and not return anything).
Rephrase it. The job of a javadocs writer is not just to convertgetWorldBuilder
toReturns world builder
.
For reference, this is how I would personally write it:
Returns the current WorldBuilder used by this WorldGenerator. If no WorldBuilder has been initialized so far, a new one is created and then returned.
Kinda verbose, but much more clear in meaning.
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.
Also, don't leave the @return
blank.
@return The WorldBuilder used by this WorldGenerator
or something.
I realize there is some duplication, but it's inevitable.
/** | ||
* Sets the seed to use for creating of the world made by this world generator. | ||
* <p> | ||
* NOTE: this is a String value. The long value used in the most common implementation of this interface |
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.
Instead of writing "in the most common implementation" link to the implementation that you're talking about.
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.
Thansk for the changes.
@@ -19,6 +19,9 @@ | |||
import org.terasology.module.sandbox.API; | |||
|
|||
/** | |||
* Chunks are the smallest logical units of the worlds. | |||
* | |||
* Each chunk is 32x32x64 in dimensions, and is the default unit passed to world generator during world generation. |
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.
and is the default unit passed to world generator during world generation. I'm not sure what this means exactly. I think you are talking about chunks as if they were a unit. The basic concept of a chunk is to allow the world to be partitioned into manageable blocks. maybe talk about a chunk in those terms rather than as a cubic unit.
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.
I think the size of a chunk is defined in ChunkConstants
. Probably it is better to add a reference to ChunkConstants instead of duplicating the values in the documentation here.
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.
One more improvement then this should be good to go :)
@@ -19,6 +19,9 @@ | |||
import org.terasology.module.sandbox.API; | |||
|
|||
/** | |||
* Chunks are the smallest logical units of the worlds. | |||
* | |||
* Each chunk is 32x32x64 in dimensions, and is the default unit passed to world generator during world generation. |
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.
I think the size of a chunk is defined in ChunkConstants
. Probably it is better to add a reference to ChunkConstants instead of duplicating the values in the documentation here.
@oniatus can you have a look at the latest changes and see if they are satisfactory? It'd be good to get this small PR out of the way. |
Looks okay. We may update the javadoc once we have sectors because then chunks have an impact on the entity-system too, at least as far as I know :) For now this is okay. |
Merged! Thanks @Adrijaned! Another PR bites the dust! |
Contains
Adds or edits a bunch of javadocs for a GCI task.
How to test
Since there are no code related changes, there is no testing required/possible.