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

Update CommandManager.java #55

Merged
merged 3 commits into from
Sep 12, 2024
Merged

Conversation

TrivCodez
Copy link
Contributor

new

@coltonk9043
Copy link
Owner

Hey! I'm not home rn so I'll review this once I'm back. I briefly looked over it and it looks great though.

@TrivCodez
Copy link
Contributor Author

Alright, No problem!

@TrivCodez
Copy link
Contributor Author

TrivCodez commented Sep 12, 2024

improved code efficiency and readability by:

  • Implementing try-with-resources statements for better resource handling
  • Simplifying methods using StringBuilder for more concise string manipulation
  • Leveraging Java 8's removeIf and anyMatch methods for more expressive and efficient collection operations
  • Removing unnecessary comments and whitespace for easier code maintainability

@coltonk9043
Copy link
Owner

Overall looked pretty good. I am a fan of the removeIf and anyMatch method changes. I have pushed a commit that fixes some issues I have/found:

While I do agree that the format of javadoc I used was a little excessive, I do believe that every public method should at least have a minimal amount of documentation to at least state what the method is doing, even if it is clear from the name of the method. I have added those back but made them single-line javadoc comments to save a little bit of space.

Another small issue was that all of the commands were gone. The constructor of 'CommandManager' uses reflection to find all of the fields that are declared as a sub-class of 'Command', and then adds them to the Map of commands. This way, all I have to do to add a command is declare it in 'CommandManager' for it to show up in-game.

Overall though, very good. Let me know what you think of the changes I made. If you are happy with it, I can likely merge this tomorrow. Thanks :)

Copy link
Owner

@coltonk9043 coltonk9043 left a comment

Choose a reason for hiding this comment

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

Merging PR. Thanks again.

@coltonk9043 coltonk9043 merged commit 57d2911 into coltonk9043:master Sep 12, 2024
2 checks passed
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