Klassen Spaghetti Code Vermeidung

[friednoodles]

Hallo, ich wollte euch mal um Rat bitten wie ich meinen Code klüger gliedern kann.
Ich habe vor 3 Monaten das Programmieren angefangen und habe es geschafft ein Programm mit kleinen nützlichen Funktionalitäten zu schreiben. Nur ist mein Problem das Programm ist inzwischen 400 Zeilen lang und niemand außer mir Blickt so wirklich durch den Code durch.

Hat jemand allgemeine Tipps für mich wie ich anfangen könnte den Code klüger aufzuteilen?
Mein Code hat etliche Methoden die alle kreuz und quer auf einander zugreifen und ich finde es unglaublich schwer einen Anfangspunkt zum Aufräumen zu finden.

Liebe Grüße,

Friednoodles

 

[Robat]

Hat jemand allgemeine Tipps für mich wie ich anfangen könnte den Code klüger aufzuteilen?

Erst Nachdenken und dann anfangen zu Programmieren.
Wenn du ein neues Projekt anfängst überleg dir, welche Funktionalitäten dein Programm hat und wie diese zusammenspielen. Überleg dir wie du die Beziehungen brauchst und wie du das ganze sinnvoll in Klassen / Methoden aufteilst.

Wichtig: Nicht alles in eine eigene Klasse .. SRP

 

[Javinner]

@friednoodles
Poste doch dein Programm hier, benutze bitte dafür Code-Tags, und beschreibe kurz, was das Programm macht usw. Dann wird dieser Thread garantiert! mehrere Seiten Lang, die den Weg aufzeichnen, wie Du DEIN Programm unter Anleitung und Vorschlägen optimierst Ist ein schmerzhafter und lehrreicher Prozess!

 

[friednoodles]

Ich glaube meinen ganzen Code kann ich leider nicht posten. Aber ich habe vier Methoden auf die ich etwas näher eingehen kann. Diese würde ich Beispielsweise gerne aus meiner Klasse in der bisher ALLES passiert rausnehmen und in eine andere Klasse verschieben.
Kurzgesagt wandelt mein Programm ein Datum im Format yyyy-MM-dd HH:mm:ss in eine Epoch Timestamp um und umgekehrt geht das auch. Die Eingabe findet in einem TextField statt und wird auf eine TextArea ausgegeben.

Diese Methode gibt mir das Ergebnis der Umwandlung in der TextArea aus.

private void showResult() {
        textArea.setText(whichPattern());
    }

Diese Methode überprüft welches Pattern zu dem TextField Input passt

    private String whichPattern() {
        SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
        String textFieldInput = textField.getText();
        Date parsInToDate = new Date();
        long epochMillis;

        if (textFieldInput.isEmpty()) {
            textArea.setText("You can't convert from nothing");
        } else if (Pattern.matches(
                "^\\d\\d\\d\\d-(0?[1-9]|1[0-2])-(0?[1-9]|[12][0-9]|3[01]) (00?[0-9]|1[0-9]|2[0-3]):([0-9]|[0-5][0-9]):([0-9]|[0-5][0-9])$",
                textFieldInput)) {
            epochMillis = dateToEpochTime(sdf, textFieldInput, parsInToDate);
            textArea.setText(epochMillis + "");
        } else if (Pattern.matches("^[0-9]*$", textFieldInput)) {
            Calendar calendar = epochTimeToDate();
            textArea.setText(sdf.format(calendar.getTime()));
        } else {
            textArea.setText(textFieldInput + " is not a valid entry");
        }
        String textAreaOutput = textArea.getText();
        return textAreaOutput;
    }

Die hier führt eine Umwandlung durch:

    private long dateToEpochTime(SimpleDateFormat sdf, String textFieldInput, Date parsInToDate) {
        long epochMillis;
        try {
            parsInToDate = sdf.parse(textFieldInput);
        } catch (ParseException e) {
            e.printStackTrace();
        }
        epochMillis = parsInToDate.getTime();
        return epochMillis;
    }

