GH-1192 Extend /back functionality to per-death and per-teleport

[Jakubk15]

Described in #1133

Resolves #1133

[gemini-code-assist]

Code Review

This pull request extends the /back command to differentiate between death and teleport locations, which is a great feature enhancement. The implementation is mostly solid, but I've found a critical issue in the configuration migration that could lead to data loss for users. Additionally, there are several opportunities for refactoring to reduce code duplication and improve readability, particularly in BackCommand and BackService. I've also noted a design consideration regarding a shared configuration setting that could be improved.

[gemini-code-assist]

This method's logic can be simplified by using Optional's functional style methods, making the code more concise and readable. This pattern can also be applied to executeBackTeleport, and both could be extracted into a single helper method to reduce code duplication.

    public void executeBackDeath(@Sender Player player) {
        this.backService.getBackLocationPair(player.getUniqueId())
            .map(Pair::getFirst)
            .filter(java.util.Objects::nonNull)
            .ifPresentOrElse(deathLocation -> {
                this.backService.teleportBack(player, deathLocation.location());
                this.noticeService.player(player.getUniqueId(), translation -> translation.back().teleportedToLastLocation());
            }, () -> this.noticeService.player(player.getUniqueId(), translation -> translation.back().lastLocationNoExist()));
    }

[Jakubk15]

doesn't look simplified at all

[gemini-code-assist]

This method contains logic that is very similar to executeBackTeleportOther and could be refactored to reduce duplication. Additionally, using Optional's functional methods can make the implementation cleaner and more readable.

    public void executeBackDeathOther(@Sender Viewer viewer, @Arg Player target) {
        this.backService.getBackLocationPair(target.getUniqueId())
            .map(Pair::getFirst)
            .filter(java.util.Objects::nonNull)
            .ifPresentOrElse(deathLocation -> {
                this.backService.teleportBack(target, deathLocation.location());
                this.noticeService.player(target.getUniqueId(), translation -> translation.back().teleportedToLastLocation());

                this.noticeService.create()
                    .viewer(viewer)
                    .notice(translation -> translation.back().teleportedSpecifiedPlayerLastLocation())
                    .placeholder("{PLAYER}", target.getName())
                    .send();
            }, () -> this.noticeService.viewer(viewer, translation -> translation.back().lastLocationNoExist()));
    }

[gemini-code-assist]

The logic in this method is a bit complex and can be hard to follow. It can be refactored to be more straightforward and readable, which improves maintainability.

    public void setBackLocation(UUID playerId, Location location, boolean isFromDeath) {
        Pair<BackLocation, BackLocation> pair = Optional.ofNullable(this.backLocationsCache.getIfPresent(playerId))
            .orElse(Pair.of(null, null));

        BackLocation newLocation = new BackLocation(location);

        if (isFromDeath) {
            this.backLocationsCache.put(playerId, Pair.of(newLocation, pair.getSecond()));
        }
        else {
            this.backLocationsCache.put(playerId, Pair.of(pair.getFirst(), newLocation));
        }
    }

[gemini-code-assist]

The teleport delay for the /back command uses settings.tpaTimer(), which is a setting from the teleport request (/tpa) feature. It would be better to introduce a dedicated configuration setting for the /back command's teleport delay, for example backTimer. This would give server administrators more granular control over teleport delays for different features.

[imDMK]

I don't like this class. I would suggest something like this;

