Incapamentum / Exalted-Sage

Discord bot that does API requests to retrieve a collection of daily achievements for the next day and pushes an alert when any that is in a watchlist shows up.
3 stars 0 forks source link

Event-Driven Methods for Modularization #5

Closed Incapamentum closed 1 year ago

Incapamentum commented 1 year ago

The Discord.NET API allows for event-driven routines to be executed. The way how this is achieved in the bot is through event handlers, i.e. MessageHandler and VoiceHandler. At the moment, these handlers are pretty simple and straightforward but lack robustness to ensure modularization upon meeting specific criteria.

Currently, I am anticipating that if this is something that is not resolved yet, it may cause a ton of technical debt to be accrued over time, so it's best to start looking into it now than not. In the case of the MessageHandler, for example, one idea would be to have the event criterion be from which text channel the message received/deletion originated in.

This is what will be done to remedy this:

Incapamentum commented 1 year ago

I think the best way to handle the modularization of the different event handlers is to check the origin of the event.

Taking the example of MessageHandler: it gets launched anytime a message is sent, but it only continues if the message originated from a small subset of channels. Therefore, making use of if/else statements (if less than 4 items to check for) or switch statements (if more than 4 items to check for) would be the ideal path to go. However, generic methods should be implemented to promote code reusability.

Incapamentum commented 1 year ago

The usage of a switch statement may not be the best due to it requiring constant values to be compared against. Pursuing this approach will then have the channels be hardcoded, and may cause scalability issues.

Will consider other alternatives for execution structures.

Incapamentum commented 1 year ago

Achieved an overall cleanup, although a better solution could probably be achieved.

As we're only concerned about a small subset of channels from which the bot can only really interact in, only process the messages that are coming from there.

Perhaps a better approach in the future would be to define different channel sets: Interactive, Monitor, etc. This may require some changes to the backend to achieve, but might be a cleaner and more future-proof approach on obtaining modularity.

Incapamentum commented 1 year ago

An initial cleanup was achieved, but I feel like more could be accomplished based on this.

Will go ahead and compile for release, then test before going ahead and merge with main.

Incapamentum commented 1 year ago

Elaborating further on this, the database backend is composed of (at the time of writing this comment) into five collections, one of which is channels. This collection contains four documents, of which three are related due to being text channels: General Channels, Supervised Channels, and Broadcast Channels. Their current format is as a <string, ID> mapping of the channels.

Aggregation of text channels in one or more docs will be necessary. An initial thought would be to aggregate all text channels under one doc: Text Channels. Similarly, the remaining doc in the collection, Event Voice Channels, could be renamed to Voice Channels and also pursue a similar template that will be pursued for the Text Channels.

For clarification: instead of having separate docs for different sets of channels in a <string, ID> mapping, aggregate all related channels based on type (i.e. message or voice) into a doc of template <string, Object>, where string is the name of the channel set, and Object is another mapping: <string, ID>, where string is the name of the channel, and ID is the channel UUID of type ulong.

In the past, I struggled with trying to come up with a doc model in C# that would allow me to achieve a <string, Object> mapping, so some additional research in this regard will be necessary.

Current plan is to discuss this with leadership to determine whether I should go ahead with this. It would be preferable to pursue it as a way to future-proof things and avoid technical debt along the way.

Incapamentum commented 1 year ago

After talking with leadership regarding this, the direction of having a mapping of <string, Object>, where Object is another mapping of <string, ID> is the best approach.

One suggestion that was provided was to have each channel set based on the channel categories of the guild, so it may provide better hierarchical organization.

Will look into ways on achieving this.

Incapamentum commented 1 year ago

After some initial work done towards this, the backend will definitely have to be changed up. A proper local environment has now been setup to avoid any clashing changes with the main database.

It is expected that the following source files will be affected:

Incapamentum commented 1 year ago

Additional changes will also be done to some (if not, all) of the docs to reflect a new structure in the schema.

The database contains a channels collection, but now it will only support one doc. This doc has the following structure:

{
    "Title": "Channels",
    "Categories": {
        "Category1": {
            "Text": {
            },
            "Voice": {
            }
        }
    }
}

