Java Wie guten Code erstellen/designen

[SparkMonkay]

Moinsen,

in letzter Zeit beschaeftige ich mich mehr damit guten Code zu fabrizieren. Darunter verstehe ich dass der Code verstaendlich sein soll und nur das tut wofuer er geschrieben wurde.

Jetzt bin ich an einer Stelle bei mir angekommen und wuerde gerne fragen wie ich es hier gescheit anstellen koennte.

Mein Vorhaben ist folgendes:
Ich moechte einen Server schreiben der Nachrichten von User A nach B wandern laesst. Dafuer brauche ich etwas was diese User annimmt. Also ein ServerSocketChannel und ich habe das Teil einfach in eine eigene Klasse gepackt und die Acceptor genannt. Hier zu sehen.

package server;

import java.io.Closeable;
import java.io.IOException;
import java.net.ServerSocket;
import java.net.Socket;
import java.net.SocketAddress;
import java.net.SocketOption;
import java.nio.channels.ServerSocketChannel;
import java.nio.channels.SocketChannel;
import java.nio.channels.spi.SelectorProvider;
import java.util.HashSet;
import java.util.Set;

public class ClientAcceptor extends ServerSocketChannel implements Runnable {

    ServerSocketChannel serverSocketChannel;
    protected HashSet<SocketChannel> socketChannels = new HashSet<>();

    /**************************************************************************
     * CONSTRUCTOR
     **************************************************************************/

    /**
     * Initializes a new instance of this class.
     *
     * @param provider The provider that created this channel
     */
    protected ClientAcceptor(SelectorProvider provider) {
        super(provider);
    }

    /**************************************************************************
     * METHODS
     **************************************************************************/

    public Set<SocketChannel> getAccepted() {
        HashSet<SocketChannel> socketChannels = this.socketChannels;
        this.socketChannels.clear();
        return socketChannels;
    }

    //
    //      overriden from runnable
    //
    @Override
    public void run() {
        SocketChannel socketChannel = null;
        try {
            while (true) {
                socketChannel = serverSocketChannel.accept();
                if (socketChannel != null) socketChannels.add(socketChannel);
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    //
    //      overriden from serversocketchannel
    //

    @Override
    public ServerSocketChannel bind(SocketAddress local, int backlog) throws IOException {
        return serverSocketChannel.bind(local, backlog);
    }

    @Override
    public <T> ServerSocketChannel setOption(SocketOption<T> name, T value) throws IOException {
        return serverSocketChannel.setOption(name, value);
    }

    @Override
    public <T> T getOption(SocketOption<T> name) throws IOException {
        return getOption(name);
    }

    @Override
    public Set<SocketOption<?>> supportedOptions() {
        return serverSocketChannel.supportedOptions();
    }

    @Override
    public ServerSocket socket() {
        return serverSocketChannel.socket();
    }

    @Override
    public SocketChannel accept() throws IOException {
        return serverSocketChannel.accept();
    }

    @Override
    public SocketAddress getLocalAddress() throws IOException {
        return serverSocketChannel.getLocalAddress();
    }

    @Override
    protected void implCloseSelectableChannel() throws IOException {
        serverSocketChannel.close();
    }

    @Override
    protected void implConfigureBlocking(boolean block) throws IOException {
        serverSocketChannel.configureBlocking(block);
    }

}

Wie initialisiere ich das Teil jetzt sauber?

Daran haenge ich jetzt was laenger und finde so keine Loesung die mir gefaellt.

//Edit
Genrelle Frage zu dem Thema: Clean Code
Was gibt es wichtiges einzuhalten/beachten?

 

[thecain]

Und warum hast du es extendet, wenn du genau die selbe Logik noch einmal verwendest?

 

[SparkMonkay]

Ich habe da etwas nicht drinnen, aendere es sofor ab.

*EDITIERE DEN STARTPOST*

Der Acceptor soll in einem Thread laufen, daher implementiere ich da 'Runnable' und soll die ganze Zeit einfach Clients pullen in die in das Set packen. Dann wenn mir danach ist rufe ich ueber die Methode 'getAccepted' und habe alle Connections die in der zwischenzeit eingegangen und angenommen wurden.

@thecain
Weil ich mir dachte dass ich den Acceptor als 'Startpunkt' fuer den Server nehmen. Daher dachte ich mir das dass Teil ein ServerSocketChannel ist, denn es geht die Connections mit den eingehenden Connections ein. Es ist halt nur im die Funktion erweitert worden dass es etwas nutzt womit es in einem Thread laufen kann

 

[VikingGe]

    public Set<SocketChannel> getAccepted() {
        HashSet<SocketChannel> socketChannels = this.socketChannels;
        this.socketChannels.clear();
        return socketChannels;
    }

3 Zeilen Code, aber du trittst in jedes vorhandene Fettnäpfchen.

a) Du willst dein Set wahrscheinlich kopieren. In Java kann man keine Objekte ohne Weiteres kopieren, du duplizierst lediglich die Referenz und löschst das gesamte HashSet - aber die Kopie ist nicht nötig. Einfach ein neues HashSet erstellen und der Membervariable zuweisen.

b) Du greifst von zwei verschiedenen Threads auf ein- und dieselbe Struktur zu, ohne die Zugriffe zu synchronisieren. Das wird dir um die Ohren fliegen - wahrscheinlich nur sehr selten, aber das macht den Fehler umso nerviger.

c) Warum überhaupt ein HashSet? Eigentlich ist das die völlig falsche Datenstruktur für das, was du vorhast. HashSet ist dann sinnvoll, wenn du Duplikate verhindern willst (können hier eigentlich gar nicht auftreten) oder prüfen willst, ob bestimmte Objekte im HashSet vorhanden sind oder nicht - was aber nicht geht, weil du ja nicht weißt, was drinsteht. Was du eigentlich willst, ist eine Queue (z.B. LinkedList in Java).

