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

Standardize all models #57

Closed Incapamentum closed 1 year ago

Incapamentum commented 1 year ago

Despite the title, this is more of a refactoring. A lot of the models contain similar fields, and there was a lot of repeated code. In an effort to adhere to DRY (Don't Repeat Yourself), the best course of action was to go ahead and add in a BaseTemplateDoc.cs, which contains fields for doc Id, doc Title, and doc Date. All these might possibly be null (which is something to further look into).

In an effort to be more conservative, this change won't be merged yet, as it might break some things. The following should be done:

Incapamentum commented 1 year ago

Explored the concepts behind null safety. In a nutshell, ! and ? postfix operators change the nullability state of a variable. If ! is used, the reference is non-nullable, which means it can not be null. In a similar sense, the use of ? denotes the reference is nullable, which means it can be null.

As it stands, some files (perhaps namely the BaseTemplateDoc.cs) make use of null!, which tells the compiler that null is not a nullable value, which can be dangerous. It's best to perhaps indicate and enforce that some fields in the doc may be nullable.

Incapamentum commented 1 year ago

Went ahead and checked all Model references. All appear in the DatabaseService.cs file, and as mentioned here and here, the file should also be refactored, but should be tracked in a separate issue.

Incapamentum commented 1 year ago

Local JSON files have been updated and uploaded to the local database. Will do a quick test before going ahead and merging all the commits to main.

Incapamentum commented 1 year ago

In hindsight, it might be difficult to try and test these changes in a dev environment. I think it may now be necessary to introduce a unit testing framework. According to Microsoft documentation, they do provide one for .NET code. Should look into it. An issue should also be opened to track this.

Incapamentum commented 1 year ago

58 has been created to document the need for a testing framework. Issue will also depend on this pull request as well. No way to try and avoid two completely different things from being done in the same PR due to the necessity to test the above changes.

Incapamentum commented 1 year ago

There's been quite a bit of major changes being done, all due to having changed a single class implementation.

Good news: now we should be able to develop unit tests now!

Incapamentum commented 1 year ago

So I think everything has been done accordingly... maybe... hopefully.

Fingers crossed!