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

[Dev] Command refactorings - revise ExecutableCommand implementations #306

Closed
ljacqu opened this issue Dec 4, 2015 · 5 comments
Closed
Assignees
Milestone

Comments

@ljacqu
Copy link
Member

ljacqu commented Dec 4, 2015

This is being done by me, @ljacqu, along with #305 on a separate branch as part of a refactoring of the commands handling in AuthMe. This issue serves as a reference for the changes that will take place, and allows me to add an issue number to the commit messages that will result.

This story comprises three main goals:

  1. a. Rename any executable command class names that exist twice. This is very misleading and very dangerous. I'm thinking adding Admin to any such classes would probably be the way to go.
    b. Revise the packages. Ideally changes shouldn't be too big (imho not much gain from moving things around like crazy) but the packages aren't balanced at all. The command.executable.email package is great for example because it's a logical package (deal with email commands).

  2. Provide some abstraction for the commands. In particular, an abstract class could be put below ExecutableCommand for player-only commands. This can take care of the sender instanceof Player check for such commands, centrally and in a consistent manner. We will need to verify if any other such subtypes would make sense.

  3. Provide a service to the command implementations so we don't need to get any of the other instances statically. A facade service is probably the way to go there.

If someone has interest in working on these goals, please contact me.

@ljacqu ljacqu self-assigned this Dec 13, 2015
@ljacqu
Copy link
Member Author

ljacqu commented Dec 23, 2015

untitled diagram 1
Planned architecture – took a while to find out how to get rid of the circular dependencies

@ljacqu
Copy link
Member Author

ljacqu commented Dec 23, 2015

Second version. The issue is that HelpCommand requires all of the command mapping logic and I don't want to pass the CommandHandler to ExecutableCommand.

The new idea compared to the previous diagram is that we put everything inside the CommandService, and then both CommandHandler and any ExecutableCommand implementations only have to worry about the interface of the CommandService --> loose coupling.

CommandMapper will take care of the mapping logic (take command parts and return a FoundCommandResult) without any permissions logic. Call to PermissionsManager will be in CommandService, so both CommandHandler and ExecutableCommand can call the method there.

untitled diagram 1 1

Edit: Unless I pass the permissions manager to the command mapper and don't pass the permissions manager to command service. Then CommandMapper takes care of everything for the mapping logic, which would make the architecture more encapsulated...

@ljacqu ljacqu added this to the 5.2 Release milestone Dec 26, 2015
@Xephi
Copy link
Contributor

Xephi commented Dec 28, 2015

Are these change finished ?

@sgdc3
Copy link
Member

sgdc3 commented Dec 28, 2015

@ljacqu

@ljacqu
Copy link
Member Author

ljacqu commented Dec 28, 2015

Not yet. I want more to go through the command service, we still have a lot of "singleton injections" right now. I don't plan on cleaning everything within this issue, but there is still a little more to come. Also want to update the graph on the Wiki within this issue. :)

@ljacqu ljacqu closed this as completed Dec 31, 2015
@ljacqu ljacqu reopened this Jan 1, 2016
@ljacqu ljacqu closed this as completed Jan 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants