Update2.0 #22
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
4 Beteiligte
Fällig am
Kein Fälligkeitsdatum gesetzt.
Abhängigkeiten
Keine Abhängigkeiten gesetzt.
Referenz: SteamWar/MissileWars#22
Laden…
In neuem Issue referenzieren
Einen Benutzer sperren
Keine Beschreibung angegeben.
Branch "Update2.0" 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?
Closes: 23
Dieser PullRequest macht es nun möglich Support Items als JSON zu ermöglichen. Alle Support Items (Items, die bis jetzt drin sind) sind auch als JSON schon implementiert (auf dem server eingestellt). Somit sind nun Missiles wie Items gesoftcoded. Ich werde mich noch dran setzten und JavaDoc dazu schreiben, dass jeder weiß wie man es verwendet.
Wurde das ganze getestet (erfolgreich)?
Warum ist das als JSON implementiert und nicht, wie alles andere auch, als YAML?
Mit YAML hättest du zusätzlich die Möglichkeit, die Items und Skripte direkt zu (de-)serialisieren und damit wesentlich weniger Code-Aufwand.
Ja dies habe ich erfolgreich in auf meinem Bau Test Server getestet. Ich habe am Anfang kurz das ganze mal in YAML überlegt und auch ein wenig getestet, fande jedoch, dass das echt nicht schön zu schreiben war und auch für neue sachen nicht gerade es einfach gemacht hat was zu "Scripten", weswegen ich dann nach einiger Zeit mich für JSON entschieden habe. Ich finde auch, dass man ganz gut damit klar kommt und es gefühlt einfacher zu lesen ist, aber das kann auch nur subjektiv von mir kommen.
@ -0,0 +34,4 @@
import java.io.IOException;
import java.util.Objects;
public class Item extends SpecialItem {
Etwas generischer Name.
@ -0,0 +39,4 @@
private ScriptedItem scriptedItem;
public Item(File item) {
try {
Warum kann der kein JSONObject bekommen? Dann müsste der Try-Catch Block nur an einer Stelle sein.
@ -0,0 +41,4 @@
public Item(File item) {
try {
JsonObject jsonObject = new JsonParser().parse(new FileReader(item)).getAsJsonObject();
System.out.println(jsonObject);
Debug Nachricht?
@ -0,0 +33,4 @@
}
class RunnableScriptEvent {
Kann man dem nicht eine Eigene klasse geben?
@ -0,0 +49,4 @@
public Location getLocation() {
// Custom location
if (locationType == LocationType.CUSTOM && customLocation != null) {
Warum kann hier kein Switch Statement sein? Wäre glaube ich schöner an der Stelle.
Ich glaube dies nicht, weil es immer den Fallback auf Default geben soll. Somit ist es linearer
@ -0,0 +73,4 @@
}
public void setLocationType(LocationType locationType) {
if (locationType == null) return;
Dieser Check ist Doppelt, LocationScript z. 68.
@ -0,0 +40,4 @@
return null;
}
String type = jsonObject.getAsJsonPrimitive("type").getAsString();
switch (type.toLowerCase()) {
Könnte man nicht einen Enum, welcher ein RunnableScript speichert, und da dann mit dem Enum Namen drüber Iteriert?
geht nicht, weil die dann statisch währen und man enum values nicht überschreiben sollte
@ -0,0 +43,4 @@
public enum EventType {
onHit("onHit"),
onThrow("onThrow"),
onClick("onClick");
Warum werden dieser String Parameter benötigt, und kann man das nicht
name()
machen?kann man, aber der Enum name muss nicht mit dem Event Namen übereinstimmen
Tut es doch jetzt schon.
@ -0,0 +85,4 @@
if (jsonObject.has("lore")) {
List<String> lore = new ArrayList<>();
jsonObject.getAsJsonArray("lore").forEach(jsonElement -> {
if (!jsonElement.isJsonPrimitive()) {
Warum ist dieser Check nötig?
Weil man auch die Lore weglassen kann, diese ist somit optional
@ -0,0 +63,4 @@
try {
clipboard = Objects.requireNonNull(ClipboardFormats.findByFile(schemFile)).getReader(new FileInputStream(schemFile)).read();
} catch (IOException e) {
throw new SecurityException("Could not load shield", e);
Warum konnte er da das Schild nicht laden? Das wird doch von mehr Sachen als nur das Schild was etwas Pastet.
@ -0,0 +36,4 @@
private Summon summon = null;
public SummonScript(JsonObject summon) {
switch (summon.getAsJsonPrimitive("entity").getAsString().toLowerCase()) {
Warum ist hier dann ein Switch und kein If?
Um es später noch zu erweitern
@ -0,0 +1,10 @@
package de.steamwar.misslewars.scripts;
License Header Fehlt
@ -0,0 +1,58 @@
package de.steamwar.misslewars.scripts;
License Header Fehlt
@ -0,0 +19,4 @@
package de.steamwar.misslewars.scripts;
import com.google.gson.JsonArray;
Unnötiger Import
@ -0,0 +1,77 @@
package de.steamwar.misslewars.scripts.implemented;
License Header Fehlt
@ -0,0 +45,4 @@
}
private void resume() {
while (index < runnableScriptList.size()) {
Wäre hier nicht eher eine For oder Foreach Schleife angebracht.
?
Nein weil sie auch abgebrochen werden kann mit dem Delay und danach wieder angefangen wird. Somit ist beides keine Option.
@ -0,0 +49,4 @@
int blockZ = location.getBlockZ();
if (offset > 0) {
if (blockZ > bz - offset) return true;
if (blockZ < rz + offset) return true;
If Else oder ||?
@ -0,0 +52,4 @@
if (blockZ < rz + offset) return true;
} else {
if (blockZ < bz - offset) return true;
if (blockZ > rz + offset) return true;
Selbiges.
Ich denke, dass die Menge an Code + die Komplexität des Codes für die gewünschte Aufgabe weitaus zu viel ist. Du hast 1270 neue Zeilen, während du nur 270 alte dadurch weg wirst. Welche großen dynamischen Änderungen sind denn in nächster Zeit an MW zu erwarten?
Ich halte das ganze a) viel zu viel Overhead für etwas, was kaum benötigt werden wird und b) die Implementierung der Idee viel zu lang, bitte die benötigten Codezeilen und Komplexität dafür deutlich einstampfen, dass ist einfach schlecht wartbar, wenn ein kleines Feature so komplex ist.
Du möchtest doch sonst auch immer vieles softcoded haben, sodass man das einfach anpassen kann. Und deswegen habe ich mir gedacht, dass ich das ganze auch auf MissileWars anwende und mich auch hier darum kümmer, dass dies so wird.
Diese Änderung würde definitiv das erstellen von neuen Items einfacher machen und auch ohne viel Programmierkenntnise ermöglicht werden. Auch finde ich, dass man einfacher so Items und Balance Änderungen ermöglichen kann, ohne, dass man immer eine Code analyse machen muss.
Des weiteren kann man auch auf dem Dev Server dann mal eben ein neues Item testen, ohne den Code zu ändern oder Items anpassen/balancen ohne immer neu zu kompilieren.
Ich softcode da, wo es Sinn macht. Die simple Frage ist halt: Wie häufig müssen neue Items erstellt/alte Items gebalanct werden? Oder, um es noch weiter herunterzubrechen: Macht dieses Feature mehr oder weniger Aufwand?
Ich glaube, dass es weniger aufwand macht, da es den Prozess des Item Erstellen vereinfacht und vom Code loslöst. Des weiteren so die Idee (primär von MaybeCreative) ungefähr alle 2 Wochen (darf muss aber nicht Zitat: MaybeCreative) ein Update zu machen. Dies kann beinhalten, dass Items geändert werden, gelöscht werden, gebalancet werden, Missiles hinzukommen, gelöscht werden, angepasst werden. Dementsprechend glaube ich, dass dieses Feature uns das Leben einfacher macht und der Aufwand minimiert wird.
YoyoNow, ich glaube, wir haben gerade einfach mit unter anderem das Problem, dass sich unsere Führungsstile in die Wolle mit mir als "Anführer" und du als "designiertem Nachfolger" bekommen. Ich würde vorschlagen, dass wir einfach mal die Projekte/Plugins und wer darüber das aus technischer Sicht letzte Wort darüber hat mal aufteilen.
Da würde ich vorschlagen, dass du MissileWars, LobbySystem und BauSystem übernimmst.
Dann muss ich mich langsam daran gewöhnen, in den Projekten das letzte Wort dir zu überlassen (was nicht heißt, dass ich nicht mehr meine Meinung kundtun werde!) :).
Was hälst du davon?
Ich glaube, dass die Aufteilung, besonders was die Änderungen (wer wo primär arbeitet) angeht eine gute Idee ist. Ich möchte definitiv weiterhin deine Meinung dabei haben und auch diese bekommen. Ich finde es nämlich immer wieder interessant deine Meinung und auch Ideen zu hören und drüber nachzudenken. Ich hoffe, dass meine Meinung auch in anderen Bereichen Willkommen ist und zum nachdenken anregt.
Des weiteren würde ich gerne noch sagen, dass die Idee hier dran einfach war, Code (technische Umsetzung) von Implementierung zu trennen. Soll heißen, dass ich einfach den Code haben möchte, der es managed, und dann die Idee (Style sozusagen) von woanders kommt.
Wie schaut es hier mit aus @YoyoNow ?
Update2.0zu WIP: Update2.0WIP: Update2.0zu Update2.0@ -85,1 +89,4 @@
BlueSpawn = new Location(Bukkit.getWorlds().get(0), blue.getDouble("SpawnX"), blue.getDouble("SpawnY"), blue.getDouble("SpawnZ"), (float)blue.getDouble("SpawnYaw"), (float)blue.getDouble("SpawnPitch"));
String blueLeader = System.getProperty("blueLeader", null);
String redLeader = System.getProperty("redLeader", null);
In den derzeit geplanten änderungen zum Server System sollten wir eher weg von System Arguementen
Gut wie soll ich es sonst lösen, so ist es im FightSystem!
Da ist dann leider auch eine Änderung im BungeeCore und FightSystem nötig, da brauchen wir noch überhaupt ein Konzept, wie wir dann solche Argumente überhaupt auf den Server bekommen. Evtl. indem wir Configs anpassbar machen. Ich denke aber nicht, dass wir da hier jetzt schon mit dem Umbau beginnen sollten.
@ -19,6 +19,7 @@
package de.steamwar.misslewars;
import de.steamwar.misslewars.items.Missile;
Unused Import
@ -0,0 +24,4 @@
import de.steamwar.misslewars.Config;
import de.steamwar.misslewars.MWTeam;
import de.steamwar.misslewars.MissileWars;
import de.steamwar.misslewars.items.Missile;
Unused Import
@ -0,0 +29,4 @@
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;
public class CommandInviteHandler implements CommandExecutor {
Unpassender Name für /accept und /decline
@ -0,0 +63,4 @@
}
public static void init() {
File itemsFolder = new File(MissileWars.getPlugin().getDataFolder(), "items");
Sollte in /configs liegen
Liegen die Missiles auch nicht. Also nein!
@ -155,9 +147,7 @@ public class Missile extends SpecialItem {
public static void init() {
File missileFolder = new File(MissileWars.getPlugin().getDataFolder(), "missiles");
Sollte auch /configs liegen
Siehe Oben nein. War schon immer so. Und Außerdem ist auch dort liegen auch die Shield Schem und so. Deswegen sollte das wohl einfach so bleiben!
Das wird wenn versymlinkt (sobald mehrere Configs gleich sind). Da kann man auch mal umbauen, aber das Plugin sollte nix mit /configs zu tun haben.
Chaos wie steht es um dein Review?
Ich würde dich beten, dass du diesen PR in kleinere Teile aufteilst, weil 1200 Zeilen ist doch schon ein bissle viel.
Immer noch nicht nötig. Alle angaben ohne gewähr!
@ -0,0 +66,4 @@
File itemsFolder = new File(MissileWars.getPlugin().getDataFolder(), "items");
if (!itemsFolder.exists() || !itemsFolder.canRead() || !itemsFolder.isDirectory()) throw new SecurityException("Items could not be loaded");
for (File itemFile : Objects.requireNonNull(itemsFolder.listFiles())) {
if (!itemFile.canRead() || !itemFile.isFile()) continue;
Vielleicht sollte man hier einen Fehler werfen.
Ist bei den Missiles auch nicht, dies habe ich einfach kopiert
@ -33,3 +34,4 @@
public class DeathListener extends BasicListener {
private static Vector ZERO = new Vector(0, 0, 0);
Könnte Final sein