Confusing control flow

Currently, the following code runs and produces the expected and desired output. However, I'm sure that there's much wrong with it.

This project queries the (flat) database, instantiates each row as an object, adds those objects to a queue, and makes the queue available to clients. Clients may then connect to the server, which pops objects from the queue and delivers that single object to the client. The client receives the object, the user updates it, and then it is sent back to the server. The server then updates the database accordingly.

So far as I can tell, this is functioning correctly, and allows for multiple clients.

However, I'm quite sure that the control flow is "odd", or at least sub-optimal.

How can I make the code clearer by using, for example, producer/client or another pattern or idiom?

WorkerRunnable:

```
package net.bounceme.dur.server.streams;

import net.bounceme.dur.jdbc.Title;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.net.Socket;
import java.util.NoSuchElementException;
import java.util.logging.Level;
import java.util.logging.Logger;
import net.bounceme.dur.jdbc.Queries;

public class WorkerRunnable implements Runnable {

private static final Logger log = Logger.getLogger(WorkerRunnable.class.getName());
protected Socket socket = null;
private RecordsQueueWrapper recordsQueue = null;
private final Queries queries = new Queries();

public WorkerRunnable(Socket clientSocket, RecordsQueueWrapper recordsQueue) {
this.socket = clientSocket;
this.recordsQueue = recordsQueue;
}

@Override
public void run() {
Title serverTitle = null;
Title clientTitle = null;
boolean queueEmpty = false;
try (ObjectOutputStream objectOutputStream = new ObjectOutputStream(socket.getOutputStream());
ObjectInputStream objectInputStream = new ObjectInputStream(socket.getInputStream())) {
d

The two main issues I see are not really code flow related. One is that you don't really have a protocol to communicate with your clients (as in, reporting success or failure, interpreting commands, et cetera).

The other is that your server may be prone to stalling. Your serving loop looks like this:

• Accept connection

• Send object to client

• Wait for an edited record ← possible issue

• Write down edited record

• Close connection

Assuming that users manually edit the records you serve them, you have the risk that someone opens a connection, finds out she's out of coffee, and heads off for ten minutes (or lunch for an hour). That leaves a connection stuck until she returns or it times out.

If we move the user interaction to be offline, we limit chances of hogging your worker pool:

• Accept connection

• Read edited record from client, if any

• Write down edited record

• Send new record to client

• Close connection

This does require a change in protocol, because now you have to recognise two scenarios:

• Your user has finished editing a record and sends it over.

• Your user is freshly starting and wants a new record to edit.

We would already need a change in protocol, because there are a number of messages that we may want to send to the client:

• I'm out of records.

• You sent me a record that's not in my database.

• You sent me a query I don't understand.

• ...

So let's introduce a kind of payload container that will sort this out for us:

/** Base class representing a response to a query. */
class Response implements java.io.Serializable {
  /** Indicates success or failure. */
  int code = CODE_SUCCESS; // consider re-using HTTP codes
  /** Informative message to go with the code. */
  String message = "";
  Title record;
}

class RecordQuery implements java.io.Serializable {
  /** Record the client has modified and wants saved.  May be null. */
  Title edited;
  /** True if the client wants another record to edit, false otherwise. */
  boolean sendMeMore;
}

// WorkerRunnable.run()
public void run() {
  try (ObjectOutputStream objectOutputStream = new ObjectOutputStream(socket.getOutputStream());
          ObjectInputStream objectInputStream = new ObjectInputStream(socket.getInputStream())) {
      final Object input;
      try {
        input = objectInputStream.readObject();
      } catch (ClassNotFoundException ex) {
        log.log(Level.FINE, "Unknown class in request", ex);
        objectOutputStream.writeObject(new Response(Response.CODE_UNKNOWN_REQUEST, "Huh?"));
        return;
      }
      if ( input instanceof RecordQuery ) {
        final RecordQuery query = (RecordQuery) input;
        if ( query.edited != null ) {
          queries.updateTitle(query.edited);
        }
        if ( query.sendMeMore ) {
          final Title nextTitle = queries.poll(); // null on empty
          if ( nextTitle != null ) {
            objectOutputStream.writeObject(new Response(nextTitle));
          } else {
            objectOutputStream.writeObject(new Response(Response.CODE_NO_MORE_RECORDS));
          }
        }
      } else {
        objectOutputStream.writeObject(new Response(Response.CODE_UNKNOWN_REQUEST, "Huh?"));
      }
  } catch (IOException ex) {
      log.log(Level.SEVERE, null, ex);
  }
}

But now WorkerRunnable.run() has multiple responsibilities. It parses input, determines a reply, and sends that reply. We can filter some things out:

public void run() {
  try (ObjectOutputStream objectOutputStream = new ObjectOutputStream(socket.getOutputStream());
          ObjectInputStream objectInputStream = new ObjectInputStream(socket.getInputStream())) {
      Response response = new Response(Response.CODE_UNKNOWN_REQUEST, "Huh?");
      Object input;
      try {
        input = objectInputStream.readObject();
      } catch (ClassNotFoundException ex) {
        log.log(Level.FINE, "Unknown class in request", ex);
        objectOutputStream.writeObject(response);
        return;
      }
      response = getReplyTo(input);
      objectOutputStream.writeObject(response);
  } catch (IOException ex) {
    log.log(Level.SEVERE, null, ex);
  }
}

protected Response getReplyTo(Object request) {
  if ( !(input instanceof RecordQuery) ) {
    return new Response(Response.CODE_UNKNOWN_REQUEST, "Huh?");
  }

  final RecordQuery query = (RecordQuery) input;
  if ( query.edited != null ) {
    queries.updateTitle(query.edited);
  }
  if ( query.sendMeMore ) {
    final Title nextTitle = queries.poll(); // null on empty
    if ( nextTitle != null ) {
      return new Response(nextTitle);
    } else {
      return new Response(Response.CODE_NO_MORE_RECORDS);
    }
  }
}

If inventing/implementing your own protocol seems a bit much (and it usually is), you could consider using an HTTP library or a general I/O framework like Apache MINA. But we wouldn't do the tag "reinventing the wheel" justice if we didn't go all the way, eh?

StackExchange Code Review Q#56972, answer score: 4