This is essentially a massive doc that keeps track of all channel categories in the server. I'll reach out to others to get some input on whether this will be a good approach to things or not. It may be necessary to also have the bot update to the doc, but currently is not the highest priority.

Incapamentum commented 1 year ago

After some reflection, I think it may be best to simply have one doc for every category.

In terms of overall memory space, it will be much lower as opposed to having a single doc for all. In terms of management it will also be better as then changes will only need to be done on a per-doc basis as opposed to trying to manage the entire document on its own.

Therefore, each document will have the following conventions:

{
    "Title": "CategoryName",
    "Channels": {
        "Text": {
            "TextChannel1": 123456789
        },
        "Voice": {
            "VoiceChannel1": 987654321
        }
    }
}

Each doc will support a mapping of channels, both text and voice, and each supports a mapping of channel names with their UID.

Incapamentum commented 1 year ago

Still working towards implementing solutions to this issue, found out that each category has its own UID, thus the doc now looks as the following:

{
    "Title": "CategoryName",
    "CatId": 1598746320,
    "Channels": {
        "Text": {
            "TextChannel1": 123456789
        },
        "Voice": {
            "VoiceChannel1": 987654321
        }
    }
}

The SocketMessage class allows one to obtain a reference to the SocketTextChannel from which the message originated from, which carries a Category and CategoryId from which the channel belongs in. This also extends to SocketVoiceChannel as it seems to inherit from SocketTextChannel.

This is advantageous as now an alternative conditional execution statement could be used, such as a switch statement, through name of the category.

Incapamentum commented 1 year ago

The past couple of commits pushed today involved in resolving this issue. A best effort was placed towards keeping things as general as possible, while trying to take advantage of categories in the flow-invocation of specific methods to accomplish certain actions depending on the origin of the message.

Currently, the code in the branch associated with this issue is live and will be tested for a week to take note of any possible edge-cases that may have been missed in this refactoring. Once all is well, a PR will be created to merge.

Incapamentum commented 1 year ago

Going ahead and perform a pull-request on this. Seems to be running fine except for some unhandled exceptions being thrown. Am considering implementing a logger so that I can see exactly why errors are occurring. If they do, bot performance does not seem to be hindered.

Incapamentum commented 1 year ago

Issues still remain. Closer look will have to be managed.

Specifically, take a look at the MessageEventHandler and VoiceEventHandler for any issues. Major parts of code may have to be rewritten.

Incapamentum commented 1 year ago

Consulted with a friend on best approach to do this. In the end, working "backwards" may be the best approach.

First, the database will have to be revisited, taking a look at the different collections and re-justify their use within the overall application.

Next, code in DatabaseService.cs will have to be reworked to enforce code-style guidelines.

Finally, error handling should be included in any code that performs a database operation via the service. This affects the (currently) two event handlers (MessageEventHandler.cs and VoiceEventHandler.cs) and the event loop that exists within ExaltedSage.cs.

New tasks:

Incapamentum commented 1 year ago

A doc that explains some of the collections found in the database has been created. It (briefly) explains the collection, and what types of docs one can expect. It's meant to be straightforward, and each collection carries the same types of doc, i.e. one will find no more than one unique doc type per collection.

Incapamentum commented 1 year ago

Latest commit is moving forward to deprecating any methods found in the DatabaseService.cs that still relied on the channels collection in the database, which will no longer be supported and will be removed sometime in the future.

Incapamentum commented 1 year ago

Seems like the MongoDB docs for the C# API do not explain their exceptions well. Or, at the least, indicate whether their methods will throw an exception or not. In the end, it may also turn out to be better to close this issue once the event handlers are checked again, but to open a new issue to maintain some kind of possibly growing list of different exceptions so as to implement error handling when the time comes.

Incapamentum commented 1 year ago

Wanted to mention that, working through the MessageEventHandler.cs, I believe that it may be possible that only one processing method may be necessary for each category DEPENDING on what kind of processing occurs.

Thus far, it seems like it may be unique enough, Just something to keep in mind.

Incapamentum commented 1 year ago

Went ahead with refactor. Seems all the issues caused by the initial refactor in completing the tasks outlined in the initial issue have been fixed, but monitoring will continue.

Any future issues that arise due to this refactor will get their own issues, depending on the nature of the issue.