Best Practice Beurteilung Programmcode

TheFrog

Aktives Mitglied
Gute Morgen. Ich hab mich gestern ein wenig mit regulären Ausdrücken in Java beschäftigt und dazu ein kleines Programm geschrieben, das den Quellcode einer Website als String liefert. Auf diesen String kann dann durch eine weitere Methode ein regulärer Ausdruck angewendet werden. So lange Zeichenketten gefunden werden, die dem Pattern entsprechen, wird diese einer ArrayList hinzugefügt. Am Ende wird die ArrayList returned und kann auf der Konsole ausgegeben werden.

Es funktioniert dabei alles wie ich es mir vorgestellt habe. Ich möchte nur gerne wissen, was ihr von dem Design haltet. Habe ich gegen irgendwelche Konventionen verstoßen oder sogar etwas falsch gemacht?

Nun folgt mein Code aufgeteilt in zwei Klassen, die eine enthält die Funktionalität und die andere ist das Hauptprogramm.

Klasse mit der Funktionalität
Java:
package Classes;

import java.io.*;
import java.net.URL;
import java.util.ArrayList;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
* Created by The Frog on 16.02.2016.
*/
public class StringUtils {

    // keine Objekterzeugung dieser Klasse
    private StringUtils(){}

    /**
     * This method returns the source code of a website as a String object.
     *
     * @param url the source code location
     * @return String returns source code as String.
     * @exception  IOException on input error
     * @see IOException
     */
    public static String getSourceCodeFromWebsite(String url)throws IOException{ //Weiterleitung der Exceptions an den Aufrufer

        URL site = new URL(url);
        String line;
        String sourceCode="";

        // try-with-resources
        try(
            BufferedReader breader = new BufferedReader(new InputStreamReader(site.openConnection().getInputStream()));){

            while((line = breader.readLine())!=null){
                sourceCode += line +"\r\n";
            }
        }

        return sourceCode;
    }

    /**
     *
     * @param regex the regular expression that is applied to a String object.
     * @param data the extraction String
     * @return returns an arraylist containing Strings that matches the pattern.
     */
    public static ArrayList<String> extractString(String regex,String data){
        ArrayList<String> results = new ArrayList<>();
        Pattern pattern = Pattern.compile(regex);
        Matcher m = pattern.matcher(data);

        while(m.find()){
            results.add(m.group());
        }
        return results;
    }
}
Klasse mit der main method
Java:
package Classes;

import java.io.IOException;
import java.util.ArrayList;

/**
* Created by The Frog on 17.02.2016.
*/
public class TestApp {
    public static void main(String[] args) {

        try {
            ArrayList<String> jokesextracted = StringUtils.extractString("(<div class=\"joke\">)+.*?</div>", StringUtils.getSourceCodeFromWebsite("http://www.witze.net"));
            // zeilenweise Ausgabe der ArrayList-Elemente
            jokesextracted.forEach(System.out::println);
        }catch(IOException e){
            e.printStackTrace();
        }
        System.out.println("\n------------------\nEnde des Programms!");
    }
}

Ich freue mich über hoffentlich viele Antworten :)
 
K

kneitzel

Gast
Das sieht doch recht gut aus würde ich sagen.

Meine Anmerkungen wären:
- Ich würde die Kommentare kritisch prüfen. Die "//" Kommentare scheinen unnötig zu sein. Wenn Du glaubst, dass die Kommentare zum Verständnis doch notwendig sind, dann würde ich ein Umschreiben des Codes in Betracht ziehen. (Also z.B. die Ausgabe in einer Schleife selbst durchführen und diesen Code dann in eine eigene Funktion stecken und so.) Dies scheint mir hier aber nicht wirklich notwendig.
- Bei den JavaDoc Kommentaren: String object / String instance. Wenn Du einfach String schreibst, dann bezeichnet dies ja eine Instanz von String. Es ist hier ja klar, dass da nicht eine Subklasse von String gemeint ist oder so. (Wie tief bin ich jetzt gesunken, dass ich das ansprechen muss?)

