Code cleanup/rewrite possible for certain parts of the plugin?

[Technoguyfication]

Hello! I've been using the TARDIS plugin since 2014 and I'm in awe at how many awesome features keep getting added, even to this day. However, after doing some major server upgrades and digging through some of the code, I've noticed it looks like a lot of features could be modernized or rewritten. I understand that when this plugin was created, Bukkit was an entirely different API and Minecraft was a very different game. A lot of new programming practices and technologies have also been created since then.

Through time a lot of of the plugin's code has been updated, but as with any project, when more and more features get added everything has the tendency to turn into spaghetti. A lot of the bugs I've encountered look like the kind of bugs that come to exist indirectly - when new features are added they don't (and realistically can't) take into account all the effects they can have on other parts of the codebase.

I'd be interested in trying to take a few parts of the plugin and rewrite them to be more stable and follow more strict OOP principles. I was thinking I'd start with the junk TARDIS as a small piece and see how it goes, pretty much just rewrite the feature from the ground up. I'd test it extensively and PR it and see how it goes.

Some of the other things that I think could be improved on aren't as simple though, namely the database and TIPS. As far as the database goes, I think it'd be very helpful to use a migrations system or ORM framework to make changes to the database schema rather than a big list of CREATE TABLE IF NOT EXISTS... every time the plugin starts. However, everything in the plugin needs the database, so it's a very very big task to make any non-backwards-compatible changes to it. I also had some ideas for ways to improve the algorithm TIPS uses to partition out TARDIS interiors, but that's almost impossible to change on existing servers due to TARDISes already relying on the previous locations TIPS gave them.

Anyways, I love this plugin and so do my players, but running a TARDIS server has been a little.. challenging for a variety of reasons. I'm also not really sure if it'd be worth it to clean up certain parts of the plugin, like how many people realistically use the junk TARDIS anyways? Are there any sort of metrics about how many servers and players are using the TARDIS plugin? It could also be cool to add some anonymous feature-specific metrics about how many people use certain things like the junk TARDIS, rechargers, etc.

Thanks for reading this far, let me know if you agree with any of this or if I'm just a crazy person lol

[eccentricdevotion]

Lol, not crazy, most of the TARDIS code I would guess is pretty terrible. Comes from never actually being taught any programming and just learning it all myself as needed, I only started coding in Java because my son was playing Minecraft - TARDIS was my second or third ever plugin. I'd love to go back to school/uni and do it properly. TARDIS would make a good project to recode efficiently. Definitely a bit old for that now, maybe when I retire...

I used to collect metrics on TARDIS but removed the code years ago. Maybe it's time to add some back.

I reckon there's a good chunk of TARDIS features that hardly anyone uses, just today when adding some Javadoc comments I had to look up what some methods were actually for - had quite a few "oh yeah" moments where I had completely forgotten about some things that you could do.

While the code is getting a bit shabby, I don't consider it broken enough to do big rewrites (nor do I really have the time), so if you want to have a go, feel free. I'm happy to update to new Minecraft/Spigot versions and fix bugs as they are reported, and pop in a new feature of two as the mood takes me :)

[DalekCraft2]

Damn. I was almost ready to test stuff.

[Chew]

Me who dropped that absolutely massive PR to the gh-pages branch 👀

[DalekCraft2]

oh so i sort of have a small-ish chance then
neat

[DalekCraft2]

Sort of relevant to code, so I will post here;

Could TARDIS and TARDISChunkGenerator be combined if every Postworld asset from the main plugin was moved to a sort of "TARDIS plugin support" addon?

Also, could code from other TARDIS addons be kept only in those addons, instead of doing things like having bits of WeepingAngels code in the main plugin?

[eccentricdevotion]

I'm not sure I see any benefit in doing that - seems like a lot of work for no real gain - you'd just be moving code from one plugin to another - in my mind TCG is the "support plugin". Gonna need some convincing...

[eccentricdevotion]

Don't know that the Junk TARDIS metric is very useful, it just shows that it is enabled in the config...
Be more interesting to see how many uses it actually gets, but I guess we'd need to record that somewhere.

What custom stats do we want to track?

[Technoguyfication]

Hmm, that's a good question. If it's possible to track events as they happen, that would be great. Something like Junk TARDIS uses could be displayed on a line graph, similar to the player count.

Other stats I can think of off the top of my head are:

• Most popular consoles / chameleon presets
• Whether the server is using TIPS or creating a world for each tardis
• End / Nether travel enabled
• Total number tardises on the server, with an extra stat for how many are abandoned
• How many servers/players use hard or easy mode
• Most condensed blocks
• Most commonly grown rooms
• Median number of ARS rooms per TARDIS
• How often each type of travel is enabled/used (saves, biome, cave, village, etc.)

Obviously all those are just ideas, probably kind of overkill but I personally love statistics, haha.

[Technoguyfication]

@eccentricdevotion Are you in the community discord server by any chance? I had some ideas about restructuring some big parts of the code to make writing new features and maintaining existing code way easier, but seeing how big the codebase is, I'd want to talk about some of that stuff before writing it and trying to get it merged haha

[eccentricdevotion]

@Technoguyfication Sorry, I don't use Discord, just post here.

[Technoguyfication]

I was thinking it might make sense to restructure some of the code to provide better abstraction and encapsulation. For example, instead of querying the database each time we need to fetch the data about someone's TARDIS, it could be structured something like this:

