Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

As practice I wanted to write a Socket in Java:

/* User: [email protected] Date: 21/02/15 Time: 13:30 */

import java.io.IOException;
import java.net.ServerSocket;

public class MyServer {

    public static void main(String[] args) throws IOException {

        ServerSocket serverSocket = new ServerSocket(8888);

        while (true) {
            new Thread(new ServerSocketThread(serverSocket.accept())).start();
        }
    }

}

and the rest of it:

/* User: [email protected] Date: 21/02/15 Time: 18:14 */

import java.io.*;
import java.net.Socket;
import java.util.Scanner;

public class ServerSocketThread implements Runnable {

    Socket socket;

    public ServerSocketThread(Socket accept) {
        this.socket = accept;
    }

    @Override
    public void run() {
        try {

            Scanner s = new Scanner(socket.getInputStream());
            String readLine;

            while (!(readLine = s.nextLine()).equals("bye")) {
                System.out.println(readLine);
            }

            new PrintWriter(socket.getOutputStream()).write("Bye then..");
            socket.close();

        } catch (IOException e) {
            e.printStackTrace();
        }

    }
}

I wanted to write it as clean as possible. Any improvements, suggestions?

I can use it like this:

Korays-MacBook-Pro:~ koraytugay$ telnet localhost 8888
Trying ::1...
Connected to localhost.
Escape character is '^]'.
koray
tugay
asdfako
bye
Connection closed by foreign host.
share|improve this question
1  
Your title should only state what your code does, and not what concerns you have. Also, please give a brief explanation in what your code exactly does. – TheCoffeeCup Feb 21 '15 at 17:53
up vote 1 down vote accepted

There's not a lot to say about such a simple program, mainly nitpicks:

  • Member fields that you don't need to expose to the outside should be private, such as the socket in ServerSocketThread.

  • Whenever you can, make member fields final, for example the socket in ServerSocketThread.

  • The Socket parameter in the constructor of ServerSocketThread is poorly named accept. It would be better to call it what it is: socket

  • s for a Scanner is not a great name either. How about scanner ?

  • It's a bit odd that the server doesn't talk back to the client, just prints stuff on its console.

  • If the program gets more complex later:

    • It might be good to move the literal string like "bye" to a constant variable, as it's somewhat special, being the special terminating sequence
    • Printing stack trace on the console is considered bad practice
    • Printing pretty much anything to the console is considered bad practice

    However, at this point, your program being a toy, it doesn't really matter, just for the record...

share|improve this answer
    
I tried to make it as readable as possible. – Koray Tugay Feb 21 '15 at 17:58
    
Oh you succeeded, it's definitely nicely readable! (It will still be so after applying my suggestions ;-) – janos Feb 21 '15 at 17:59

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.