4
\$\begingroup\$

I would like to add some capabilities to the server. Firstly, it should accept and handle connections with multiple clients, so that there are no mixups between clients. Secondly, there should be some very basic JDBC connectivity: serialize a result set as a List, and then, as requested pop from the list and send that instance to a client for updates. When the client sends back an updated record, update the database accordingly.

package net.bounceme.dur.driver;

import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.Properties;
import java.util.logging.Level;
import java.util.logging.Logger;

public class Server {

    private static final Logger log = Logger.getLogger(Server.class.getName());
    private final RecordQueue recordsQueue = new RecordQueue();

    public static void main(String[] args) {
        Properties props = PropertiesReader.getProps();
        int portNumber = Integer.parseInt(props.getProperty("port"));
        while (true) {
            try {
                new Server().inOut(portNumber);
            } catch (java.net.SocketException se) {
                Logger.getLogger(Server.class.getName()).log(Level.FINE, "spammy", se);
            } catch (IOException ioe) {
                Logger.getLogger(Server.class.getName()).log(Level.SEVERE, null, ioe);
            } catch (ClassNotFoundException cnf) {
                Logger.getLogger(Server.class.getName()).log(Level.INFO, null, cnf);
            }
        }
    }

    public void inOut(int portNumber) throws IOException, ClassNotFoundException, java.net.SocketException {
        ServerSocket serverSocket = new ServerSocket(portNumber);
        Socket socket = serverSocket.accept();
        ObjectOutputStream objectOutputStream = null;
        MyRecord recordFromClient = null;
        try (ObjectInputStream objectInputStream = new ObjectInputStream(socket.getInputStream())) {
            objectOutputStream = new ObjectOutputStream(socket.getOutputStream());
            recordFromClient = (MyRecord) objectInputStream.readObject();
        }
        objectOutputStream.flush();
        objectOutputStream.close();
        log.info(recordFromClient.toString());
    }
}

Is that a reasonable progression from this server code? Obviously, I would only add a single feature at a time. While I would love to learn Log4J or a similar testing framework, my immediate concern is adding functionality to server code.

What are some pitfalls I might run into? What would be the most pragmatic approach to increasing the functionality of the server side operations?

For example, I might start with a Queue, and then only later tie that into a database. Each client will only have access to a single record, so I'm not concerned about corrupt data. Will there be a problem when multiple clients are trying to access the Queue, however? The clients will only need pop and add, nothing more. The client will update, or modify, each record instance it receives.

\$\endgroup\$
  • 1
    \$\begingroup\$ While I would love to learn Log4J or a similar testing framework => Log4J is not a testing framework, it's a logging framework. junit.org is a testing framework. \$\endgroup\$ – chillworld Jun 30 '14 at 9:21
7
\$\begingroup\$

Using imports:

catch(java.net.SocketException se) { /* [...] */
throws java.net.SocketException { /* [...] */

fully qualifying these is unnecessary:

import java.net.SocketException;

catch(SocketException se) { /* [...] */
throws SocketException { /* [...] */

I feel that this code is much more concise, while containing the same information.

Using try-with-resources:

First: You are using try-with-resources. Good. That's the way to go.
Second: You're doing it wrong.

    try (ObjectInputStream objectInputStream = new ObjectInputStream(socket.getInputStream())) {
        objectOutputStream = new ObjectOutputStream(socket.getOutputStream());
        recordFromClient = (MyRecord) objectInputStream.readObject();
    }
    objectOutputStream.flush();
    objectOutputStream.close();

try-with resources does the last two things for you. You achieve the exact same result when you leave those out. The last two statments become useless clutter if you used try-with-resources:

try (ObjectInputStream objectInputStream = new ObjectInputStream(socket.getInputStream());
    ObjectOutputStream objectOutputStream = new ObjectOutputStream(socket.getOutputStream())) {
     recordFromClient = (MyRecord) objectInputStream.readObject();
}

Apart from that... You don't use your objectOutputStream anywhere. Why do you have it?

Using exceptions / logging:

Why is your ClassNotFoundException only logged as INFO? I'd expect a ClassNotFoundException to be minimum ERROR, if not FATAL! If you "expect" ClassNotFoundExceptions, then your design might be flawed.

\$\endgroup\$
  • \$\begingroup\$ I'll do a bit of nitpicking but it's not true that try-with-resource is doing the flush(). The try is closing the stream and the close action automatically flush the stream. The flush() before was redundant even before try-with-resource. (No need to edit your answer, just wanted to make a precision) \$\endgroup\$ – Marc-Andre Jun 30 '14 at 13:55
  • 1
    \$\begingroup\$ cfr. ClassNotFoundException: as it stands, the code deserializes foreign objects, and it has no guarantees that it knows about the classes of these objects, so ClassNotFoundException basically means "I got a request that I don't understand." Logging that as an error would be overkill. \$\endgroup\$ – JvR Jun 30 '14 at 13:55
  • \$\begingroup\$ @JvR what are the options besides "foreign" objects? How can it guarantee that it knows about the classes of these objects? \$\endgroup\$ – Thufir Jul 1 '14 at 14:48
  • \$\begingroup\$ @Thufir Unfortunately, you can't guarantee that your clients will only send you well-formed, known messages, so you cannot avoid the scenario. (This is not unique to Java serialisation; HTTP requests can be malformed, too). You should narrow the scope of your catch, though, and put it directly with the call to readObject(), or wrap the exception in an explicit "I don't know what you're saying" exception and throw that. \$\endgroup\$ – JvR Jul 21 '14 at 16:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.