Ansonsten ist mir erst einmal nichts aufgefallen.
 

Thallius

Top Contributor
MAl ganz ehrlich. Ist ja wirklich gut und schön und wahrscheinlich nach Lehrbuch aber wozu braucht eine Methode die

public static String getSourceCodeFromWebsite(String url) throws IOException

heißt ein JavaDoc? Da habe ich aus der einen Zeile direkt alles heruasgelesen was ich mir im Javadoc mühsam zusammenbastelb muss.

Finde ich total übertrieben.

Viel schlimmer finde ich eine so universelle Methode static zu machen und ihr damit jegliche Verwendung im Mutli-Threadding zu nehmen.

Gruß

Claus
 
K

kneitzel

Gast
Also JavaDoc muss eigentlich sein, denn daraus entsteht ja auch ein Teil der Dokumentation. Also im beruflichen Umfeld (das ist aber C# / Visual Studio) hat jede Funktion, die public ist, die entsprechende Dokumentation zu haben. Da wäre eher für mich wichtig, dass da mehr Informationen geliefert werden, wann denn mit Exceptions zu rechnen ist. Was bedeutet denn eine IOException? Beim schreiben der Funktion nutzt Du ja Funktionen, die diese werfen und da müsstest Du dann ja recherchiert haben, wann dies auftritt.
Wenn ich diese Funktion nutze, dann ist diese Information wichtig und ich will doch nicht die Implementation prüfen um zu wissen welche Exceptions Du ggf. 1:1 weiter gibst!

@Thallius: Was ist denn da die Limitation bezüglich Multi-Threading? Habe ich da etwas verpasst?
Du kannst die Funktion doch problemlos aus Threads heraus aufrufen.
 

Flown

Administrator
Mitarbeiter
Die Methode extractString sollte statt einer ArrayList eine List (also das Interface zurücklieren), da es nur ein Implementierungsdetail ist. Allgemein sollte man immer gegen Interfaces implementieren.
 

Jardcore

Top Contributor
public static ArrayList<String> extractString(String regex,String data){
Als Rückgabewert sollte man, wenn möglich, den allgemeinsten Typen nehmen, hier List<String>

Ich bin auch kein Fan von unzähligen Dokumenten, in meinem Arbeitsumfeld wird nur sehr sehr selten dokumentiert. Vielmehr werden zusätzliche verständliche Methoden erstellt. Welche dann anhand ihres Namens die Funktion wiederspiegeln.

extractString() könnte vielleicht einen besseren Namen bekommen: extractStringDependingOnRegualarExpression()

Weiterhin gibt es die Möglichkeit ein try mit Ressourcen zu benutzen.
https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
Dadurch wird sichergestellt, dass geöffnete Streams automatisch geschlossen werden.

Beste Grüße,
Jar
 

Flown

Administrator
Mitarbeiter
Noch eine Kleinigkeit: Mach die Klasse final, da sie ja eine Utilityfunktion erfüllt. Der User deiner API sollte so viel wie nötig und so wenig wie möglich ändern können an deinem Framework. Generell in der API-Entwicklung zählt (mit mehreren Versionen): Es ist immer leichter eine Restriktion zu lockern, als eine zu erstellen!

Wie schon vorher gesagt sollte ArrayList<String> results = new ArrayList<>(); so aussehen: List<String> result = new ArrayList<>();

@Jardcore TO benutzt bereits try-resource
 

TheFrog

Aktives Mitglied
@Jardcore try-with-resources habe ich doch verwendet?

Eine bessere Wahl der methodennamen und der return einer Liste statt der ArrayList wurden auch schon vorgeschlagen und von mir dankend angenommen :D

@Flown final als Modifier für die Klasse zur Unterbindung der Vererbung ist eine sehr gute Idee, ich setze das sofort im Code um.
 
Zuletzt bearbeitet:

Joose

Top Contributor
Eine bessere Wahl der methodennamen

Niemand hat den Methodennamen als verbesserungswürdig dargestellt ;)
Thallius hat sogar geschrieben, das er anhand der Signatur (und da gehört der Methodenname dazu) schon rauslesen kann was die Methode macht und zurückliefert.
Und der Name "getSourceCodeFromWebsite" passt so schon ganz gut.

Wenn würde ich eher die Methoden in unterschiedliche Klassen geben.
"StringUtils" würde für mich ein Sammlung an Methoden zur Manipulation bzw. herum hantieren von Strings bedeuten. In diese Klasse passt "extractString" gut hinein.
"getSourceCodeFromWebsite" hingegen erwarte ich nicht in dieser Klasse sondern eher in "WebsiteUtils". Damit könnte man den Methodennamen auch auf "getSourceCode" kürzen.
Aber sowas sollte man erst wirklich beachten wenn das Projekt entsprechend größer ist. Bei kleinen Bastel-/Testapplikationen hat es keinen Sinn eine Flut an packages und Klassen zu erfinden ;)
 

