HellsBells #275
Keine Reviewer
Label
Kein Label
Bug
Codeverbesserung
Einsteiger Freundlich
Idee
In Arbeit
Neues Feature
Prio A
Security Breach
Überprüfung notwendig
Verbesserung
Zu Beobachten
Kein Meilenstein
Niemand zuständig
3 Beteiligte
Fällig am
Kein Fälligkeitsdatum gesetzt.
Abhängigkeiten
Keine Abhängigkeiten gesetzt.
Referenz: SteamWar/FightSystem#275
Laden…
In neuem Issue referenzieren
Einen Benutzer sperren
Keine Beschreibung angegeben.
Branch "Hells_bells" löschen
Das Löschen eines Branches ist permanent. Obwohl der Branch für eine kurze Zeit weiter existieren könnte, kann diese Aktion in den meisten Fällen NICHT rückgängig gemacht werden. Fortfahren?
Hells_bellszu HellsBells@ -106,6 +107,9 @@ public class FightSystem extends JavaPlugin {
new RankedPlayerLeftWincondition();
new WinconditionPercentTimeout();
//noinspection InstantiationOfUtilityClass
Bitte rausnehmen (sollte auch nicht nötig sein?!?)
@ -0,0 +23,4 @@
public static final Random random = new Random();
private enum Direction {
Du kannst dafür nicht BlockFace verwenden?
In der 1.8 gibt es BlockFace mit dem toVector intern noch nicht.
@ -0,0 +71,4 @@
@Override
public void enable() {
if (event != null) {
event.terminate();
In der enable() ist sichergestellt, dass es noch nie ausgeführt wurde oder vorher die disable() ausgeführt wurde, die Überprüfung sollte demnach unnötig sein.
@ -0,0 +79,4 @@
@Override
public void disable() {
if (event != null) {
Es ist garantiert, dass vor der disable() die enable() ausgeführt wurde.
@ -0,0 +87,4 @@
}.register();
}
private static class HellsBellsEvent {
Diese Unterklasse scheint mir ziemlich unnötig, die Inhalte dieser Klasse sollten in die übergeordnete Klasse eingebaut werden können.
@ -0,0 +110,4 @@
startCountdown();
}
public void terminate() {
init() und terminate() lesen sich mir wie eine enable() und disable() eines StateDependent...
@ -0,0 +111,4 @@
}
public void terminate() {
current = State.NONE;
Wird der State NONE wirklich benötigt?
@ -0,0 +128,4 @@
}
currentDrops++;
currentCountdown = new HellsBellsCountdown(current.MIN_TIME + random.nextInt(current.MAX_TIME - current.MIN_TIME), SWSound.BLOCK_NOTE_BASS, true);
Gibt es nicht random.nextInt(MIN, MAX)? (kA)
Nein leider bietet die normale Random implementation dies nicht an.
@ -0,0 +139,4 @@
AtomicInteger length = new AtomicInteger(10 + random.nextInt((direction.zLength ? zLength : xLength) - 10));
int width = 5 + random.nextInt(5);
int xOffset = getWidthStart(direction.zLength ? xLength : zLength, direction.zLength ? length.get() : width);
int zOffset = getLengthStart(direction.zLength ? zLength : xLength, direction.zLength ? width : length.get());
Könnte man statt diesen Ifs nicht einfach eine Methode in der Direction machen, die dann den passenden Wert zurückgibt?
@ -0,0 +145,4 @@
Point redStart;
Point blueStart;
if (direction == Direction.NORTH || direction == Direction.WEST) {
Auch hier müsste das zu einer Methode in Direction umwandelbar sein.
@ -0,0 +153,4 @@
blueStart = new Point(Config.BluePasteRegion.getMaxX() - xOffset, Config.BlueExtendRegion.getMaxY() + yOffset, Config.BluePasteRegion.getMaxZ() - zOffset);
}
new BukkitRunnable() {
Üblicherweise verwenden wir eher Bukkit...getScheduler().runTaskTimer(Plugin, lambda-Methode, Zeit(en))
@ -0,0 +189,4 @@
private class HellsBellsCountdown extends Countdown {
public HellsBellsCountdown(int time, SWSound sound, boolean level) {
Ich glaube, du verwendest immer dieselben Parameter (bis auf die variable Zeit), daher kannst du den Sound und Level direkt im Konstruktor setzen und musst das nicht übergeben...
@ -0,0 +206,4 @@
}
}
private static class Point {
Warum geht es nicht, Location oder Vector zu verwenden?
Weil Location und Vektor zu mehr undurchsichtigem code führt
@ -0,0 +1,271 @@
package de.steamwar.fightsystem.event;
License fehlt.
@ -0,0 +42,4 @@
public static final Random random = new Random();
private enum Direction {
Subklassen bitte nach dem Inhalt der Hauptklasse.
@ -0,0 +48,4 @@
EAST(1, 0, 0),
WEST(-1, 0, 0);
static final Direction[] DIRECTIONS = values();
Warum das nochmal extra vorhalten? In getRandom kann ja auch values() aufgerufen werden...
values() auf einem Enum erzeugt immer eine kopie der Enum Elemente intern, daher recht inefficient.
Ich finde, es ist ineffizienter, eine Kopie ständig im RAM zu halten, als alle x Minuten mal eine neue Kopie des 4! Elemente großen Array anzulegen. Ist zudem kürzer und einfacher, einfach neu zu kopieren.
@ -0,0 +93,4 @@
}
public HellsBells() {
new StateDependent(Winconditions.HELLS_BELLS, FightState.Running) {
Das StateDependent sollte nur die Sachen (de-)aktivieren, nicht das ganze Behaviour innehaben. Um die Nestung zu verringern: Nimm den ganzen Code bis auf die Enable und Disable und bewege den in die HellsBells-Klasse. (Also die übergeordnete)
@ -0,0 +135,4 @@
Point redStart;
Point blueStart;
if (direction.isNorthOrWest()) {
Diesen if-Block kann man denke ich vereinfachen, wenn man stattdessen in den Assignments mit dem conditional-Operator arbeitet ?:.
@ -0,0 +145,4 @@
currentDropping = Bukkit.getScheduler().runTaskTimer(FightSystem.getPlugin(), () -> {
for (int w = 0; w < width; w++) {
if (direction.isNorthOrWest()) {
Stat dem If-Block mit Subtract and add: Alle Werte mit
int factor = direction.isNorthOrWest() ? 1 : -1;
multiplizieren.@ -0,0 +204,4 @@
}.register();
}
private static class Point {
Ich weiß nicht, aber ich glaube, dafür, dass an einer Stelle mal ein subtract und ein add ausgeführt wird (generalisierbar mit * +-1) ist es etwas overkill, eine eigene Klasse dafür vorzuhalten, selbst wenn Location in der 1.8 (glaube war der Grund) eine .add o.ä. Methode bereitstellt, ist etwas overkill. Dann hat man halt beim Aufruf etwas (mehr) Arithmetik.
Schaut jetzt schon mal ganz gut aus, in dieser Fassung getestet?
@ -0,0 +138,4 @@
}.register();
}
class HellsBellsCountdown extends Countdown {
Kann glaube noch private static sein.