Mal so als Beispiel (sieh es als Pseudocode, nicht getestet):

public class ... {
  
  private LinkedList<SocketChannel> connectionFrontBuffer = new LinkedList<SocketChannel>();
  private LinkedList<SocketChannel> connectionBackBuffer  = new LinkedList<SocketChannel>();
  
  [...]
  
  public void processConnections(Consumer<SocketChannel> function) {
    LinkedList<SocketChannel> connections;
    synchronized (this) {
      // Queues tauschen, damit der andere Thread auf einer
      // leeren Queue arbeitet 
      connections = this.connectionBackBuffer;
      this.connectionBackBuffer  = this.connectionFrontBuffer;
      this.connectionFrontBuffer = connections;
    }
    for (SocketChannel connection : connections)
      function.accept(connection);
    connections.clear();
  }

  [...]
  
  public void run() {
    [...]
        if (socketChannel != null) synchronized(this) {
          this.connectionBackBuffer.add(socketChannel);
        }
    [...]
  }
};

d) Dein Modell basiert darauf, dass du an irgendeiner Stelle selbst die Connections pollst. Warum nicht einfach ein Callback, welches aufgerufen wird, sobald der Thread eine Verbindung hat?

Weil ich mir dachte dass ich den Acceptor als 'Startpunkt' fuer den Server nehmen. Daher dachte ich mir das dass Teil ein ServerSocketChannel ist, denn es geht die Connections mit den eingehenden Connections ein. Es ist halt nur im die Funktion erweitert worden dass es etwas nutzt womit es in einem Thread laufen kann

Vererbung ist aber da das falsche Mittel. Du sagst damit, dein Acceptor ist-ein ServerSocketChannel, und implizierst, dass er auch genau wie einer benutzt werden kann. Dass du also zum Beispiel problemlos von außen die accept-Methode aufrufen kannst. Ergibt das Sinn? Klares Nein.

Du benutzt innerhalb deines Acceptors einen ServerSocketChannel, um dessen Low-Level-Interface zu verstecken und den ganzen Kram mit accept etc. von einem anderen Thread aus auszuführen. Du willst ein einfacheres Interface, nicht noch einmal das gleiche. Ein paar Funktionen wirst du wrappen müssen (z.B. bind - am besten die Parameter dafür einfach schon im Konstruktor übergeben), die meisten kannst du aber komplett weglassen.

in letzter Zeit beschaeftige ich mich mehr damit guten Code zu fabrizieren. Darunter verstehe ich dass der Code verstaendlich sein soll und nur das tut wofuer er geschrieben wurde.

Generell empfiehlt es sich, seine Klassen so zu designen, dass die Objekte nie in einen ungültigen Zustand geraten können. Konkret heißt das: So viel wie möglich immutable machen. Veränderbarer Zustand bringt haufenweise Fehlerquellen mit sich, ist oft nicht nötig und behindert Multithreading (siehe oben) - und wenn sowas schon nötig ist, dann sorge dafür, dass das Objekt eben nach jedem (korrekten) Methodenauf gültig bleibt und man nicht zwangsweise zwei Methoden hintereinander ausführen muss oder sowas. An je weniger Regeln man sich halten muss, desto weniger Fehler können auch passieren.

Vor allem die in Java recht beliebten Setter-Methoden für irgendwelche Member-Variablen sind ungefähr so toll wie ein Krebsgeschwür. Wenn man schon keine Encapsulation will, dann sollte man auch nicht so tun, als hätte man welche - dass JavaBeans nur so funktionieren, mag irgendwo Gründe haben, aber solche "Designs" haben in normalem Code nichts verloren.