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

Refactored the command classes that adds stats #265

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thebuilderbob1
Copy link

@thebuilderbob1 thebuilderbob1 commented Aug 21, 2024

Description

  1. The commands to assign stats LUK, STR, DEX and INT all use the same code, basically copy/pasted 4 times. I have refactored that code into one class and made it reuseable.
  2. Changed the logic to only add the amount of AP that is actually typed to prevent the player from adding too much AP by mistake. For example:
  • Player A has 100 AP.
  • Player A wants to add 5 AP into INT.
  • Player A has fat fingers and hits both '5' and '6' and accidentally adds 56 AP.
  • Player A has network issues and accidentally taps '5' two times and adds 55 AP.

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested my changes
  • [] I have added unit tests that prove my changes work

Screenshots

@thebuilderbob1 thebuilderbob1 changed the title Refactored Stat Command Refactored the command classes that adds stats Aug 21, 2024
@MatthewHinds
Copy link

Are the config.yaml changes required for this PR? If so could you please explain the reason

@thebuilderbob1
Copy link
Author

no, disregard those. I set some options to login faster for testing

Copy link
Owner

@P0nk P0nk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, though some changes are required before I'll approve.

Comment on lines +13 to +16
@Override
public void execute(Client client, String[] params) {
super.execute(client, params);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of these 4 identical execute implementations in all the subclasses? It's already implemented in StatCommand.

Comment on lines +367 to +370
//addCommand("str", StatStrCommand.class);
//addCommand("dex", StatDexCommand.class);
//addCommand("int", StatIntCommand.class);
//addCommand("luk", StatLukCommand.class);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these comments and delete the old commands since they are useless now.

AssignStat(c, params, stat);
}

private void AssignStat(Client client, String[] params, Stat stat)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

camelCase method name is Java standard

Comment on lines 162 to 166
DB_URL_FORMAT: "jdbc:mysql://%s:3306/cosmic" # If the docker ENV for DB_HOST is anything but "db", this string format should be changed from 3306 to 3307 (or whichever port it was changed to in docker)
DB_HOST: "localhost"
DB_USER: "root"
DB_PASS: ""
DB_PASS: "root"
INIT_CONNECTION_POOL_TIMEOUT: 90 # Seconds
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to commit your own changes to config.yaml. Please revert all the changes to this file.

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.

3 participants