Feedback zum Downloadmodul bitte

?

Welche Schulnote (auch wenn die Schule ein wenig her ist :D ) würdest du diesem Code geben?

  1. 1

    0 Stimme(n)
    0,0%
  2. 2

    0 Stimme(n)
    0,0%
  3. 3

    5 Stimme(n)
    83,3%
  4. 4

    1 Stimme(n)
    16,7%
  5. 5

    0 Stimme(n)
    0,0%
  6. 6

    0 Stimme(n)
    0,0%

Diskutiere Feedback zum Downloadmodul bitte im Codeschnipsel u. Projekte Forum; Hallo Leute, ich habe vor einiger Zeit ein kleines Downloadmodul geschrieben, welches ich nun in ein paar voneinander unabhängigen Projekten...

  1. patklu1988
    patklu1988 Neues Mitglied
    Hallo Leute,
    ich habe vor einiger Zeit ein kleines Downloadmodul geschrieben, welches ich nun in ein paar voneinander unabhängigen Projekten einbinden möchte. Ich bin leider ein wenig zu sehr in dem Code um ihn objektiv bewerten zu können und würde mich freuen wenn ihr mir ein paar Meinungen zur Umsetzung geben könnt.
    Vorab: Ich habe alle Kommentare und Dokumentationen entfernt, in der Hoffnung, dass der Code, auch für "fremde" Entwickler, für sich spricht.

    Die Schnittstelle nach außen:
    Code (Java):
    public class DownloadManager {
        private static final long UPDATE_INTERVAL = 100;
       
        private DownloadException downloadEx;
       

        public long downloadFile(final DownloadUpdater du, final DownloadRecord dRecord) throws DownloadException {
            final Downloader d = new Downloader(dRecord);
            initializeFileSize(d);
            d.download();
            new Thread() {
                public void run() {  
                    try {                  
                        float percent = 0.00f;
                        long bytesRead = 0;
                        while(!d.isDownloadFinished() && !d.isDownloadAborted()) {
                            if(d.getBytesRead() >= 0) {
                                bytesRead = d.getBytesRead();
                                percent = (bytesRead * 100f) / d.getdFile().getFileSize();                          
                            }                                                  
                            du.updateComponent(percent, bytesRead);
                            sleep(UPDATE_INTERVAL);                  
                        }
                        if(d.isDownloadAborted()) {
                            downloadEx = new DownloadException(String.format("Download aborted! %s", d.getResultMessage()));
                            du.downloadFinished(false);
                        }
                        du.downloadFinished(true);
                    } catch(Exception e) {
                        if(DEBUG.ON)
                            e.printStackTrace();
                        downloadEx = new DownloadException("Error while downloading file. For further information check inner exception.", e);
                        du.downloadFinished(false);
                    }                          
                }
            }.start();
           
            return d.getdFile().getFileSize();
        }
       
        private void initializeFileSize(final Downloader d) throws DownloadException {
            try {
                d.initializeFileSize();          
            } catch(Exception e) {
                if(DEBUG.ON)
                    e.printStackTrace();
                DownloadException de = new DownloadException("Error while searching file. Please make sure that the link is valid and your internet connection enabled.", e);
                throw de;
            }
        }

       
        public DownloadException getDownloadEx() {
            return downloadEx;
        }
    }
    Das Interface um den Downloadfortschritt von außen zu beobachten:
    Code (Java):
    public interface DownloadUpdater {  
        public abstract void updateComponent(float currentPercent, long currentBytes);  
       
        public abstract void downloadFinished(boolean success);
    }
    Das Record mit den benötigten Daten:
    Code (Java):

    public class DownloadRecord {
     private String url;
     private String fileDestination;
     
     public DownloadRecord(String url, String fileDestination) {
      this.url = url;
      this.fileDestination = fileDestination;
     }
     
     public void verifyRecord() throws RecordVerificationException {
      // Prüfe die Dateitypen von der Datei hinter dem Downloadlink und der lokalen Datei:
      String urlExtension = url.substring(url.lastIndexOf(".") + 1);
      String fileExtension = fileDestination.substring(fileDestination.lastIndexOf(".") + 1);
      if(!urlExtension.equals(fileExtension))
       throw new RecordVerificationException("File extensions do not match to each other.");
     }
     
     
     public String getUrl() {
      return url;
     }
     public void setUrl(String url) {
      this.url = url;
     }
     public String getFileDestination() {
      return fileDestination;
     }
     public void setFileDestination(String fileDestination) {
      this.fileDestination = fileDestination;
     }
     
     
     public static class RecordVerificationException extends Exception {
      private static final long serialVersionUID = -7267259284068249626L;
     
      public RecordVerificationException(String message) {
       super(message);
      }  
     }
    }
     
    Der DownloadHandler
    Code (Java):

    class Downloader extends Thread {
     private boolean isDownloadFinished, isDownloadCanceled, isDownloadAborted;
     private DownloadFile dFile;
     private DownloadRecord dRecord;
     private long bytesRead;
     private String resultMessage;
     
     Downloader(final DownloadRecord dRecord) {
      this.dRecord = dRecord;
      this.dFile = new DownloadFile(dRecord.getUrl());  
      this.isDownloadFinished = false;
      this.bytesRead = 0;
      this.resultMessage = "";
     }
     
     
     void initializeFileSize() throws IOException {
      this.dFile.calculateFileSize();
     }
     void download() {  
      this.isDownloadCanceled = false;
      this.isDownloadAborted = false;
      this.start();
     }
     @Override
     public void run() {
      try {
             URL url = new URL(dRecord.getUrl());
             URLConnection conn = url.openConnection();
             InputStream is = conn.getInputStream();
             BufferedOutputStream fOut = new BufferedOutputStream(new FileOutputStream(new File(dRecord.getFileDestination())));
             byte[] buffer = new byte[32 * 1024];
             int bytesRead = 0;
             while (!isDownloadCanceled && (bytesRead = is.read(buffer)) != -1) {
             this.bytesRead += bytesRead;
                 fOut.write(buffer, 0, bytesRead);
             }
             fOut.flush();
             fOut.close();
             is.close();  
             isDownloadFinished = true;
      } catch(Exception e) {
       if(DEBUG.ON)
        e.printStackTrace();
       this.resultMessage = e.getMessage();
       this.isDownloadAborted = true;
      }
     }
     
     
     DownloadFile getdFile() {
      return dFile;
     }
     
     boolean isDownloadFinished() {
      return isDownloadFinished;
     }
     long getBytesRead() {
      return bytesRead;
     }
     
     void setDownloadCanceled(boolean isDownloadCanceled) {
      this.isDownloadCanceled = isDownloadCanceled;
     }
     
     String getResultMessage() {
      return resultMessage;
     }
     
     boolean isDownloadAborted() {
      return isDownloadAborted;
     }
    }
     
    Das DownloadFile
    Code (Java):

    class DownloadFile {
     private String fileURL;
     private long fileSize;
     
     DownloadFile(String url) {
      this.fileURL = url;
     }
     
     
     void calculateFileSize() throws IOException {
      URL url = new URL(fileURL);
            URLConnection conn = url.openConnection();
      this.fileSize = conn.getContentLength();
     }
     
     long getFileSize() {
      return fileSize;
     }
    }
     
    Die eindeutige (/teilweise gewollte) Exception
    Code (Java):

    public class DownloadException extends Exception {
     private static final long serialVersionUID = 7026110735840228658L;
     
     public DownloadException(String message) {
      super(message);
     }
     
     public DownloadException(String message, Throwable innerException) {
      super(message, innerException);
     }
    }
     


    Das war es.
    Der eigentliche Gedanke, der mich dazu brachte dieses Modul zu entwickeln, war ein Modul zu haben, welches Dateien jeder Art aus dem Netz downloaden kann.
    Wichtig hierbei war mir, dass es in dem Konzept eine Möglichkeit gibt, den Downloadfortschritt von außen zu beobachten um so auf welche Weise auch immer dem Benutzer den aktuellen Stand mitzuteilen (sei es in einem Label, als Progressbar, in der Konsole usw...).

    Falls es jemanden gab der diesen Code lesen wollte und ihn auf Anhieb verstanden hat, wäre es super. Falls es Probleme beim Verständnis gab, bitte ich diese zu kommentieren.

    Danke
     
  2. Vielleicht helfen dir diese Grundlagen hier weiter: (hier klicken)
  3. SchwarzWeiß
    SchwarzWeiß Mitglied
    Da sich sonst keiner meldet, von mir ein paar minimale, kleinliche Anmerkungen:

    1. ich würde nicht von Thread erben, sondern das Interface Runnable implementieren, so hast du nie das Problem der Mehrfachvererbung

    2.
    Code (Java):
    final Downloader d = new Downloader(dRecord);
    Ich würde sprechende Namen verwenden, auf keinen fall nur "d", allermindestens "dl".

    3.
    Code (Java):
    while(!d.isDownloadFinished() && !d.isDownloadAborted()) {
    Diese Bedingung würde ich in eine Methode, bsp. namens "isDownloadRunning" auslagern.
     
  4. JuKu
    JuKu Aktives Mitglied
    Ich würde dir dringend empfehlen das Buch "Clean Code" zu kaufen. Und wenn du schon dabei bist, kann auch das Buch "Clean Coder" nicht schaden.

    Aber der Reihe nach.

    1. Leerzeilen
    Leerzeilen können nicht schaden (es sei denn bei extremen Gebrauch) und verbessern die Übersichtlichkeit.
    Als erstes würde ich persönlich nach dem Klassenaufruf eine Leerzeile machen:
    Code (Java):
    public class DownloadManager {

        private static final long UPDATE_INTERVAL = 100;
     
        //other code

    }
    Oder hier:
    Code (Java):
    float percent = 0.00f;
    long bytesRead = 0;

    while(!d.isDownloadFinished() && !d.isDownloadAborted()) {

        //do somethign
       
    }
    2. Kommentare
    Dein Code enthält auf den ersten Blick keinen einzigen Kommentar! Wie soll er da gut sein? Siehst du da überhaupt selbst noch durch?

    3. Variablennamen
    Bitte sinnvolle Variablen Namen verwenden.

    Schlechtes Beispiel:
    Code (Java):
    final Downloader d = new Downloader(dRecord);
    Besseres Beispiel:
    Code (Java):
    final Downloader downloader = new Downloader(dRecord);
    4. Lambdas nutzen (Java 8)
    Seit Java 8 gibt es Lambdas und sie verbessern die Lesbarkeit & Übersichtlichkeit.

    Beispiel ohne Lambdas (dein Code):
    Code (Java):
    new Thread() {
        public void run() {

        //do something

        }

    }.start();
    Beispiel mit Lambda:
    Code (Java):
    new Thread(() -> {
        //do something
    }).start();
    5. this verwenden
    Auch wenn es in deinem Fall keinen Unterschied macht, ist this immer besser.
    initializeFileSize(d) --> this.initializeFileSize(d)

    6. Klammern setzen
    Die SSL Heartbleed Sicherheitslücke ist genau deshalb entstanden, weil einer die Klammern vergessen hat und dadurch eine Überprüfung des Pufferüberlaufs nicht stattgefunden hat.

    Statt:
    Code (Java):
    if(DEBUG.ON)
        e.printStackTrace();
    Besser so:
    Code (Java):
    if(DEBUG.ON) {
        e.printStackTrace();
    }
    7. JavaDoc
    Du solltest JavaDoc Kommentare verwenden.

    8. Scope nutzen
    Du solltest public, protected oder private vor jede Methode und jede Variable (außerhalb von Methoden) verwenden.

    Statt:
    Code (Text):
    DownloadFile getdFile() {
        return dFile;
    }
    Besser:
    Code (Java):
    public DownloadFile getdFile() {
        return this.dFile;
    }
    Ich glaube das reicht erstmal für den Anfang. :D
     
  5. mrBrown
    mrBrown Bekanntes Mitglied
    Das war doch Absicht: ;)
    Abgesehen davon: ich weiß, ist von vielen nicht gern gehört - aber wenn der Code ohne Kommentare unverständlich ist, ists schlechter Code ;) diesen hier fand ich durchaus verständlich...
     
  6. JuKu
    JuKu Aktives Mitglied
    Wie geil! :D

    Stimmt! Gebe dir vollkommen recht! :D
     
  7. truesoul
    truesoul Aktives Mitglied
    Hi.

    Eigentlich wurde auf alles schon hingewiesen und der Code ist in Ordnung und ohne den Kommentaren verständlich.
    Lambdas sind cool aber ehrlich gesagt machen die den gut nicht immer lesbarer.

    Schaue dir mal ThreadPoolExecutor an. Nur als Tipp.

    Grüße
     
    Javinner gefällt das.
  8. mrBrown
    mrBrown Bekanntes Mitglied
    In dem Satz könnte man Lamda mit jedem Sprachmittel ersetzen :p
     
    JuKu gefällt das.
  9. Flown
    Flown Administrator Mitarbeiter
    Mmn. ist dieser Downloadmanager ein wenig overengineered. Könnte viel kürzer ausfallen und das Gleiche wirken.
    Code (Java):
    import java.io.File;
    import java.io.IOException;
    import java.io.InputStream;
    import java.io.OutputStream;
    import java.net.URL;
    import java.nio.file.Files;
    import java.nio.file.StandardOpenOption;
    import java.util.Collection;
    import java.util.Objects;
    import java.util.concurrent.CopyOnWriteArrayList;
    import java.util.concurrent.ExecutionException;
    import java.util.concurrent.ExecutorService;
    import java.util.concurrent.Executors;
    import java.util.concurrent.Future;

    public class Test {
      public static void main(String... args) {
        DownloadProgress console = new DownloadProgress() {
          @Override
          public void progress(long size, long current) {
            System.out.println(size + " - " + current);
          }
         
          @Override
          public void finished(boolean errors) {
            System.out.println("Finished with errors: " + errors);
          }
        };
        DownloadManager downloadManager = new DownloadManager("http://www.google.at", "C:\\temp\\google.html");
        downloadManager.addListener(console);
        Future<File> fileFuture = downloadManager.downloadAsync();
        try {
          File file = fileFuture.get();
          System.out.println(file.length());
        } catch (InterruptedException e) {
          e.printStackTrace();
        } catch (ExecutionException e) {
          e.printStackTrace();
        }
      }
    }

    interface DownloadProgress {
      public void progress(long size, long current);
     
      public void finished(boolean errors);
    }

    class DownloadManager {
      private final String source;
      private final String destination;
     
      private Collection<DownloadProgress> listener = new CopyOnWriteArrayList<>();
     
      public DownloadManager(String source, String destination) {
        this.source = Objects.requireNonNull(source);
        this.destination = Objects.requireNonNull(destination);
      }
     
      public File download() throws IOException {
        File result = new File(destination);
        URL destinationURL = new URL(source);
        long estimatedSize = destinationURL.openConnection().getContentLengthLong();
        try (InputStream inputStream = destinationURL.openStream();
             OutputStream outputStream = Files.newOutputStream(result.toPath(), StandardOpenOption.CREATE, StandardOpenOption.WRITE)) {
          byte[] buffer = new byte[32 * 1024];
          int length;
          long currentLength = 0L;
          while ((length = inputStream.read(buffer)) != -1) {
            outputStream.write(buffer, 0, length);
            fireProgress(estimatedSize, (currentLength += length));
          }
          fireFinished(false);
        } catch (IOException e) {
          fireFinished(true);
        }
        return result;
      }
     
      public Future<File> downloadAsync() {
        ExecutorService executorService = Executors.newSingleThreadExecutor();
        try {
          return executorService.submit(this::download);
        } finally {
          executorService.shutdown();
        }
      }
     
      public void addListener(DownloadProgress progress) {
        listener.add(progress);
      }
     
      public boolean removeListener(DownloadProgress progress) {
        return listener.remove(progress);
      }
     
      private void fireFinished(boolean isError) {
        listener.forEach(l -> l.finished(isError));
      }
     
      private void fireProgress(long max, long cur) {
        listener.forEach(l -> l.progress(max, cur));
      }
    }
     
    truesoul gefällt das.
  10. JuKu
    JuKu Aktives Mitglied
    Ich kenne den ThreadPoolExecutor ziemlich gut und verstehe gerade nicht ganz, wieso Lambdas den Code dort unübersichtlicher machen sollten. Könntest du bitte etwas Beispiel Code zur Verfügung stellen? :D
     
  11. truesoul
    truesoul Aktives Mitglied
    Ich habe nicht speziell ThreadPoolExecutor gemeint mit. Wobei unübersichtlicher habe ich auch nicht geschrieben ;)
     
  12. Hast du dir unsere Java-Grundlagen hier schon gesichert? *Klick*
Die Seite wird geladen...

Feedback zum Downloadmodul bitte - Ähnliche Themen

Feedback erwünscht
Feedback erwünscht im Forum Java Basics - Anfänger-Themen
[Feedback erwünscht] Spring Boot Screencasts
[Feedback erwünscht] Spring Boot Screencasts im Forum Allgemeines EE
Berechtigungskonzept - Bitte um Feedback
Berechtigungskonzept - Bitte um Feedback im Forum Softwareentwicklung
Euer Feedback zu meinem Code ist gefragt
Euer Feedback zu meinem Code ist gefragt im Forum Spiele- und Multimedia-Programmierung
Feedback für neues Buch über "Java und Datenbanken" erwünscht
Feedback für neues Buch über "Java und Datenbanken" erwünscht im Forum Datenbankprogrammierung
Thema: Feedback zum Downloadmodul bitte