TheFrog

Aktives Mitglied
@Joose danke auch dir für deine Meinung.

Darüber habe ich mir noch keine Gedanken gemacht den Code auf auf zwei Klassen aufzuteilen. Das ist trotz des geringen Codeumfangs eine tolle Idee sofern ich die Methoden in späteren Projekten wiederverwenden möchte. :)
 
K

kneitzel

Gast
@TheFrog Ja, du solltest darüber nachdenken, Dir evtl. nach und nach Deine eigene Library zu bauen. Klar, am Anfang scheint es etwas Overkill zu sein, aber wenn die etwas gewachsen ist, dann hast Du da etwas, das Dir die Arbeit erleichtern kann. Dann kommt auch noch die Unterteilung in Packages dazu.
 
K

kneitzel

Gast
Und dann auch einen Namen haben, der mit einer umgedrehten Domain anfängt, so dass sicher gestellt ist, dass man die volle Kontrolle hat. Wenn man keine Domain besitzt, dann ist die umgedrehte Email-Adresse als Lösung denkbar. Also sowas wie de.web.username oder net.gmx.username.
 

klauskarambulut

Bekanntes Mitglied
Warum nicht gleich eine Klasse Website anstelle von StringUtils, bzw. WebsiteUtils?

Einen Konstruktor der die URL als String erhält und eine einfache Methode getText()

Das ganze in einem Interface Generalisieren, wenn! man sich überlegt, das ganze auch auf andere Quellen wie zum Beispiel Dateien auszuweiten. z.b. TextProvider mit getText()

Dann kann man sich überlegen, ob man mit StringUtils arbeiten möchte oder eine Klasse RegExFinder zu nehmen, die im Konstruktor einen TextProvider erhält und dann eine Methode extractStrings(String pattern) hat die List zurückliefert.

Was aber garnicht geht ist diese Zeile, weil zu lange
ArrayList<String> jokesextracted = StringUtils.extractString("(<div class=\"joke\">)+.*?</div>", StringUtils.getSourceCodeFromWebsite("http://www.witze.net"));

ein Minimum ist dies wie folgt zu ändern
String websiteURL = "http://www.witze.net";
String websiteText = StringUtils.getSourceCodeFromWebsite(websiteURL);

String pattern = "(<div class=\"joke\">)+.*?</div>";

List<String> jokesextracted = StringUtils.extractString(pattern, websiteText);

bzw.

String websiteURL = "http://www.witze.net";
TextProvider text = new Website(websiteURL);

RegExFinder regExFinder = new RegExFinder(text);

String pattern = "(<div class=\"joke\">)+.*?</div>";
List<String> jokesextracted = regExFinder.extractStrings(pattern);

Beim Code, der die Website ausliest, kann man try-with-ressource nehmen, da man mittlerweile Java 7 voraussetzen kann (solange man nicht für Android programmiert, allerdings werden ja auch schon Java 8 Features verwendet) und damit die Vorteile von AutoCloseable nutzen kann.
 

Ähnliche Java Themen

Neue Themen


Oben