Improve ReactionRole command
BasketBandit opened this issue · 3 comments
Is your feature request related to a problem? Please describe.
The ReactionRole command needs a revamp. It's fairly long winded and could do with simplification. I keep forgetting to do it so am creating this issue so it lingers until something is done about it.
Describe the solution you'd like
Improvement to current command, re-written and simplified. Potentially making adding/removing reaction roles a 1 step command instead of the current system which takes 2-3.
For example, the current command works as so:
-reactrole select <message_id>
-reactrole add <_emote> <_role>
It would be quicker for users to do this all in a single step:
-reactrole <message_id> <_emote> <_role>
Removing a react would simply be the same but without including the role and clearing all reacts would just be a case of using the command again but removing both emote and role.
Link to command file
https://github.com/Yuuko-oh/Yuuko/blob/master/src/main/java/com/yuuko/core/commands/utility/commands/ReactionRoleCommand.java
Hi!
Really nice project!
It will be my first time working with Discord bots, but I would like to help with it.
Unfortunately I need some clarification.
What do you mean by making a 1 step command?
Also, I see that you are using your own wrapper around OkHTTPClient, maybe turn to something like retrofit? Form my own experience, using self made wrapper can result in many additional work when implementing new functions in project. I can try to implement retrofit if you want.
There are also many literal strings in code. It would be easier to translate or refactor them if they were in one class or external config file.
Wouldn't it be better if SQL would be handled by ORM? Like Hibernate.
Apologies, I have updated the issue to clarify what I meant by a 1 step command.
Also, I see that you are using your own wrapper around OkHTTPClient, maybe turn to something like retrofit? Form my own experience, using self made wrapper can result in many additional work when implementing new functions in project. I can try to implement retrofit if you want.
Regarding Retrofit, for the scope of the project currently, it doesn't seem make too much sense to switch away from OkHttp. I like the level of abstraction there currently is since I am able to make http/s calls and then manipulate the result however necessary. For additional API based commands I feel that the groundwork has been more or less done and I am comfortable traversing JSON objects at this point.
That said, if there are any substantial benefits to performance, etc, that I am unaware of I'd be more than happy to look further into it.
There are also many literal strings in code. It would be easier to translate or refactor them if they were in one class or external config file.
Regarding the string literals, I do actually agree that moving them all/the majority into an external config/yaml/etc would be beneficial. Especially for purposes such as translation of the bot in future. I'm not exactly sure where I would start with that though. It's quite a lot of work to do this retroactively with 85+ commands.
Wouldn't it be better if SQL would be handled by ORM? Like Hibernate.
Lastly regarding the SQL stuff. I did some research last night and I can't immediately see what improvement switching to an ORM would give to the project at this point. From my limited knowledge of them it seems like an unnecessary level of abstraction since the current system works as intended, and is very easy to understand/maintain. If you could give me more information about the cost/benefit of implementing something like that over the current system I would be happy to look more into it.
I do appreciate however that a class named DatabaseFunctions isn't exactly useful and could potentially be split into separate classes as it currently reads more as a utility class than anything.
I appreciate you taking the time to look at the project!
Addressed by 12c2e52.