Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wanted to practice using sockets and multithreading. This is simple code where I start a Server and connect to it via Client and chat between them.

Server class:

public class Server {
    public static void main(String[] args) throws Exception{
        ServerSocket serverSocket = new ServerSocket(4545);
        SocketEnhancer socketEnhancer = new SocketEnhancer(serverSocket.accept());
    }
}

Client class:

public class Client {
    public static void main(String[] args) throws Exception{
        SocketEnhancer socketEnhancer = new SocketEnhancer(new Socket("localhost",4545));
    }
}

SocketEnchancer:

public class SocketEnhancer {

    DataReceiver dataReceiver;
    DataSender dataSender;
    Socket socket;

    public SocketEnhancer(Socket socket){
        this.socket = socket;
        dataReceiver = new DataReceiver(this);
        dataSender = new DataSender(this);
        new Thread(dataSender).start();
        new Thread(dataReceiver).start();
    }

    public Socket getSocket() {
        return socket;
    }
}

DataReceiver:

public class DataReceiver implements Runnable {

    SocketEnhancer socketEnhancer;

    public DataReceiver(SocketEnhancer socketEnhancer) {
        this.socketEnhancer = socketEnhancer;
    }

    @Override
    public void run() {
        try {
            BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(socketEnhancer.getSocket().getInputStream()));
            while (true) {
                String readLine = bufferedReader.readLine();
                if(readLine != null){
                    System.out.println("Data received: " + readLine);
                }
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}

DataSender:

public class DataSender implements Runnable {

    SocketEnhancer socketEnhancer;
    Scanner scanner = new Scanner(System.in);

    public DataSender(SocketEnhancer socketEnhancer) {
        this.socketEnhancer = socketEnhancer;
    }

    @Override
    public void run() {
        try {
            PrintWriter printWriter = new PrintWriter(socketEnhancer.getSocket().getOutputStream());
            while(true) {
                String readLine = scanner.nextLine();
                printWriter.write(readLine + System.lineSeparator());
                printWriter.flush();
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}

Any comments and criticism is appreciated.

share|improve this question

1 Answer 1

SocketEnhancer socketEnhancer = new SocketEnhancer(serverSocket.accept());

Currently, this is a not very advanced chat application as you can only have one client. It'll be a 1-1 conversation. It will just be one server and one client talking to each other. You can run the serverSocket.accept() in a loop to allow you to have more clients. The server then needs to loop through all the available clients (so you need some Collection for those) and then send the incoming message to all clients.


public static void main(String[] args) throws Exception{
    ServerSocket serverSocket = new ServerSocket(4545);
    SocketEnhancer socketEnhancer = new SocketEnhancer(serverSocket.accept());
}

You don't need to declare the socketEnhancer variable as you're not using it any more in the method. The compiler should give you an unused variable warning here.


DataReceiver dataReceiver;
DataSender dataSender;
Socket socket;

private final, private final, private final. All of those should be private final.

The same goes for all your fields in other classes.


dataReceiver = new DataReceiver(this);
dataSender = new DataSender(this);

So you create a DataReceiver and a DataSender and give them both the SocketEnhancer reference? Let's see how you use that in the classes then.

new InputStreamReader(socketEnhancer.getSocket().getInputStream())

and

socketEnhancer.getSocket().getOutputStream()

You send the entire socketEnhancer only so that you can use the getSocket method on it once to create the OutputStream. It is much better to give them the InputStream / OutputStream directly. This is what is called Tell, Don't Ask!

When possible, don't give your classes an object they can ask about something, give them what they need directly.


public class DataSender implements Runnable {

    Scanner scanner = new Scanner(System.in);
    ...

I would recommend moving the Scanner to your main method, and letting your main method call a method on your SocketEnhancer object whenever it wants to send a message. The SocketEnhancer can then call a method on dataSender to perform the sending.


Overall, I have to say that your code is quite clean and easy to read. Good job.

share|improve this answer
    
Thanks, great tips. –  Koray Tugay Jul 5 at 19:13

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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