HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Chatroom log in with no 3rd party libraries

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
log3rdwithlibrariespartychatroom

Problem

So, I'm creating a chatroom with no 3rd party libraries and currently I'm logging people in like this:

public void run() {
    try {
        this.running = true;
        ServerSocket ss = new ServerSocket(1000);
        while (this.running) {
            this.threadPool.execute(new InfoGrabber(ss.accept()));
        }
    } catch (IOException ex) {
        Logger.getLogger(Listener.class.getName()).log(Level.SEVERE, null, ex);
    }
}

private class InfoGrabber implements Runnable {

    private final Socket socket;

    public InfoGrabber(Socket socket) {
        this.socket = socket;
    }

    @Override
    public void run() {
        try {
            ObjectInputStream ois = new ObjectInputStream(socket.getInputStream());
            Object obj = ois.readObject();
            if (obj instanceof ClientInformation) {
                clientManager.addClient(new ClientClass((ClientInformation) obj, new ClientConnection(socket, ois, ((ClientInformation)obj).getUUID())));
                System.out.println("Client connected");
            }
        } catch (IOException | ClassNotFoundException ex) {
            Logger.getLogger(Listener.class.getName()).log(Level.SEVERE, null, ex);
        }
    }
}


Is there a better way to do this?
Git for more context https://github.com/CjStaal/ChatRoom
I have a systemQueue set up which takes SystemMessage which is a string, and an object. I want to be able to make this better, and have it more encapsulated and modular to allow easier maintainability and expandability. In the long run, I want it to be able to authenticate against a SQL database.

Solution

You have a number of missing links in your code, which makes a review hard to do.

Your ServerSocket code at first glance looks OK, but it is actually broken.

this.running = true;
    ServerSocket ss = new ServerSocket(1000);
    while (this.running) {
        this.threadPool.execute(new InfoGrabber(ss.accept()));
    }


You don't have a declaration for running. If it is a volatile boolean, then it will probably work. If it is not volatile, it'll possibly never work, depending on the setup. I would never recommend the use of volatile, though, so I would recommend an AtomicBoolean. Still, even with an atomic boolean, you may never exit your server loop because it only checks to see if it should stop after it has accepted a connection. The code will sit waiting on the accept() until something happens, even if the running is false. You need a different mechanism, or timeouts, on your code.

The actual socket you get is also badly handled. You create a thread to run the socket on, and you create an ObjectInputStream on the socket. This is OK, but you then schedule that on a thread which exits quickly... the ClientClass instance is registered on to the clientManager, but then no thread appears to be listening for more input from the client? Additionally, nothing has told the client that it is connected.

This code appears to be pretty new, and incomplete.

Code Snippets

this.running = true;
    ServerSocket ss = new ServerSocket(1000);
    while (this.running) {
        this.threadPool.execute(new InfoGrabber(ss.accept()));
    }

Context

StackExchange Code Review Q#86911, answer score: 2

Revisions (0)

No revisions yet.