develop-chatReply #5

Merged
Pupsi merged 11 commits from develop-chatReply into master 2024-10-06 13:52:59 +00:00
Member

/r

/r
Pupsi added 8 commits 2024-10-06 10:33:09 +00:00
MineTec reviewed 2024-10-06 11:15:08 +00:00
@ -0,0 +25,4 @@
private final Map<Player, List<Conversation>> replyMapping = new WeakHashMap<>();
public void reply(Player sender, String message) {
if(this.replyMapping.get(sender) != null) {
Owner

hier wird ganz oft this.replyMapping.get(sender) verwendet, rausziehen in eine eigene variable z.B. replyMap = this.replyMapping.get(sender)

hier wird ganz oft `this.replyMapping.get(sender)` verwendet, rausziehen in eine eigene variable z.B. `replyMap = this.replyMapping.get(sender)`
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:16:26 +00:00
@ -0,0 +24,4 @@
private record Conversation(UUID target, Long lastSet) {}
private final Map<Player, List<Conversation>> replyMapping = new WeakHashMap<>();
public void reply(Player sender, String message) {
Owner

die null checks überall sind nicht schön, es macht vielleicht eher sinn am anfang einmal zu prüfen ob ein Eintrag vorhanden ist und wenn nicht ne leere Liste zu initialisieren..

this.replyMapping.computeIfNotPresent(() -> new ArrayList<>());

die null checks überall sind nicht schön, es macht vielleicht eher sinn am anfang einmal zu prüfen ob ein Eintrag vorhanden ist und wenn nicht ne leere Liste zu initialisieren.. `this.replyMapping.computeIfNotPresent(() -> new ArrayList<>());`
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:21:22 +00:00
@ -0,0 +44,4 @@
Component currentTargetComponent = Component.text("niemandem.");
Player currentTargetPlayer = Bukkit.getPlayer(youngestEntry.target());
if(currentTargetPlayer != null) {
currentTargetComponent = Main.instance().getAppliance(ChatMessages.class).getReportablePlayerName(currentTargetPlayer);
Owner

initialisierung und sofortig bedingtes überschreiben würde ich als bad practice einstufen...

Umschreiben zu

Component currentTargetComponent = currentTargetPlayer != null
    ? Main.instance().getAppliance(ChatMessages.class).getReportablePlayerName(currentTargetPlayer)
    : Component.text("niemandem.");
initialisierung und sofortig bedingtes überschreiben würde ich als bad practice einstufen... Umschreiben zu ``` Component currentTargetComponent = currentTargetPlayer != null ? Main.instance().getAppliance(ChatMessages.class).getReportablePlayerName(currentTargetPlayer) : Component.text("niemandem."); ```
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:24:05 +00:00
@ -0,0 +60,4 @@
if(target == null) throw new ApplianceCommand.Error("Der Spieler " + Bukkit.getOfflinePlayer(youngestEntry.target()).getName() + " ist nicht mehr verfügbar.");
this.replyMapping.get(sender).clear();
sendWhisper(sender, new ResolvedPmUserArguments(target, message));
Owner

this.sendWhisper

this.sendWhisper
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:25:50 +00:00
@ -0,0 +64,4 @@
return;
}
sender.sendMessage("");
Owner

das senden einzelner messages ist problematisch...

in der theorie könnten andere messages welche parallel gesendet werden so zwischen die messages rutschen. Außerdem ist jeder sendMessage aufruf ein eigenes paket, welches an den Spieler gesendet wird.

die Aufrufe sollten zusammengezogen werden zu einem einzelnen aufruf, leerzeilen können mit .appendNewLine eingefügt werden.

Hier bietet sich ggf der ComponentBuilder an, dieser kann mit Component.text() erstellt werden.

Anschließend kann darauf mit component.append(otherComponent) gearbeitet werden. Zum schluss dann sender.sendMessage(component.build())

das senden einzelner messages ist problematisch... in der theorie könnten andere messages welche parallel gesendet werden so zwischen die messages rutschen. Außerdem ist jeder sendMessage aufruf ein eigenes paket, welches an den Spieler gesendet wird. die Aufrufe sollten zusammengezogen werden zu einem einzelnen aufruf, leerzeilen können mit `.appendNewLine` eingefügt werden. Hier bietet sich ggf der ComponentBuilder an, dieser kann mit `Component.text()` erstellt werden. Anschließend kann darauf mit `component.append(otherComponent)` gearbeitet werden. Zum schluss dann `sender.sendMessage(component.build())`
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:29:16 +00:00
@ -0,0 +70,4 @@
.append(privatePrefix)
.append(Component.text("Das Ziel für /r hat sich bei dir in den letzten ", NamedTextColor.RED))
.append(Component.text(String.valueOf(this.targetChangeTimeoutSeconds), NamedTextColor.RED))
.append(Component.text(" Sekunden geändert. Wer soll deine Nachricht bekommen? ", NamedTextColor.RED))
Owner

"erhalten" statt "bekommen" klingt denke ich besser

"erhalten" statt "bekommen" klingt denke ich besser
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:31:04 +00:00
@ -0,0 +74,4 @@
);
List<Conversation> oldConversations = this.replyMapping.get(sender).stream()
.filter(conversation -> conversation.lastSet < System.currentTimeMillis() - (targetChangeTimeoutSeconds*1000))
Owner

hier steht die gleiche logik wie in zeile 58

lässt sich das vielleicht schlau kombinieren, dass du oldConversations schon oben erstellst und die andere prüfung auf der bestehenden Liste dann ausführst?

hier steht die gleiche logik wie in zeile 58 lässt sich das vielleicht schlau kombinieren, dass du oldConversations schon oben erstellst und die andere prüfung auf der bestehenden Liste dann ausführst?
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:32:14 +00:00
@ -0,0 +77,4 @@
.filter(conversation -> conversation.lastSet < System.currentTimeMillis() - (targetChangeTimeoutSeconds*1000))
.toList();
Conversation youngestOldConversation;
Owner

gibt es einen Grund warum die Variable hier außerhalb allokiert wird aber nur innerhalb des if statements genutzt wird?

gibt es einen Grund warum die Variable hier außerhalb allokiert wird aber nur innerhalb des if statements genutzt wird?
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:35:41 +00:00
@ -0,0 +91,4 @@
.distinct()
.toList();
final Component[] finalComponent = {Component.text("")};
Owner

(ㆆ _ ㆆ)

(ㆆ _ ㆆ)
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:40:32 +00:00
@ -0,0 +93,4 @@
final Component[] finalComponent = {Component.text("")};
playerNames.forEach(playerName -> finalComponent[0] = finalComponent[0].append(
Owner

wenn du einen weg findest finalComponent nicht überschreiben zu müssen brauchst du den array quark nicht.

ComponentBuilder<TextComponent, TextComponent.Builder> component = Component.text();
                            playerNames.forEach(playerName -> component.append(
                                        Component.text("[")
                                        .append(Component.text(playerName, NamedTextColor.GOLD))
                                        .append(Component.text("]"))
                                        .clickEvent(ClickEvent.runCommand("/msg " + playerName + " " + message))
                                        .hoverEvent(HoverEvent.showText(Component.text("Klicke, um diesem Spieler zu schreiben.").color(NamedTextColor.GOLD))))
                                .append(Component.text("  ")));

                            sender.sendMessage("");
                            sender.sendMessage(component.build());
                            sender.sendMessage("");

wenn du einen weg findest finalComponent nicht überschreiben zu müssen brauchst du den array quark nicht. ``` ComponentBuilder<TextComponent, TextComponent.Builder> component = Component.text(); playerNames.forEach(playerName -> component.append( Component.text("[") .append(Component.text(playerName, NamedTextColor.GOLD)) .append(Component.text("]")) .clickEvent(ClickEvent.runCommand("/msg " + playerName + " " + message)) .hoverEvent(HoverEvent.showText(Component.text("Klicke, um diesem Spieler zu schreiben.").color(NamedTextColor.GOLD)))) .append(Component.text(" "))); sender.sendMessage(""); sender.sendMessage(component.build()); sender.sendMessage(""); ```
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:40:54 +00:00
@ -0,0 +103,4 @@
sender.sendMessage("");
sender.sendMessage(finalComponent[0]);
sender.sendMessage("");
Owner

.appendNewline()

`.appendNewline()`
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:41:37 +00:00
@ -0,0 +109,4 @@
public void sendWhisper(Player sender, ResolvedPmUserArguments userArguments) {
if(!(this.replyMapping.get(userArguments.receiver) == null)) {
Owner

klammer weg und != verwenden

klammer weg und `!=` verwenden
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:42:49 +00:00
MineTec reviewed 2024-10-06 11:43:49 +00:00
@ -0,0 +121,4 @@
System.currentTimeMillis()
);
if(this.replyMapping.get(userArguments.receiver) == null) {
Owner

hier kann durchaus ein else verwendet werden beim if im zusammenhang mit dem vorhergehenden if

hier kann durchaus ein `else` verwendet werden beim if im zusammenhang mit dem vorhergehenden if
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:45:42 +00:00
@ -0,0 +147,4 @@
);
ChatMessages chatMessages = Main.instance().getAppliance(ChatMessages.class);
Owner

wird in der reply methode auch aufgerufen, macht ggf sinn die chatMessages referenz als objektvariable zu halten.

wird in der reply methode auch aufgerufen, macht ggf sinn die chatMessages referenz als objektvariable zu halten.
MineTec marked this conversation as resolved
MineTec reviewed 2024-10-06 11:46:21 +00:00
@ -0,0 +97,4 @@
Component.text("[")
.append(Component.text(playerName, NamedTextColor.GOLD))
.append(Component.text("]"))
.clickEvent(ClickEvent.runCommand("/msg " + playerName + " " + message))
Owner

String.format

String.format
Pupsi marked this conversation as resolved
MineTec reviewed 2024-10-06 11:48:00 +00:00
@ -0,0 +2,4 @@
import java.util.ArrayList;
public class LimitedSizedList<T> extends ArrayList<T> {
Owner

wenn die Klasse nicht mehr verwendet wird kann sie raus

wenn die Klasse nicht mehr verwendet wird kann sie raus
Pupsi marked this conversation as resolved
MineTec requested changes 2024-10-06 11:49:54 +00:00
Dismissed
MineTec left a comment
Owner

bot

bot
Pupsi added 1 commit 2024-10-06 13:23:44 +00:00
MineTec reviewed 2024-10-06 13:25:52 +00:00
@ -0,0 +34,4 @@
.toList();
this.replyMapping.get(sender).removeAll(tooOldConversations);
List<Conversation> replyMap = this.replyMapping.get(sender);
Owner

das kann noch über tooOldConversations und dort verwendet werden, auch bei dem removeAll

das kann noch über tooOldConversations und dort verwendet werden, auch bei dem removeAll
MineTec marked this conversation as resolved
MineTec approved these changes 2024-10-06 13:28:22 +00:00
Dismissed
Pupsi added 1 commit 2024-10-06 13:50:22 +00:00
MineTec approved these changes 2024-10-06 13:51:39 +00:00
Pupsi added 1 commit 2024-10-06 13:52:21 +00:00
Pupsi merged commit 77fbc12873 into master 2024-10-06 13:52:59 +00:00
Pupsi deleted branch develop-chatReply 2024-10-06 13:53:27 +00:00
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: CraftAttack/spawn#5
No description provided.