Skip to content

Conversation

@whimxiqal
Copy link

I have yet to delve into other files, really. I just did the command files to see how you like these types of changes.

  • Added Javadocs to every class type
  • Added Javadocs to most unobvious public and protected methods
  • Made modifiers more restrictive on methods and classes which shouldn't be accessed or overridden
  • Abstracted the two root commands to reduce redundancy and created extra root command methods
  • Slight, mostly inconsequential but IntelliJ-recommended edits to things like singleton lists, string building, stream iteration, or space-between-closure-character formatting

Copy link
Author

@whimxiqal whimxiqal left a comment

Choose a reason for hiding this comment

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

I have some thoughts about organizing the Portal and Destination classes without so many static components. See below.

*/
public class Portal {

private static List<Portal> portals = new ArrayList<>();
Copy link
Author

Choose a reason for hiding this comment

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

@jackah2 The Portal file gets a little too busy with these static fields and methods. While the behavior of these cached portals and destinations are basically static, I am of the mind that storage should be handled by storage classes, separate from any base object class to reduce confusion and clutter. I like what you did with the LocationSaver; maybe another portal/destination handler could be added as a field in Main and then all command classes and any future APIs can use Main's instance of the handler instead of using a bunch of static fields bundled into the Portal class.

It also doesn't really matter because I don't think there are plans to expand on this plugin so reducing clutter might not be important anyway. This is also my organizational opinion and maybe static fields are clearer.

Copy link
Member

Choose a reason for hiding this comment

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

I definitely agree that there is a need for abstraction here. In some of the other plugins, I use a manager with static fields to take care of creating, removing, and fetching portals.

private static MyConfig portalData;
private LocationSaver locationSaver;

public static Main getInstance() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not the biggest fan of statically accessing the main class. I know this is a bit contradictory given my comment about making Portal/Destination managers, but that's just the personal style I use.

Copy link
Author

Choose a reason for hiding this comment

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

The reason why I like it is for those managing classes can be accessed with that single static method, so there are practically no other public static fields and everything is retrieved from the same place, and for logging because I don't want to be five references removed from the Main class and have to pass the plugin through everything just to access the logger (or managers within Main). I guess it just depends on how many static fields you want, I'm generally for few as possible.

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