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

Read partial socket message

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

Problem

I've written some code that is suppose to read socket all the time and when needed return the partial data from it. I just want the response to send a command. It works, but this is a bit messy implementation.

Is it possible to write it better?

public StringBuilder message = new StringBuilder();
private BufferedReader reader;
private Socket socket;
public boolean writing = true;
public boolean wantToRead = false;

public String getMessage() {
        synchronized (message) {
            while (writing) {
                try {
                    System.out.println("waiting");
                    wantToRead = true;
                    message.wait();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
            System.out.println("take the resource");
            writing = true;
            return message.toString().substring(0, message.toString().lastIndexOf("\n"));
        }
    }

@Override
    public void run() {
        int c;
        try {
            while((c = reader.read()) != -1) {
                synchronized(message) {
                    message.append(Character.toString((char) c));
                    if(!reader.ready()) {
                        System.out.println("end of response");
                        if(wantToRead) {
                            writing = false;
                            wantToRead = false;
                            message.notify();
                        }
                    }
                }
            }
            System.out.println("zamykam socket");
            socket.close();
        } catch (IOException e) {
            e.printStackTrace();
        }

Solution

You are locking on a public field message which might cause a deadlock. If locking is needed, then do it on a private field.

private final Object SYNC_OBJECT = new Object();
...
synchronized(SYNC_OBJECT){
}


Your class extends Thread apparently, and the rule is : favor composition over inheritance.

private Runnable runnable;

  public void startListening(){
    // use the runnable
  }


Moreover, you barely need to use threads, use Executors instead

Code Snippets

private final Object SYNC_OBJECT = new Object();
...
synchronized(SYNC_OBJECT){
}
private Runnable runnable;

  public void startListening(){
    // use the runnable
  }

Context

StackExchange Code Review Q#55591, answer score: 5

Revisions (0)

No revisions yet.