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

More polishing #17

Merged
merged 7 commits into from
Mar 2, 2021
Merged

More polishing #17

merged 7 commits into from
Mar 2, 2021

Conversation

imDaniX
Copy link
Contributor

@imDaniX imDaniX commented Mar 1, 2021

This is my latest PR for a while.

  1. Some more of those encapsulations and move-arounds like from the latest PR.
  2. Remove some of spaghetti code from the commands to increase readability.
  3. Added more TODOs for future changes.

It's possible to resolve issue #1 right now, but the problem is the data package - if we'll add the feature right now, it will be much harder to rework the package later. I think it should be generally recoded from scratch with OOP style in mind.

Move instance initialization into onLoad method
Refactor Settings class a bit and move header straight to the file
Added some TODOs to do
Replaced "dynamic" regex with cached Patterns
@WiIIiam278
Copy link
Owner

This is great stuff! Thanks again - i'll merge this in the morning.
On #1 and the data package, I definitely agree that it needs a total rewrite; it's pretty bad code right now and I have SQL syntax being shared across SQLite and mySQL which is definitely a bad idea. Moving it off the main thread will be a big boost to performance on servers with slow databases as that can cause big lagspikes at the moment.

I'm probably going to look towards #13 soon as my next project, which involves adding some more plugin messages and handlers to track a list of all players on HuskHomes installed servers. The refactoring you've done will make life a lot easier there 👍
Thanks again.

@WiIIiam278 WiIIiam278 self-assigned this Mar 2, 2021
@WiIIiam278 WiIIiam278 merged commit c7e6515 into WiIIiam278:master Mar 2, 2021
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

Successfully merging this pull request may close these issues.

2 participants