public void onCommand(CommandSender sender, Command cmd, String label, String[] args) {
	// fetch TARDIS from database
	// this method queries the database in the background and parses all
	// the plaintext values into the TARDIS object, which contains methods
	// and members for anything we need to interact with a tardis.
	// ex. travelling, setting artron amount, dematerializing, power on/off, etc.
	// it can also store this TARDIS object in a hashmap so the database doesn't need
	// to be queried again to look up this tardis
	// cache expiration can be implemented to free up memory eventually
	TARDIS tardis = TARDIS.get(sender);
	
	// tardis will be null if a record wasn't found for this tardis
	if (tardis == null) {
		sender.sendMessage("You don't have a TARDIS!");
		return;
	}

	// magic method that finds the location based on the command args
	Location loc = findLocation(args);

	// this sets the TARDIS' next location, updating the database automatically
	tardis.setNextLocation(loc);

	// prompt user to release handbrake
	sender.sendMessage("Release the handbrake!");

	// and we're done! no resultsets or insert statements here. All the ugly database code
	// goes inside our TARDIS class, or if we want an even better layer of abstraction,
	// we put the query code inside a Database class and expose methods that the
	// TARDIS class will use, like Database#fetchTardis:ResultSetTardis and
	// Database:updateTardis(TARDIS):void to update a TARDIS record in the database
}

// example of what a simple TARDIS class might look like
public class TARDIS {
	private int _artronLevel;
	private boolean _handbrakeOn;
	private OfflinePlayer _owner;
	private TARDISState _currentState;
	private ChameleonCircuit _chameleon;

	private Location _lastLocation;
	private Location _currentLocation; // can be null, in the case of the TARDIS floating in the vortex
	private Location _nextLocation;	// can be null, when the TARDIS has no destination set

	// static references
	private static Database _database;
	private static TARDISPlugin _plugin;

	// ## static methods ##

	// initialize the class. because the methods for fetching TARDISes are static we don't need
	// to hold an object reference to use them. We do, however, need to add references to the
	// database and plugin before any other parts of the code call those static methods
	public static void initialize(TARDISPlugin plugin, Database database) {
		this._plugin = plugin;
		this._database = database;
	}

	public static TARDIS getTardis(OfflinePlayer player) {
		ResultSetTardis rs = Database.fetchTardisData(player.getUuid());

		// no records found
		if (!rs.resultSet()) {
			return null;
		}

		// create a new TARDIS object and load values into it
		TARDIS tardis = new TARDIS(rs);

		// optionally add this TARDIS to a cache so we can return it quicker next time

		// return the new TARDIS instance
		return tardis;
	}


	// ## instance methods ##

	TARDIS(ResultSetTardis rs) {
		// load values from the resultset into this instance
		this._artronLevel = rs.artronLevel();
		this._handbrakeOn = rs.handbrakeOn();

		// ...and repeat, you get the point
	}

	// sets the next location of the tardis
	public void setNextLocation(Location loc) {
		_nextLocation = loc;
	}

	// dematerializes the TARDIS and rematerializes it in the next location
	// useEnergy is used to determine whether to subtract from the TARDIS'
	// artron supply for this move (False in the case of admin /tsudo travel)
	public void travelToNextLocation(boolean useEnergy, void callback) {
		// i'm not very experienced with the bukkit scheduler or callbacks in java so this should be treated largely as pseudocode

		try {
			// start dematerializing the TARDIS.
			// the callback function executes once the animation is done, time taken depends on throttle setting
			this.dematerialize(() -> {

				// start rematerializing the tardis at the next location.
				this.materialize(_nextLocation, () -> {

					// run the callback function to tell the caller we're done moving
					callback();
				});	
			});
		} catch (Exception ex) {
			// revert to a safe state in the event of an error. send the tardis home, tell the player
			// what happened, etc.
		}
	}

	public void dematerialize(void callback) {
		this._currentState = TARDISState.IN_VORTEX;

		Database.updateTardis(This);	// update any changed values in database (like _currentState)
		Database.addTardisToVortex(this);	// add this to the list of tardises floating in the vortex
		
		// start the demat animation and callback when done
		this._chameleon.dematerialize(() -> {
			callback();
		});
	}

	// materialize() would be largely the same as dematerialize() but in reverse
}

It focuses heavily on keeping the database query code contained in a single place, reducing the likelihood of database corruption and making maintaining the code easier. It also saves programmer time not having to write out database queries every time (Just use TARDIS.getTardis(player) each time!) and can potentially save a ton of CPU cycles if we cache the result and don't query the database as much.

It also focuses on encapsulating code behavior as much as possible, which can help prevent the plugin from reaching an invalid state, since in most cases you just call one method to do what you want (travelling, hiding, rebuilding, etc.) and the TARDIS class figures it out from there.

This is a pretty ambitious idea, but it might help a lot with preventing weird bugs from cropping up and should make writing new features much easier if we provide ourselves with a good API for making calls across major parts of the program.

[Chew]

On the subject of this, I ran a global inspection and IntelliJ spit out several thousand "warnings".

Notably, it wanted to replace all switch statements with enchanted switch expressions or whatever the other term is.

It also pointed out that the for loop in TARDISRandomTheEnd never actually loops, because it returns on the first run here: https://github.com/eccentricdevotion/TARDIS/blob/v4.0/src/main/java/me/eccentric_nz/TARDIS/api/TARDISRandomTheEnd.java#L89

It also offered about 312 classes to be converted to records but warned that the local fields would become public instead of private (not sure if this is a huge issue).

Additionally, it offered blocks of code to be converted into native text blocks.

Finally, it suggested 2 instances of a string compare with ==s instead of .equals() and another change to inline casting.

Here is that commit that does most of the above minus records: Chew@d7d1c3d

[eccentricdevotion]

Java 16 suggestions...
I've already done a few 60ea3f2 amongst others
I quite like the enhanced switch expressions, but sometimes the older version is a lot more readable...

[eccentricdevotion]

@Chew Feel free to submit a PR