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

Improvements #6

Open
messer00 opened this issue Jun 21, 2022 · 1 comment
Open

Improvements #6

messer00 opened this issue Jun 21, 2022 · 1 comment

Comments

@messer00
Copy link

Hey

As promised I'd like to offer a few suggestions.

  1. As far as code organization goes, current separation of concerns is mainly constrained by highly horizontal dependency.
    I think that separating lobby creation out into some sort of a builder and something to actually obtain required data would be a great start.

Long term this codebase could make use of simple dependency injection. This way you can make it much more flexible.
This can easily be done in Rust with Traits. That way you can easily redesign and add new code without having to modify previously written components.

It's easy to start here, even repeating code between modules if required to untangle unrelated things.

  1. Fixing a few critical issues:
    Same with task scheduling via tokio. For example tokio::spawn fails, the lobby being posted cannot be joined until the bot is restarted and critically doesn't seem to produce any errors to notify the user.

There are some Discord limitations that can cause issues. For example, options in select boxes are limited to 25, so with bigger guilds it can easily break lobby creation.

You can make changes to characters belonging to someone else.

  1. Messages visibility and limits:
    Currently commands are largely public with their output. For example, lobby creation is needlessly visible for everyone even before being posted.

  2. Certain things are outdated
    I'm sure you know that already, but ilvls are limited to 1490, which you can pass in the current version of the game. Classes are missing the latest addition too. This can and will prevent people from trying to use this the most.

  3. The choice of using .embed
    Some people have embeds disabled. So for them, each message is just a few buttons with no text or images.

I don't have much time to code features here for now, but I do intend to contribute in the future. It's a solid foundation for a bot.

@boraarslan
Copy link
Owner

First of all, thanks for your interest and feedback. I am currently the only developer of the bot, so a second opinion is very valuable to me.

  1. As far as code organization goes, current separation of concerns is mainly constrained by highly horizontal dependency.
    I think that separating lobby creation out into some sort of a builder and something to actually obtain required data would be a great start.

You are right. But to give context, this project was never meant to scale this big (even though it is still small now). Separating the logic into a kind of like Service style is probably the most desirable and scalable solution. Currently, the only "source of truth" is the lobby context itself which is flawed in many ways, as you mentioned.

  1. Same with task scheduling via tokio. For example tokio::spawn fails, the lobby being posted cannot be joined until the bot is restarted and critically doesn't seem to produce any errors to notify the user.

Embarrassingly, it was the first idea that came to my mind when thinking about creating and restarting lobbies. But, as I said, the bot was not meant to be this complicated but a simple test project to try out poise and the rust discord ecosystem. So in future updates moving away from this approach is preferable.

For example, options in select boxes are limited to 25, so with bigger guilds it can easily break lobby creation.

I was not aware of this, thanks for pointing it out.

  1. Messages visibility and limits:
    Currently commands are largely public with their output. For example, lobby creation is needlessly visible for everyone even before being posted.

Good point. I was thinking about changing it after adding some key features because it is easier to debug this way. Still, it would be great for visibilities to be configured from the start considering people might want to use the bot before the "release."

  1. The choice of using .embed
    Some people have embeds disabled. So for them, each message is just a few buttons with no text or images.

This problem would limit development if we wanted to solve it. I have some idea why someone would want to disable embeds, but sadly, discord doesn't give developers any other user interface to work with besides basic text and embed messages. Embeds look distinct from regular messages, and I think it is better to stick with embed messages even though some people wouldn't have a good (or any) experience.

  1. Certain things are outdated
    I'm sure you know that already, but ilvls are limited to 1490, which you can pass in the current version of the game. Classes are missing the latest addition too. This can and will prevent people from trying to use this the most.

Yes. Sadly, I quit playing the game and developing the bot to focus on work and school. That's why I haven't released even a simple update for the bot.

Like I said in the previous issue, some of the codebase (if not most) needs to be rewritten. I am currently busy with work and graduating, so I will not be able to work on the project for some time (I can't give you an exact time when I am free, but it should not be long since I expect to graduate in 2 weeks.) I also quit the game for the reasons above (sadly), but I am still willing to develop the bot. And it would be great to have more maintainers.

You can always contact me if you have any counter-proposal or additional comments.

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

No branches or pull requests

2 participants