Und diese die Andere:

    private Calendar epochTimeToDate() {
        long long1 = Long.valueOf(textField.getText()).longValue();
        Calendar calendar = Calendar.getInstance();
        calendar.setTimeInMillis(long1);
        return calendar;
    }

Diese vier Methoden sind neben ALLEN anderen die mein Programm hat in meiner einzigen Klasse "Konverter"
Am Thema refactoring hänge ich schon ziemlich lange, habe viele Beispiele gesehen und weiß aber einfach nicht wie ich es auf meinen speziellen Fall anwenden kann. Für Hilfe wäre ich sehr dankbar.

 

[VfL_Freak]

Moin,
ich würde evtl. nochmal ein paar Kleinigkeiten an der Namensgebung ändern!

"whichPattern" ist ein recht seltsamer Name. Etwas Sprechenderes wäre schön!
Und was genau ist "Epoch" ??

VG Klaus

 

[friednoodles]

Moin,
ich würde evtl. nochmal ein paar Kleinigkeiten an der Namensgebung ändern!

"whichPattern" ist ein recht seltsamer Name. Etwas Sprechenderes wäre schön!
Und was genau ist "Epoch" ??

VG Klaus

Danke dir, darum sollte ich mich auch kümmern ja.
Was genau "Epoch" ist fragst du? Hab ich das irgendwo nicht richtig benannt oder möchtest du eine Definition?
Die Epoch time sind doch einfach Millisekunden seit dem 01.01.1970 00:01:00, also heute wäre das beispielsweise eine 13 stellige Zahl.

 

[VfL_Freak]

möchtest du eine Definition?

genau, ist ein etwas seltsamer, wenig aussagekräftiger Begriff und fiel in Deinem Code 'etwas vom Himmel'

 

[Robat]

Generell solltest du View, Model und Logik von einander trennen. Wenn du also selber sagst, dass bspw. showResult() [View] und dateToEpochTime(SimpleDateFormat, String, Date) [Logik] in der selben Klasse stehen, läuft etwas falsch. Schau dir dazu am Besten mal das ModelViewController-Pattern an.

Außerdem würde ich nicht mehr auf die Calendar API setzen. Seit Java 8 gibt es eine neue Date & Time API mit der sich einige Dinge leichter umsetzen lassen. Schau dir diese vielleicht mal an.

Deine Methode whichPattern() macht mEn gerade zu viel auf einmal. (SRP) Sie holt sich den Text von aus der GUI, verarbeitet die Daten und gibt eine Nachricht auf der GUI aus. Außerdem würde diese Methode gerade nur für Swing funktionieren, weil sie vom JTextArea abhängig ist. Übergib' der Methode doch den String und gib den Text, den du auf der TextArea anzeigen lassen willst, zurück.

ist ein etwas seltsamer, wenig aussagekräftiger Begriff

@VfL_Freak Epoch ist doch aber ein gängiger Begriff sogar in der Java Date-Time-API. Siehe zB. LocalDate#toEpochDay()

 

[mrBrown]

genau, ist ein etwas seltsamer, wenig aussagekräftiger Begriff und fiel in Deinem Code 'etwas vom Himmel'

epochMillis/epochTime ist doch einfach nur ein Domänenspezifischer Begriff, was soll man denn stattdessen verwenden?

Diese Methode überprüft welches Pattern zu dem TextField Input passt

Absehen von dem schon angesprochenen Namen macht die Methode zwei Dinge: Daten aus dem Textfeld holen und mit diesen arbeiten. Leichter machst du es, wenn du beide Dinge trennst. Das Verarbeiten der Daten ist dann wiederverwendbar und testbar.

 

[VfL_Freak]

Moin,

Epoch ist doch aber ein gängiger Begriff sogar in der Java Date-Time-API.

Ah, ok - Danke
Wir nutzen hier JodaTime, von daher war mir das kein Begriff ...
VG Klaus

 

[friednoodles]

@Robat
Ich habe versucht deinen Ratschlägen zu folgen und habe eine neue Klasse "KonverterTools" erstellt.

 KonverterTools tools = new KonverterTools();

