current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 10 down vote favorite
3

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

Here is my code:

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
    
sorry for the mistake . i have edited the question . –  Rex Jan 8 '12 at 22:35
    
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
add comment

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

This question came from our site for professional and enthusiast programmers.

3 Answers

up vote 7 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
    
thanks John, can u help me with a small example code for the same . –  Rex Jan 9 '12 at 5:51
    
@Rex - Sure take a look at my edit –  John V. Jan 9 '12 at 14:33
    
Thankx a lot @John –  Rex Jan 10 '12 at 13:45
add comment
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
    
ya the exceptions part is okay i can handle that later, i have closed the socket . i need some logical part to make the concurrency better. –  Rex Jan 8 '12 at 22:41
    
@Rex: see my edit. –  Tudor Jan 8 '12 at 22:45
    
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
show 1 more comment
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
    
thanks for the suggestion, can u help me with some small example code or a modification in my(above) code itself. –  Rex Jan 9 '12 at 5:49
1  
i am using this Runtime.getRuntime().addShutdownHook(new Thread() { @Override public void run() { keepServerRunning = false; } }); below the main method in the code written in the question. is this okay! thanks –  Rex Jan 9 '12 at 10:08
add comment

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.