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

Feature/code clean up #603

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ekmillard
Copy link
Contributor

No description provided.

Copy link
Owner

@GregHib GregHib left a comment

Choose a reason for hiding this comment

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

Documentation should provide clarity or additional information that isn't already obvious from the code itself e.g. why it was built the way it was, how it should be used etc...

A lot of these, presumably ai generated, comments are summaries of what the code already says, not providing any extra info and actively making it harder to comprehend or find out information out as reader (who doesn't have a million token context length).

I've marked a few examples of ones that aren't useful and why.

One of the places documentation summaries are useful however, is at a class level. A sentence or two explaining what a class does, so a reader can quickly understand without having to read a hundred lines of code would be far more useful. (And a lot easier to review)

I would caution overriding existing descriptions, and certainly no @params.

Comment on lines +47 to +48
* @param E The type of elements held in this queue.
* @param capacity The maximum number of elements the queue can hold.
Copy link
Owner

Choose a reason for hiding this comment

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

Self explanatory

Comment on lines +27 to +35
* @param players The collection of active players in the game world.
* @param npcs The collection of non-player characters (NPCs) in the game world.
* @param items The collection of floor items available in the game world.
* @param objects The collection of objects in the game world.
* @param itemDefinitions Definitions and metadata describing all game items.
* @param objectDefinitions Definitions and metadata describing all game objects.
* @param npcDefinitions Definitions and metadata describing all NPCs.
* @param interfaceDefinitions Definitions for user interfaces within the game.
* @param handler Interface handler for user interaction with game interface elements.
Copy link
Owner

Choose a reason for hiding this comment

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

Unnecessary

Comment on lines +48 to +64
/**
* Represents a handler for managing interactions with floor items in the game.
* The variable encapsulates a FloorItemOptionHandler which provides functionality
* to handle specific options or actions related to items located on the ground.
* It operates on a list of items that can be interacted with directly from the game floor.
*/
private val interactFloorItem = FloorItemOptionHandler(items)
/**
* A private instance of DialogueContinueHandler used to manage and control the continuation of dialogue interactions.
* It leverages interfaceDefinitions to handle specific interaction rules or configurations.
*/
private val interactDialogue = DialogueContinueHandler(interfaceDefinitions)
/**
* This variable represents an instance of InterfaceClosedHandler
* that is used to handle the closure of an interface. It encapsulates
* logic or actions to be performed when the interface is closed.
*/
Copy link
Owner

Choose a reason for hiding this comment

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

All of these are excessive

Comment on lines +40 to +44
/**
* Logger used for recording debug and error messages related to the processing
* of player instructions in the `InstructionTask` class. Provides inline logging
* capabilities for better performance and structured output.
*/
Copy link
Owner

Choose a reason for hiding this comment

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

No need to explain what a logger is.

Comment on lines +16 to +19
/**
* Companion object for the `Catch` class.
* Provides utility methods and properties for handling instances of the `Catch` class.
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Companions don't ever need descriptions

Comment on lines +196 to +199
* @param public The public status of the player. Expected values are:
* "friends", "off", "hide", or other values default to 0.
* @param trade The trade status of the player. Expected values are:
* "friends", "off", or other values default to 0.
Copy link
Owner

Choose a reason for hiding this comment

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

Misses context as to what 0 means - "on"

Comment on lines +39 to +44
/**
* Retrieves the area associated with the given name.
*
* @param name The name of the area to retrieve.
* @return The area corresponding to the provided name, or an empty area if the name is not found.
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Says nothing

Comment on lines +12 to +13
* The `AmmoDefinitions` class is responsible for decoding and managing a collection of ammo definitions.
* It inherits from `DefinitionsDecoder` with a specific type of `AmmoDefinition`.
Copy link
Owner

Choose a reason for hiding this comment

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

Loses all the useful information of the existing comment and isn't using Kotlin doc syntax

Comment on lines +15 to +17
* Represents a collection of area definitions, which can be accessed based on various keys such as
* name, tags, or zones. This class centralizes the management and retrieval of area definitions,
* allowing for efficient queries and lookups.
Copy link
Owner

Choose a reason for hiding this comment

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

Is a good description

Comment on lines +22 to +25
* @param text The content of the message that will be sent.
* @param type The type of chat message to display, default is ChatType.Game.
* @param tile The tile ID associated with the message, default is 0.
* @param name Optional name associated with the message, default is null.
Copy link
Owner

Choose a reason for hiding this comment

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

These param descriptions are better than the original, but arguably neither should exist.

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