Meine View, die noch in der alten Klasse ist:

    private void showResult() {
        textArea.setText(tools.whichPattern(this.textField.getText()));
    }

Und die Folgenden drei Methoden sind jetzt in der neuen Klasse:
Mir fällt für meine ehemalige whichPattern() Methode einfach kein guter Name ein :/
Habe hier als Parameter einfach einen String eingeführt, damit kann ich mir die textArea in dieser Methode ja sparen, oder? Funktionieren tut es zumindest.

    public String PatternQuery(String toCheck) {
        SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
        Date parsInToDate = new Date();
        long epochMillis;
       
        if (toCheck.isEmpty()) {
            return "You can't convert from nothing";
        } else if (Pattern.matches(
                "^\\d\\d\\d\\d-(0?[1-9]|1[0-2])-(0?[1-9]|[12][0-9]|3[01]) (00?[0-9]|1[0-9]|2[0-3]):([0-9]|[0-5][0-9]):([0-9]|[0-5][0-9])$",
                toCheck)) {
            epochMillis = dateToEpochTime(sdf, toCheck, parsInToDate);
            return epochMillis + "";
        } else if (Pattern.matches("^[0-9]*$", toCheck)) {
            Calendar calendar = epochTimeToDate(Long.parseLong(toCheck));
            return sdf.format(calendar.getTime());
        } 
        return toCheck + " is not a valid entry";
    }

In dieser Methode muss ich soweit nichts ändern denke ich.

    private long dateToEpochTime(SimpleDateFormat sdf, String textFieldInput, Date parsInToDate) {
        long epochMillis;
        try {
            parsInToDate = sdf.parse(textFieldInput);
        } catch (ParseException e) {
            e.printStackTrace();
        }
        epochMillis = parsInToDate.getTime();
        return epochMillis;
    }

Und hier habe ich einfach wieder einen Parameter hinzugefügt und konnte eine Zeile löschen.

    private Calendar epochTimeToDate(Long timestamp) {
        Calendar calendar = Calendar.getInstance();
        calendar.setTimeInMillis(timestamp);
        return calendar;
    }

Funktionieren tut alles so wie bisher. Nur ist die Frage ob das konform genug ist?

 

[Robat]

Mir fällt für meine ehemalige whichPattern() Methode einfach kein guter Name ein :/

Versuch doch mal in Worten zu beschreiben, was die Methode macht und dann überlegst du dir einen kurzen, aussagekräftigen, (englischen) Namen für die Methode.

Warum bekommt dateToEpochTime denn das SimpleDateFormat übergeben? Es ist doch immer das selbe Pattern, was du zum parsen brauchst, oder?
Ansonsten noch ein Hinweis: Variablen immer so spät wie nötig deklarieren und so früh wie möglich initialisieren. Bedeutet für dein konkretes Beispiel:

public String patternQuery(String toCheck) {
    ...
    long epochMillis;
    if(...) {
      
    } else if( ... ) {
        epochMillis = ...
        ...
    } else if( ...) { ... }
    return ...
}

Du verwendest epochMillis nur in dem zweiten else-if, deklarierst die Variable aber schon ganz am Anfang der Methode.
Das mag bei so einer Methode vielleicht nicht stören aber sollte diese doch mal etwas länger werden, mit etwas mehr Variablen, verliert man schnell den Überblick. Zieh die Deklaration lieber mit in die if-Anweisung rein

 

[Javinner]

SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");

dateToEpochTime(sdf, toCheck, parsInToDate);

Du erstellst bei jedem Aufruf von PatternQuery, was nach den Regeln hätte patternQuery heißen sollen, immer ein neues SimpleDataFormat. Dieses Format übergibst du dann an eine weitere Methode, so dass ich hier es als eine finale Klassenvariable erstellt hätte.

 

[mihe7]

Ich habe versucht deinen Ratschlägen zu folgen und habe eine neue Klasse "KonverterTools" erstellt.

