Hallo Leute,
habe ein kleines Programm geschrieben ohne jegliche Designentscheidungen.
Wo kann ich ansetzen ? Bis jetzt hab ich nur „gelernt“ man sollte das Programm in Visual , Data und Management aufteilen und nicht zu stark kapseln.
What?!
Sogar für ein Forum ist das „too broad“. Wo soll man da ansetzen? Soll das ein komplettes Code Review werden?
Erstes Verzeichnis, erste Datei, „Config.java“:
Packagename ist blöd
Keine Copyrightinfos
Keine JavaDocs
„public static“ Fields sind ein NO-GO! (augenlid zuckt)
…
Was mit copyright ist nur an einer Stelle hab ich nen Kommentar dran gemacht.
Es gibt keine Datei die Config.java heißt. Nur eine config.txt und ein config package?
Das Programm ist noch überhaupt nicht überarbeitet oder gut programmiert.
Einfach hauptsache es funktioniert deswegen hier der Post falls interesse besteht mal was “größeres” zu reviewen.
Was ist an public static fields falsch ?
Jeder kann auf public static fields zugreifen und sie verändern, deswegen eher die Gegenfrage: Warum programmierst du überhaupt in Java? Wegen OOP ganz sicher nicht. 
Das mag für dich jetzt vielleicht nebensächlich wirken aber in „größeren“ Projekten, sind es genau diese Dinge die erst zu vollkommener Unübersichtlichkeit, Unleserlichkeit und Inkonsistenz sowie kaum zu behebenden grundlegenden Fehlern in der Projektarchitektur führen oder schlussendlich das Projekt komplett in den Sand setzen weil das Projekt nur durch komplettes Neuschreiben überhaupt abgeschlossen, gar erweitert werden kann oder weil der Code so unleserlich und unübersichtlich wird, dass eine weitere Wartung des Codes durch den Menschen nur durch hohen zeitlichen Aufwand erbracht werden kann.
(Auch mit Referenz auf das Thema das gerade nebenan geführt wird: Was wenn Multithreading?)
Oft sind „public static final“ fields von Nutzen als mehrfach verwendete Konstanten, wie z.B. Math.PI.
Selten sind auch „privat static“ fields von Nutzen.
Aber nie sind „public static“ fields von Nutzen.
Dann wäre das so besser aber warum ist das liveColor im Array keine Referenz mehr auf das Livecolor in ColorConfig
import java.awt.Color;
import java.util.regex.Matcher;
import data.datamanagement.StringPattern;
public class ColorConfig {
private Color liveColor = new Color(0, 0, 0);
private Color gameOverColor = new Color(0, 0, 0);
private Color onComingColor = new Color(0, 0, 0);
private Color drawColor = new Color(0, 0, 0);
private Color warningColor = new Color(0, 0, 0);
private Color postponedColor = new Color(0, 0, 0);
private Color backgroundColor = new Color(0, 0, 0);
public static void main(String[] args) {
ColorConfig c = new ColorConfig();
c.updateColors();
}
public void updateColors() {
Color[] colorList = { liveColor, gameOverColor, onComingColor, drawColor, warningColor, postponedColor,
backgroundColor };
String data = new Reader().getColors();
Matcher matcher = StringPattern.colorPattern.matcher(data);
for (int i = 0; i < colorList.length; i++) {
matcher.find();
colorList** = new Color(Integer.parseInt(matcher.group(1)), Integer.parseInt(matcher.group(2)),
Integer.parseInt(matcher.group(3)));
}
System.out.println(liveColor.getBlue() + "x" + colorList[0].getBlue());
}
}
[quote=Quiji]warum ist das liveColor im Array keine Referenz mehr auf das Livecolor in ColorConfig[/quote]wegen Zeile 40?
bye
TT
Ist colorList[0] keine Referenz auf liveColor ?
So gelöst ?
import java.awt.Color;
import java.util.HashMap;
import java.util.Map;
import java.util.regex.Matcher;
import data.datamanagement.StringPattern;
public class ColorConfig {
private Map<String, Color> colorMap = new HashMap<>();
private int numberOfColors;
private final String[] colorList = { "liveColor", "gameOverColor", "onComingColor", "drawColor", "warningColor",
"postponedColor", "backgroundColor" };
public ColorConfig() {
numberOfColors = colorList.length;
}
public static void main(String[] args) {
ColorConfig c = new ColorConfig();
c.updateColors();
}
public void updateColors() {
String data = new Reader().getColors();
Matcher matcher = StringPattern.colorPattern.matcher(data);
for (int i = 0; i < numberOfColors; i++) {
matcher.find();
colorMap.put(colorList**, new Color(Integer.parseInt(matcher.group(1)), Integer.parseInt(matcher.group(2)),
Integer.parseInt(matcher.group(3))));
}
System.out.println(colorMap.get("gameOverColor").getBlue());
}
}
[quote=Quiji]Ist colorList[0] keine Referenz auf liveColor ?[/quote]nach Zeile 40 nicht mehr, da Du ein Neues Color-Objekt im Array abgelegt hattest.
bye
TT
Jap 
Darauf hab ich gar nicht geachtet…
[quote=Quiji]Darauf hab ich gar nicht geachtet…[/quote]genau deswegen macht mal solche “Doppelinitialisierungen” nicht.
Und außerdem sichert man das gewünschte Verhalten seines Programms mit UnitTests ab…
bye
TT
Mal eine schnelle review. Ich weiß nicht wie erfahren du bist, ich schreib einfach mal ein paar Dinge, die mir aufgefallen sind:
- Du solltest dir mal sowas wie Maven oder Gradle anschauen. Das macht dir das Leben wesentlich einfacher.
- Das Problem mit public static wurde ja schon zu genüge genannt. Eine Sache die du dir wohl sehr einfach abgewöhnen kannst, wenn du auf ein DI-Framework setzt (z.B. Guice). Letztendlich willst du ja überall die gleiche Config verteilen und standest wohl vor dem Problem, wie du diese Werte elegant und ohne viel Aufwand im kompletten Programm bereit stellen kannst. Ein DI-Framework löst dir das Problem auf eine wirklich elegante Art.
- Meine Multithreading-Kenntnisse sind jetzt nicht die besten aber: HTMLPackage.java.
— Du hast nicht synchronisierte Schreibbefehle (das kann nach hinten los gehen)
— Von einem unserer Softwarearchitekten im Betrieb hab ich mal den Ratschlag bekommen Threads niemals selber zu erstellen - sondern immer mit ExecutorServices zu arbeiten. Wenn du einen angepassten Thread haben willst, kannst du das dann in der ThreadFactory vom Executor - Gewöhn dir javadoc an
- Unit tests fehlen. Wenn du dein Projekt mit Maven oder Gradle aufsetzt, dann hast du es von der Projektstruktur auch gleich einfacher mit deinem Test-setup
An der Stelle hör ich mal auf weiter drüber zu schauen, ich glaub da ist erstmal genug Stoff für dich dabei
Was mir als erstes aufgefallen ist, ist die ausgeprägte Code-Duplikation. Spätestens nach dem dritten new Color(Integer.parseInt(matcher.group(1)), Integer.parseInt(matcher.group(2)), Integer.parseInt(matcher.group(3))) sollte man mal darüber nachdenken, das in eine Methode auszulagern. Das macht den Code leichter lesbar, bewahrt vor Copy-und-Paste-Fehlern und erleichtert spätere Änderungen. Siehe dazu https://de.wikipedia.org/wiki/Don’t_repeat_yourself
In visual.MatchLabel wird in Zeile 62 ein String mit == verglichen
Ebenso in visual.labelcontainer.LabelContainer Zeile 33
control.Loader schließt den BufferedReader aus 173 nicht in allen Fällen, also wenn Exceptions fliegen. Try-with-Resource ist hier angebrachter.
In config.Reader werden die Strings in den Schleife mit einem + aneinandergefügt. Das ist in Java meist nicht optimal, auch wenn es hier noch nicht so auffällt.
Hier würde man wohl einen StringBuffer mit append bevorzugen oder alternativ den One-liner
String content = new Scanner(file).useDelimiter("\Z").next();
Statt dem Scanner kann man auch java.nio.file.Files mit readAllLines verwenden, dass eine List liefert. Dies kann man dann z.B. über die Streams-Api zu einem großen String vereinen.
In data.Match.java wird in Zeile 90-91 Zeug in der lokalen Variable score abgelegt und dann am Ende trotzdem einfach ein Hash zurückgegeben. Sinnloser Code
Leerer Catch-Block in control.Loader Z. 156
Das sind so Handwerkliche Fehler die man noch beseitigen könnte.
Findbugs ist das Tool das ich verwendet habe um die oben genannten Dinge zu finden. Die bereits genannten Dinge mit den static fields werden dort übrigens auch angekreidet.
Zudem wäre es super wenn JHook irgendwo verfügbar wäre. Idealerweise mit einem Build-Tool wie gradle oder maven. Ansonsten kann dies niemand bauen, der nicht JHook irgendwoherbekommt.
[quote=ionutbaiu]In config.Reader werden die Strings in den Schleife mit einem + aneinandergefügt. Das ist in Java meist nicht optimal, auch wenn es hier noch nicht so auffällt.
Hier würde man wohl einen StringBuffer mit append bevorzugen oder alternativ den One-liner
String content = new Scanner(file).useDelimiter(„\Z“).next();
Statt dem Scanner kann man auch java.nio.file.Files mit readAllLines verwenden, dass eine List liefert. Dies kann man dann z.B. über die Streams-Api zu einem großen String vereinen.[/quote]
An sich richtig, aber aus den Oracle-Docs geht hervor
Chapter*15.*Expressions
To increase the performance of repeated string concatenation, a Java compiler may use the StringBuffer class or a similar technique to reduce the number of intermediate String objects that are created by evaluation of an expression.
und
String (Java Platform SE 8 )
The Java language provides special support for the string concatenation operator ( + ), and for conversion of other objects to strings. String concatenation is implemented through the StringBuilder(or StringBuffer) class and its append method. String conversions are implemented through the method toString, defined by Object and inherited by all classes in Java. For additional information on string concatenation and conversion, see Gosling, Joy, and Steele, The Java Language Specification.
Der Compiler ersetzt also das „+“ automatisch durch ein „stringBuilder.append(s)“.
Das ist klasse, denn man kann dadurch weiterhin die schnellere und viel einfacher lesbare Methode („+“) verwenden ohne auf die Performancevorteile von StringBuilder zu verzichten. 
@TMII sicher?
Hier wird zum Beispiel das Gegenteil behauptet.
String concatenation with Java 8
Das innerhalb des Schleife ein + durch ein StringBuilder.append ersetzt wird, bezweifle ich ja nicht.
Das Problem, das aber entsteht ist, dass bei jeder Iteration, also jedem Schleifendurchlauf, ein neuer StringBuilder erzeugt wird.
Und das ist ein Problem!
Kannste Selbst mal nachmessen was da bei 30.000 Iterationen für ein Unterschied rauskommt.
boolean stringBufferConcatenation = false;
int iterations = 30000;
long start = System.currentTimeMillis();
String data = "";
StringBuffer stringBuffer = new StringBuffer();
for (int i = 0; i < iterations; i++) {
if(stringBufferConcatenation) {
stringBuffer.append("Much more data");
} else {
data += "Much more data";
}
}
data = stringBuffer.toString();
System.out.println("done in "+(System.currentTimeMillis()-start));
}```
10 millis vs 7 Sekunden, da brauch ich nicht mal in den ByteCode reinschauen um zu sehen, dass das doch nicht so glatt läuft.… und das ist auch total relevant wenn man ein config file mit 41 Zeilen einliest 
Wuerde da auch String mit + anstatt expliziten StringBuilder mit append verwenden, der Lesbarkeit wegen.
StringBuffer dagegen sollte man nur verwenden wenn man synchronisation braucht, ansonsten eben StringBuilder.
Bytecode gucken und theoretische Gedankenspiele zur Performance sind sicher wichtig, aber wichtiger ist ein Profiler
Wie war das nochmal mit der premature Optimisation?
Wenn Anfaenger lernen klaren Code zu schreiben ist das IMHO besser als dass man sie zu nicht-messbaren Micro Optimierungen anregt.
[ot]
Ich stolpere JEDES mal drüber: “StringBuffer war das synchronisierte, oder? Lieber nochmal nachschauen - ja” :rolleyes:
[/ot]
einmal da ist so eine Methode auch schnell zum Einlesen eines Wörterbuches verwendet,
(abgesehen davon dass es für jene kaum Sinn macht, alles in einen String hineinzuschreiben )
in Reader.java schon jetzt der Code dreimal kopiert für drei Dateien,
besser nur eine allgemeine Methode readToString(String filename) und in dieser Methode bei Gelegenheit ruhig StringBuilder
eine allgemein geringe Gefahr, aber durchaus eine reale, und dazu eine logisch und praktisch gut zu verstehene,
genau die richtige für Anfänger, sowas kann man noch lernen, vergleichbar mit Set statt List für contains-Suche,
es auch dann anzuwenden ist der logische Schritt
hier ist es wirklich leicht: StringBuffer ist alt, StringBuilder neu und immer die bessere Wahl,
wenn Synchronisation eine Frage darstellen sollte, dann besser selber organisieren, nicht in zweifelhaften Altklassen verstecken,
Vector statt ArrayList verwendet man doch auch nicht, oder?
Vielen dank erstmal für das viele Feedback ich setze mich mal davor und berichte 