current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
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
up vote 11 down vote favorite
3

I am creating a simple server that accepts a int and returns the value received twice:

public class MyServer{

    private static int port = 1234;

    public static void main(String args[]){
        try{
            ServerSocket serversock=new ServerSocket(port);
            while(true){
                Socket  socket=serversock.accept();
                new Thread(new MyClass(socket)).start();
            }
        }catch(IOException e){}
        }
    }

    class MyClass implements Runnable{

        Socket socket;

        public MyClass(Socket s){socket = s;}

        public void run(){
            try{
                DataInputStream in = new DataInputStream(socket.getInputStream());
                DataOutputStream out = new DataOutputStream(socket.getOutputStream());
                int x = in.readInt();
                out.write(2*x);
                socket.close();
            }
            catch(IOException e){}
    }
}

My question is about redesigning the code. Are there any weaknesses in the design? If yes, then what changes would make the code better?

share|improve this question

migrated from stackoverflow.com Jan 8 '12 at 23:18

This question came from our site for professional and enthusiast programmers.
    
NEVER catch exceptions and eat them silently, as you are doing: catch(IOException e){} At least put e.printStackTrace(); between the braces. – Jesper Jan 19 '12 at 15:18
up vote 8 down vote accepted

If you really are planning on creating a thread for each incoming request just use and Executors.newCachedThreadPool(). As long as the incoming requests arent very often, this ExecutorService will create a new thread when you submit to it, but if a thread is idle and not being used, the service will reuse the thread instead of creating a new one.

Edit: Here is the example using your code.

public class MyServer{

    private static int port = 1234;


    public static void main(String args[]){
        try{
            final ExecutorService service = Executors.newCachedThreadPool();
            ServerSocket serversock=new ServerSocket(port);
            while(true){
                Socket  socket=serversock.accept();
                service.submit(new MyClass(socket));
            }
        }catch(IOException e){}
        }
    }

    class MyClass implements Runnable{

        Socket socket;

        public MyClass(Socket s){socket = s;}

        public void run(){
            try{
                DataInputStream in = new DataInputStream(socket.getInputStream());
                DataOutputStream out = new DataOutputStream(socket.getOutputStream());
                int x = in.readInt();
                out.write(2*x);
                socket.close();
            }
            catch(IOException e){}
    }
}

With this you have the possibility of thread reuse.

share|improve this answer
up vote 6 down vote

The ugly part that sticks out is that you are silently ignoring the exceptions... You should do some cleanup and close the sockets at least.

Improving a bit on the performance side, you are starting a new thread to execute a very small task, thus wasting a lot of time to start-up threads. You could instead submit the Runnables to an ExecutorService, provided that receiving a number from the client does not depend on user input (otherwise you will block the thread pool if the first few clients don't send anything)

share|improve this answer
    
Thankx for the valuable suggetion i am new to this, i am implementing Executerservice as ExecutorService es = newCachedThreadPool(); es.execute(new Double(socket)); in place of new Thread(new Double(socket)).start(); is this okay! – Rex Jan 9 '12 at 5:42
    
@Rex, yes, the cached thread pool creates a number of initial threads and then recycles them when there are new tasks to execute. If you want to specify a fixed number of threads to use you can use newFixedThreadPool(); – Tudor Jan 9 '12 at 8:33
1  
@Tudor newCachedThreadPool will actually only create threads on demand. It will start with 0 threads. – John V. Jan 9 '12 at 17:33
up vote 6 down vote

In addition to Tudor's response, you also shouldn't write infinite loops - that's just silly. For servers that are intended to run for an unknown amount of time (possibly endlessly), I do

while (keepServerRunning) {   // where 'keepServerRunning' is a simple boolean variable
  ...

...and then use a shutdown hook to set the value to false which allows the server (and the JVM) to exit normally, without having to kill the task via kill -9 or a force close.

While it's theoretically possible that a service will run forever, it's just not good practice to pretend like that happens in reality. Clean up after yourself, and provide your apps a simple, clean way to exit normally whenever possible.

share|improve this answer

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.