Klassen, die Tools, Utils oder ähnliches im Namen haben, sind Indiz für Designfehler oder für die Faulheit des Entwicklers oder beides

Mir fällt für meine ehemalige whichPattern() Methode einfach kein guter Name ein

Das ist tatsächlich problematisch: gute Namen finden.

Mal so als Idee:

class DateMillis {
    private final Date date;
    private static final SimpleDateFormat sdf = 
            new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
    private boolean millis;

    private DateMillis(Date date, boolean millis) {
        this.date = date;
        this.millis = millis;
    }

    public static DateMillis parse(String text) {
        if (Pattern.matches(DATE_PATTERN, text)) {
            return new DateMillis(sdf.parse(text), false);
        } else if (Pattern.matches(MILLIS_PATTERN, text)) {
            return new DateMillis(new Date(Long.valueOf(text)), true);
        } else {
            throw new IllegalArgumentException();
        }
    }

    public DateMillis inverse() {
        this.millis = !millis;
        return this;
    }

    public String toString() {
        return millis ? String.valueOf(date.getTime()) : sdf.format(date);
    }
}

Benutzung:

textField.setText(DateMillis.parse(textField.getText()).inverse().toString());

Achtung: der Spaß ist ungetestet.

 

[friednoodles]

Warum bekommt dateToEpochTime denn das SimpleDateFormat übergeben? Es ist doch immer das selbe Pattern, was du zum parsen brauchst, oder?

Ja das Pattern ist immer das selbe. Wenn ich sdf aber nicht übergebe kommen lauter Fehlermeldungen die damit enden, dass Eclipse von mir möchte SimpleDateFormat in static zu ändern. Lass es deswegen einfach drin.

Ansonsten noch ein Hinweis: Variablen immer so spät wie nötig deklarieren und so früh wie möglich initialisieren. Bedeutet für dein konkretes Beispiel:

public String patternQuery(String toCheck) {
    ...
    long epochMillis;
    if(...) {
   
    } else if( ... ) {
        epochMillis = ...
        ...
    } else if( ...) { ... }
    return ...
}

Du verwendest epochMillis nur in dem zweiten else-if, deklarierst die Variable aber schon ganz am Anfang der Methode.

Danke für den Tipp, habe meinen Code dementsprechend angepasst.

Du erstellst bei jedem Aufruf von PatternQuery, was nach den Regeln hätte patternQuery heißen sollen, immer ein neues SimpleDataFormat. Dieses Format übergibst du dann an eine weitere Methode, so dass ich hier es als eine finale Klassenvariable erstellt hätte.

Also einfach aus

 SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");

das hier machen?

final SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");

 

[friednoodles]

Klassen, die Tools, Utils oder ähnliches im Namen haben, sind Indiz für Designfehler oder für die Faulheit des Entwicklers oder beides

Wie würdest du das ab ändern? Einfach einen passenderen Namen auswählen oder noch mehr aus dieser Klasse rausziehen?

Mal so als Idee:

class DateMillis {
    private final Date date;
    private static final SimpleDateFormat sdf =
            new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
    private boolean millis;

    private DateMillis(Date date, boolean millis) {
        this.date = date;
        this.millis = millis;
    }

    public static DateMillis parse(String text) {
        if (Pattern.matches(DATE_PATTERN, text)) {
            return new DateMillis(sdf.parse(text), false);
        } else if (Pattern.matches(MILLIS_PATTERN, text)) {
            return new DateMillis(new Date(Long.valueOf(text)), true);
        } else {
            throw new IllegalArgumentException();
        }
    }

    public DateMillis inverse() {
        this.millis = !millis;
        return this;
    }

    public String toString() {
        return millis ? String.valueOf(date.getTime()) : sdf.format(date);
    }
}

Benutzung:

textField.setText(DateMillis.parse(textField.getText()).inverse().toString());

Achtung: der Spaß ist ungetestet.

Mit ein paar Anpassungen funktioniert das tatsächlich, danke für den Vorschlag

 

[Javinner]

#14 @mihe7 hat es dir vorgemacht.