Implementing a Java server

Question

My biggest problem is attempting to remove a client when the "disconnect" button is hit. It doesn't work properly, and I'm not sure if my approach is best: sending out a list of all the current users to the clients.

Any major logic/coding flaws and any suggestions for improvement would also be welcome.

Server class:

import java.io.IOException;
import java.io.PrintWriter;
import java.net.ServerSocket;
import java.util.ArrayList;
import java.util.Collections;

public class Server {
static ArrayList<String> users = new ArrayList<String>();
static ArrayList<PrintWriter> outG = new ArrayList<PrintWriter>();
static int port=4444;

// ip address: 192.168.0.104


public static void main(String[] args) throws IOException {
    Collections.synchronizedList(users);
    ServerSocket serverSocket = null;
    try {
        serverSocket = new ServerSocket(port);
        System.out.print("success");
    } catch (IOException e) {
        System.err.println("Could not listen on port: "+port);
        System.exit(1);
    }
    try {
        while(true){
            Thread thread = new Thread(new UserThread(serverSocket.accept()));
            thread.start();
        }
    } catch (IOException e) {
        System.err.println("Accept failed.");
        System.exit(1);
    }
    serverSocket.close();
}
}

User Thread (when a client tries to connect):

 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.PrintWriter;
 import java.net.Socket;
 import java.util.ArrayList;

 public class UserThread extends Server implements Runnable{
private Socket socket = null;
String message="";
String name="";
String id="";
public UserThread(Socket sock){
    this.socket=sock;
}
@Override
public void run() {
    try {
        PrintWriter out = new PrintWriter(socket.getOutputStream(), true);
        BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
        addWriter(outG,out);
        boolean temp2=true;
        while(temp2){
            boolean temp3=true;
            String name = readIn(in,true);
            for(int i=0;i<getArraySize(users);i++){//trying to make sure that one client doesnt 
                if(getArray(users,i).equals(name)){//enter a name that another already typed in
                    sendOut(out,"no",true,false,false);
                    temp3=false;
                    i=getArraySize(users);
                }
            }
            if(temp3){
                sendOut(out,"yes",true,false,false);
                addArray(users,name);
                temp2=false;
            }
        }

        for(int i=0;i<getWriterSize(outG);i++){//sending to all clients a list of all the clients
            sendOut(outG.get(i),"",false,true,false);
        }
        while (true) {
            readIn(in,false);
            if(name.equals("bye")){
                for(int i=0;i<getWriterSize(outG);i++){
                    sendOut(outG.get(i),"",false,true,false);
                }
                removeArray(users,id);
                removeoutG(outG,id);
                break;
            }
            int index=search(id);
            sendOut(outG.get(index),"",false,false,true);
            message=null;
            name=null;
            id=null;
        }
        out.close();
        in.close();
        socket.close();
    }
    catch (IOException e) {
        e.printStackTrace();
    }
}


public int search(String name){
    for(int i=0;i<getArraySize(users);i++){
        if(getArray(users,i).equals(name)){
            return i;
        }
    }
    return -1;
}

public /*synchronized*/ void sendOut(PrintWriter out,String item,boolean init,boolean sendingNames,boolean sendingMessage){
    if(init){//not sure if i should syncronize??
        out.println(item);
        out.flush();
    }
    else if(sendingNames){
        out.println("~StOp");
        out.flush();
        out.println(users);
        out.flush();
    }
    else if(sendingMessage){
        out.println(message);
        out.flush();
        out.println(name);
        out.flush();
        out.flush();
    }
}
public /*synchronized*/ String readIn(BufferedReader in, boolean one){
    if(one){
        try {
            return in.readLine();
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }
    else{
        try {
            message= in.readLine();
            name= in.readLine();
            id= in.readLine();
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }
    return null;
}       
public /*synchronized*/ String getArray(ArrayList<String>users,int i){
    return users.get(i);
}
public /*synchronized*/ void removeArray(ArrayList<String>users,String name){
    users.remove(name);
}
public /*synchronized*/ void removeoutG(ArrayList<PrintWriter>users,String name){
    int index=search(name);
    outG.remove(index);
}

public /*synchronized*/ int getArraySize(ArrayList<String>users){
    return users.size();
}
public /*synchronized*/ int getWriterSize(ArrayList<PrintWriter>out){
    return out.size();
}
public /*synchronized*/ void addArray(ArrayList<String>users,String name){
    users.add(name);
}
public /*synchronized*/ void addReader(ArrayList<BufferedReader>inG,BufferedReader r){
    inG.add(r);
}
public /*synchronized*/ void addWriter(ArrayList<PrintWriter>outG,PrintWriter p){
    outG.add(p);
}
 }

Answer 1

(It's not a complete review, just a few random notes.)

1. ArrayList<...> reference types should be simply List<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

   private static List<String> users = new ArrayList<String>();
   

2. Collections.synchronizedList() does not modify the parameter. It returns a new lists which is synchronized, you should use its return value.

3. Instead of the boolean flag of the readIn method (which is hard to read, readers have to check the implementation to know what true and false means) I'd create two methods: a readName and (probably) a readMessage. Then it would be obvious what the called method does. See: Clean Code by Robert C. Martin, Flag Arguments, p41

4. Instead of modifying i (i = getArraySize(users)) to finish the loop you could break it:

   for (int i = 0; i < getArraySize(users); i++) {
       if (getArray(users, i).equals(name)) {
           sendOut(out, "no", true, false, false);
           temp3 = false;
           break;
       }
   }
   

5. Furthermore, you could replace the whole loop with the List.contains method. The Java ecosystem is really big - if you find that you're writing something that could be useful for someone else too (like getting the index of an element from a list) it certainly has been written. Search for a few minutes. If JDK does not have a method for that Google Guava or Apache Commons probably has.

6. Comments like these are hard to modify:

   for(int i=0;i<getArraySize(users);i++){//trying to make sure that one client doesnt 
       if(getArray(users,i).equals(name)){//enter a name that another already typed in
   

   Placing them before the line are easier to maintain:

   //trying to make sure that one client doesnt enter a name that another already typed in
   for(int i=0;i<getArraySize(users);i++){
       if(getArray(users,i).equals(name)){
   

   Anyway, comments often could be function names and the commented code could be extracted out to these functions. References:

   • Clean Code by Robert C. Martin, Bad Comments, p67: Don’t Use a Comment When You Can Use a Function or a Variable.
   • Refactoring by Martin Fowler, Chapter 3. Bad Smells in Code, Long Method

7. Variable name like temp2, temp3 are hard to understand. For example,temp3 could be renamed to userExists, if I'm right. It explains the purpose and helps readers a lot.