Guarmanda / LootChest

Plugin to reload chest with hologram, particles, and editing menu
GNU General Public License v3.0
11 stars 23 forks source link

Proposal to implement subcommand system for easier command handling #72

Closed GorgeousOne closed 2 months ago

GorgeousOne commented 2 months ago

Hello! First of all, I’d like to thank you for your work on this plugin. It’s been a great tool, and I really appreciate the effort you’ve put into developing and maintaining it!

I’ve been going through the command handling system in your plugin and noticed that you currently handle all your subcommands in a single class. Your plugin already has many features and handling 20+ subcommands in one class looks like it's increasingly hard to maintain and extend.

I’d like to propose implementing a subcommand system to delegate each subcommand’s logic to individual classes. This way, each subcommand can have its own dedicated handler, leading to cleaner, more modular code.

If you’re open to the idea, I’d be more than happy to help implement this for the plugin. The system would include:

This approach would keep the core command registration logic simple, while giving each subcommand its own well-defined responsibility.

Let me know if you’re interested in collaborating on this, and I’d be glad to get started with a pull request! Looking forward to hearing your thoughts.

Guarmanda commented 2 months ago

I thought about this once, but man, I wouldn't know someone would dig so deep into a random plugin code and ask to fix it :'D I agree with this idea and will read any pull request^^

Guarmanda commented 2 months ago

I'll review this this evening

Guarmanda commented 2 months ago

For respawnall and despawnall, [world] is an optional argument, but I can't really make it optional with your system, without creating a second command.

That's, I think, the only issue with it^^ But we can still do something to make optional args working

Guarmanda commented 2 months ago

I'm thinking about giving an array of required arg types and another of optional arg types to the constructor: It would remove many duplicated code about verifying if chest exist for example

GorgeousOne commented 2 months ago

Ah I see what you are saying. My initial thought was to reduce the number of required arguments in the super() call and then check for any additional arguments in the onCommand function. That might be more quick than clean though. Feel free to let me know if you'd like any help with the implementation!

Guarmanda commented 2 months ago

Ok, now all args are checked in ArgType class, optional arguments are possible, and I repaired arg number in all commands: you were, for example, getting the arg[0] to get a chest name, but arg[0] is the subcommand, not the first argument

This allowed me to remove any arg checking from commands

Guarmanda commented 2 months ago

75

Guarmanda commented 2 months ago

Thank you so much for this hard work^^