Suggested change

	public class BackCommand {
	import com.eternalcode.annotations.scan.command.DescriptionDocs;
	import com.eternalcode.core.feature.back.BackService;
	import com.eternalcode.core.feature.back.BackService.BackLocation;
	import com.eternalcode.core.injector.annotations.Inject;
	import com.eternalcode.core.notice.NoticeService;
	import com.eternalcode.core.viewer.Viewer;
	import dev.rollczi.litecommands.annotations.argument.Arg;
	import dev.rollczi.litecommands.annotations.command.Command;
	import dev.rollczi.litecommands.annotations.context.Sender;
	import dev.rollczi.litecommands.annotations.execute.Execute;
	import dev.rollczi.litecommands.annotations.permission.Permission;
	import org.bukkit.entity.Player;
	import panda.std.Pair;
	import java.util.Optional;
	import java.util.UUID;
	import java.util.function.Function;
	@Command(name = "back")
	public class BackCommand {
	private static final String PLAYER_PLACEHOLDER = "{PLAYER}";
	private final BackService backService;
	private final NoticeService noticeService;
	@Inject
	public BackCommand(BackService backService, NoticeService noticeService) {
	this.backService = backService;
	this.noticeService = noticeService;
	}
	@Execute(name = "death")
	@Permission("eternalcore.back.death")
	@DescriptionDocs(description = "Teleport to your last death location")
	public void executeBackDeath(@Sender Player player) {
	teleportSelf(player, this::pickDeath);
	}
	@Execute(name = "teleport")
	@Permission("eternalcore.back.teleport")
	@DescriptionDocs(description = "Teleport to your last teleport location")
	public void executeBackTeleport(@Sender Player player) {
	teleportSelf(player, this::pickTeleport);
	}
	@Execute(name = "death")
	@Permission("eternalcore.back.death.other")
	@DescriptionDocs(description = "Teleport specified player to their last death location", arguments = "<player>")
	public void executeBackDeathOther(@Sender Viewer viewer, @Arg Player target) {
	teleportOther(viewer, target, this::pickDeath);
	}
	@Execute(name = "teleport")
	@Permission("eternalcore.back.teleport.other")
	@DescriptionDocs(description = "Teleport specified player to their last teleport location", arguments = "<player>")
	public void executeBackTeleportOther(@Sender Viewer viewer, @Arg Player target) {
	teleportOther(viewer, target, this::pickTeleport);
	}
	private void teleportSelf(Player player, Function<Pair<BackLocation, BackLocation>, BackLocation> selector) {
	resolveBack(player.getUniqueId(), selector).ifPresentOrElse(
	loc -> {
	this.backService.teleportBack(player, loc.location());
	this.noticeService.player(player.getUniqueId(),
	t -> t.back().teleportedToLastLocation());
	},
	() -> this.noticeService.player(player.getUniqueId(),
	t -> t.back().lastLocationNoExist())
	);
	}
	private void teleportOther(Viewer viewer, Player target, Function<Pair<BackLocation, BackLocation>, BackLocation> selector) {
	resolveBack(target.getUniqueId(), selector).ifPresentOrElse(
	loc -> {
	this.backService.teleportBack(target, loc.location());
	this.noticeService.player(target.getUniqueId(),
	t -> t.back().teleportedToLastLocation());
	this.noticeService.create()
	.viewer(viewer)
	.notice(t -> t.back().teleportedSpecifiedPlayerLastLocation())
	.placeholder(PLAYER_PLACEHOLDER, target.getName())
	.send();
	},
	() -> this.noticeService.viewer(viewer, t -> t.back().lastLocationNoExist())
	);
	}
	private Optional<BackLocation> resolveBack(UUID playerId, Function<Pair<BackLocation, BackLocation>, BackLocation> selector) {
	return this.backService.getBackLocationPair(playerId)
	.map(selector)
	.filter(loc -> loc != null);
	}
	private BackLocation pickDeath(Pair<BackLocation, BackLocation> pair) {
	return pair.getFirst();
	}
	private BackLocation pickTeleport(Pair<BackLocation, BackLocation> pair) {
	return pair.getSecond();
	}
	}

[imDMK]

Suggested change

	public void setBackLocation(UUID playerId, Location location, boolean isFromDeath) {
	Pair<BackLocation, BackLocation> existing = backLocationsCache.getIfPresent(playerId);
	BackLocation newLocation = new BackLocation(location);
	BackLocation deathLocation;
	BackLocation normalLocation;
	if (existing == null) {
	deathLocation = isFromDeath ? newLocation : null;
	normalLocation = isFromDeath ? null : newLocation;
	}
	else {
	deathLocation = isFromDeath ? newLocation : existing.getFirst();
	normalLocation = isFromDeath ? existing.getSecond() : newLocation;
	}
	backLocationsCache.put(playerId, Pair.of(deathLocation, normalLocation));
	}
	public void setBackLocation(UUID playerId, Location location, boolean fromDeath) {
	Pair<BackLocation, BackLocation> cached = backLocationsCache.getIfPresent(playerId);
	BackLocation newLoc = new BackLocation(location);
	BackLocation deathLoc = (cached != null) ? cached.getFirst() : null;
	BackLocation normalLoc = (cached != null) ? cached.getSecond() : null;
	if (fromDeath) {
	deathLoc = newLoc;
	} else {
	normalLoc = newLoc;
	}
	backLocationsCache.put(playerId, Pair.of(deathLoc, normalLoc));
	}

[imDMK]

Move to private static variable