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

Unify Town and Nation code, and introduce new Eco Account Mechanics #3988

Merged
merged 94 commits into from Jul 17, 2020

Conversation

ghost
Copy link

@ghost ghost commented May 7, 2020

Description:

Part I: New Object: Territory

As I was working on the Town and Nation code, I couldn't help but realize that so much of the code between towns and nations is similar if not exactly the same. To prevent duplication of code, I've introduced a new object Territory which both Town and Nation inherit from. By introducing this we'll remove redundant implementations, and centralize code while providing compatibility. As a result, a net change of code is now negative even with other various changes in this PR, and we can take advantage of the polymorphic design in method parameters.

Here's a UML diagram which illustrates this change:

Screen Shot 2020-05-08 at 10 32 30 PM

Part II:

While the economy account serves our purposes right now, it wasn't built for any type of expansion. I'm planning on adding auditing features to accounts so now players can keep track of where expenses end up, but we may need to add other additions in the future, hence these changes were put into place.

Because of this, objects define their Economy account field differently:

// Before
EconomyAccount account = new EconomyAccount(...);

// After
Account account = new EconomyAccount(...);

EconomyHandler has been updated to reflect these changes as well:

public interface EconomyHandler {
   // Before
  EconomyAccount getAccount();

  // After
  Account getAccount();
}

This makes the return and parameter types more open, as any subclass or variant of Account can be accepted rather than just EconomyAccount.

A new object: Account was created, this is now the base for EconomyAccount and CappedAccount, EconomyAccount is just an account for holding money, while a CappedAccount is an object that puts a cap on the balance it contains.

Part III:

New mechanics are now being used to keep track of the state of Account's, it uses an observer pattern to notify any listener of any account balance changes which means we can separate tracking code from accounts, and use the logic in other classes.

Here's how this functionality is being used:

First, create an observer.

public class MyObserver implements AccountObserver {
    @Override
    public void withdrew(Account account, double amount, String reason) {
	// Do something when money is withdrawn in the specified account.
    }

    @Override
    public void deposited(Account account, double amount, String reason) {
	// Do something when money is deposited in the specified account.
    }
}

Then add the observer to an account to listen for changes to that account:

myAccount.addObserver(new MyObserver());

Now any balance changes to myAccount will call the methods in MyObserver to handle the balance changes in whichever way is wanted.

"Why not use the Bukkit event system for eco transaction tracking?"

We still are, but this system is separate from the Bukkit events:
Essentially, this auditor system is meant for inner use and not third-party manipulation, in short, we have a clear internal data flow rather than having to worry about third-party intervention. External plugin devs will use a separate Bukkit event described below.

This also adds a new event:

BankTransactionEvent, which gets called regardless of a Town or Nation bank transaction.

Part IV - New abstract EconomyAdapter

A new economy adapter has been introduced to allow with interfacing directly with different economy implementations easier.

Currently we have to switch and check for every eco type:

public void operateOnEco() {
    switch (EcoType) {
       case VAULT:
         vaultEco.subtract("account", 10);
       case RESERVE:
         reserveEco.subtract("account", 10);
    }
}

Now this is abstracted into a single object:

public void operateOnEco() {
   ecoAdapter.subtract("account", 10);
}

Additional Changes

Most if not all of the methods that get a list of objects from another object eg: #getTownBlocks(), getResidents()... now return immutable/read only lists. Why? it has to do with write protection.

We don't want clients doing:

// Bypasses any logic that should be executed in .addTownBlock(),
// which leads to undefined behavior down the road.
myTown.getTownBlocks.add(new TownBlock());

Now if clients try to do that with this version:

myTown.getTownBlocks.add(new TownBlock()); // ERROR: Throws UnsupportedOperationException

Thus they will be forced to use the proper way of adding/removing things:

myTown.addTownBlock(new TownBlock()); // OK

New Nodes/Commands/ConfigOptions:

New Config Node:
ECO_DEBT_PREFIX - Specifies the prefix for debt accounts in towny. Defaults to "[DEBT]-".


  • I have tested this pull request for defects on a server.

By making this pull request, I represent that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the TownyAdvanced organization has the copyright to use and modify my contribution under the Towny License for perpetuity.

@ghost ghost requested a review from LlmDl May 7, 2020 23:27
@ghost ghost self-assigned this May 7, 2020
@ghost ghost requested a review from LlmDl May 8, 2020 20:09
@ghost ghost changed the title Unify Bank methods and provide defaults. Unify Town and Nation code under new Object: Territory May 8, 2020
@ghost ghost changed the title Unify Town and Nation code under new Object: Territory Unify Town and Nation code, and introduce new Eco Account Mechanics May 17, 2020
@ghost ghost requested a review from LlmDl July 7, 2020 02:22
Suneet Tipirneni (Siris) added 6 commits July 7, 2020 14:39
# Conflicts:
#	src/com/palmergames/bukkit/towny/command/TownCommand.java
#	src/com/palmergames/bukkit/towny/event/NationTransactionEvent.java
#	src/com/palmergames/bukkit/towny/event/TownTagChangeEvent.java
#	src/com/palmergames/bukkit/towny/event/TownTransactionEvent.java
#	src/com/palmergames/bukkit/towny/invites/InviteHandler.java
#	src/com/palmergames/bukkit/towny/object/Resident.java
#	src/com/palmergames/bukkit/towny/object/Town.java
#	src/com/palmergames/bukkit/towny/object/Transaction.java
#	src/com/palmergames/bukkit/towny/object/inviteobjects/NationAllyNationInvite.java
#	src/com/palmergames/bukkit/towny/object/inviteobjects/PlayerJoinTownInvite.java
#	src/com/palmergames/bukkit/towny/object/inviteobjects/TownJoinNationInvite.java
#	src/com/palmergames/util/StringMgmt.java
@ghost ghost requested a review from silverwolfg11 July 16, 2020 16:12
Suneet Tipirneni (Siris) and others added 2 commits July 16, 2020 12:24
- unused imports
- undo orphanged residentList->owner renames
- remove unused method in Town object.
# Conflicts:
#	src/com/palmergames/bukkit/towny/tasks/DailyTimerTask.java
@LlmDl LlmDl merged commit 5e70b60 into master Jul 17, 2020
@LlmDl LlmDl deleted the refactor-bank branch July 17, 2020 22:22
LlmDl added a commit that referenced this pull request Jul 17, 2020
courtesy of Siris with #3988.
  - New config option: economy.debt_prefix
    - default: "[DEBT]-"
    - The debt prefix for the debt eco account.
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.

None yet

4 participants