Skip to content
This repository has been archived by the owner on Dec 5, 2021. It is now read-only.

Refactoring #35

Open
15 of 19 tasks
yannicklamprecht opened this issue Nov 26, 2018 · 26 comments
Open
15 of 19 tasks

Refactoring #35

yannicklamprecht opened this issue Nov 26, 2018 · 26 comments
Assignees
Labels
dependencies Pull requests that update a dependency file Enhancement New feature or request In Process This issue work in process v4 goal
Milestone

Comments

@yannicklamprecht
Copy link
Contributor

yannicklamprecht commented Nov 26, 2018

Needs heavily refactoring

  • be SOLID

    • Single Responsibility Principle
    • Open Closed Principle
    • Liskov Substitution Principle
    • Interface Segregation Principle
    • Dependency Inversion Principle
  • remove cyclic dependencies ( MsgUtil is depending on Util and the other way around, same for DatabaseHelper)

  • properly inject dependencies instead of static abuse shit

  • properly handle configurations

  • write Adapter for used functionality in libraries

  • use PreparedStatement in the right way

  • cache data instead of writing each time -> persist every N minutes

  • encapsulate the classes to reduce complexity

  • cleanup commands, introduce a subcommand system

  • use Player#hasPermission instead of using an explicit PermissionProvider that is already injected into Bukkit system

  • remove duplication of ServerNMS and ItemNMS

  • refactor MsgUtil

    • reduce code duplication
    • apply SRP
  • write javadoc for plugin

  • Remove classes that are not used anymore

  • refactor shop loader

@Ghost-chu
Copy link
Owner

use Player#hasPermission instead of using an explicit PermissionProvider that is already injected into Bukkit system
Actually this is from upstream https://github.com/KaiKikuchi/QuickShop's code.
New code part already use hasPermission,

@yannicklamprecht
Copy link
Contributor Author

Then it can be removed.

@Ghost-chu
Copy link
Owner

com.comphenix.packetwrapper maybe will use again in next version, so temporarily not deleted

@Ghost-chu
Copy link
Owner

cache data instead of writing each time -> persist every N minutes
what cache data?

@Ghost-chu
Copy link
Owner

use PreparedStatement in the right way
Actually, i not be good at for SQL...

@Ghost-chu
Copy link
Owner

properly handle configurations Yes, you are right

@Ghost-chu Ghost-chu added the Enhancement New feature or request label Nov 26, 2018
@Ghost-chu
Copy link
Owner

I think i need sleep and find another time to continue refactoring.
00:03 BJT now.

@yannicklamprecht
Copy link
Contributor Author

cache data instead of writing each time -> persist every N minutes
what cache data?

Here we need to evaluate how frequently the database is accessed and if it is necessary to cache certain data changes to reduce load.

Good night.

@Ghost-chu
Copy link
Owner

Complete Remove classes that are not used anymore

@yannicklamprecht
Copy link
Contributor Author

yannicklamprecht commented Dec 5, 2018

What do you think of that config format?

!!org.maxgamer.quickshop.language.Language
command:
  descriptionDebug: '&ePrint debug infomation'
  noTargetGiven: '&cUsage: /qs export mysql|sqlite'
controlpanel:
  empty:
    hover: '&eLooking you want changing shop and click to clear.'
    normal: '&aEmpty: Remove shop all items &e[&d&lOK&e]'
  information: '&aShop Control Panel:'
  modeBuying:
    hover: '&eLooking you want changing shop and click to switch enabled or disabled.'
    normal: '&aShop mode: &bBuying &e[&d&lSwitch&e]'
  modeSelling:
    hover: '&eLooking you want changing shop and click to switch enabled or disabled.'
    normal: '&aShop mode: &bSelling &e[&d&lSwitch&e]'
  price:
    hover: '&eLooking you want changing shop and click to set new price.'
    normal: '&aPrice: &b{0} &e[&d&lSet&e]'
  refill:
    hover: '&eLooking you want changing shop and click to refill.'
    normal: '&aRefill: Refill the shop items &e[&d&lOK&e]'
  remove:
    hover: '&eClick to remove this shop.'
    normal: '&c&l[Remove Shop]'
  setOwner:
    hover: '&eLooking you want changing shop and click to switch owner.'
    normal: '&aOwner: &b{0} &e[&d&lChange&e]'
  sign:
    owner:
      line1: ''
      line2: Enter
      line3: new owner name
      line4: at first line
    price:
      line1: ''
      line2: Enter
      line3: new shop price
      line4: at first line
    refill:
      line1: ''
      line2: Enter amount
      line3: you want fill
      line4: at first line
  unlimited:
    hover: '&eLooking you want changing shop and click to switch enabled or disabled.'
    normal: '&aUnlimited: {0} &e[&d&lSwitch&e]'
language:
  contributors: [
    ]
  country: England
  version: 1
languageVersion: 2
shopDoesNotExist: '&cThere had no shop.'
sign:
  owner:
    line1: ''
    line2: Enter
    line3: new owner name
    line4: at first line
  price:
    line1: ''
    line2: Enter
    line3: new shop price
    line4: at first line
  refill:
    line1: ''
    line2: Enter amount
    line3: you want fill
    line4: at first line
signsUlimited: Unlimited

See Structure:
https://github.com/Craftstuebchen/QuickShop-Reremake/tree/remaster/src/main/java/org/maxgamer/quickshop/language

See test case for loading:
https://github.com/Craftstuebchen/QuickShop-Reremake/blob/remaster/src/test/java/org/maxgamer/quickshop/language/LanguageProviderTest.java

@Ghost-chu
Copy link
Owner

404 Not Found

@Ghost-chu
Copy link
Owner

Add a YamlProvider not a good idea in this project.
Developer should keep the plugin source is simple and easy to modfiled because this is a Bukkit plugin.
If add provider for all configuration, that's will make code mess and waste time.

But you are right, i need improve configuration files.

@yannicklamprecht
Copy link
Contributor Author

In my opinion the objectification of the configuration is a need.

@Ghost-chu
Copy link
Owner

This is a how make code clear, simple, and make the configuration file friendly and human-readable question

@Ghost-chu
Copy link
Owner

So i will create a branch to work(Do a breaking changs on master 100% not a great idea) and try that.

Maybe i can explode the config.yml to mutil files.

@yannicklamprecht
Copy link
Contributor Author

The Advantage of my system is, that outdated config values are automatically removed, when the model classes doesn't have the value anymore. New attributes are automatically updated if version number is increased in in main model class.

@yannicklamprecht
Copy link
Contributor Author

I think you should forget everything. The code of this project should be thrown away. Keep the feature description and code it from scratch.

@Ghost-chu
Copy link
Owner

Ghost-chu commented May 27, 2019

Issue reopened.
I'm cleaning codes and hope it keep the stable.

-> working for adapter configurations

Use reflect to read config to field and write to config from field

@Ghost-chu
Copy link
Owner

refactored shop loader

@Ghost-chu
Copy link
Owner

Finished
use PreparedStatement in the right way
and
cache data instead of writing each time -> persist every N minutes

@Ghost-chu
Copy link
Owner

Finished cleanup commands, introduce a subcommand system in 2.2.0

@cakoyo cakoyo added the In Process This issue work in process label Jan 26, 2020
@Ghost-chu Ghost-chu added this to the 4.0.0 milestone Jan 28, 2020
@Ghost-chu
Copy link
Owner

This issue can be closed because we're under full rewriting.

@Ghost-chu Ghost-chu reopened this Apr 6, 2020
@Ghost-chu
Copy link
Owner

Reopen issue cause v3 under refactoring

@Ghost-chu
Copy link
Owner

Cleanup the static abuse everywhere.
Only use it when really need.

@issuelabeler issuelabeler bot added the dependencies Pull requests that update a dependency file label Oct 2, 2021
@Ghost-chu
Copy link
Owner

Ghost-chu commented Oct 2, 2021

Refactored MsgUtil class, but still have lots of works need to do.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Pull requests that update a dependency file Enhancement New feature or request In Process This issue work in process v4 goal
Projects
None yet
Development

No branches or pull requests

5 participants