Server side of a chat program

Question

This is just the server part of a chat program. The user of the server can receive and send messages to all the clients connected to the server. Everything works fine but since I'm a beginner I don't know if I should avoid something or not, what I should improve, etc..

The server class of the app:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.HashMap;
import java.util.Map;



public class ServerSide {
private static ServerSocket ss;
private static Socket sock;
private int port, ID;
private static String textReceived;
private static Map<Client, Integer> clientList = new HashMap<Client, Integer>();

public ServerSide() {

}
// Main Constructor takes the port to connect to a server
// in order to chat with the clients
public ServerSide(int port) throws IOException {
    this.port = port;
    ss = new ServerSocket(port);

    Thread t1 = new Thread(new Runnable() {

        @Override
        public void run() {

            while (true) {
                try {
                    sock = ss.accept();
                    //Every client has an unique ID that will be use to send      the receiving messages
                    //to all the clients except to the sender
                    ID++;
                    clientList.put(new Client(sock, ID), ID);
                } catch (IOException e) {
                    e.printStackTrace();
                }
            }
        }
    });
    t1.start();

}

public Map<Client, Integer> getClientsList() {
    return clientList;
}
//This method is used everytime the user of the server clicks on "Send".
protected void send(String textSent) throws IOException {
    new SendMessages(clientList, textSent);

}
//Return the text to append on the TextArea of the chat.
public String getTextReceived() {
    return textReceived;
}

public void setTextReceived(String textReceived) {
    this.textReceived = textReceived;
}
}

class SendMessages {
private Map<Client, Integer> list1 = new HashMap<Client, Integer>();
private String textSent;
private PrintWriter pw;
private int ID;
//This constructor is used only when the server receives messages from the clients
//it sends the received messages to all the clients of the list with the exception of the sender.
public SendMessages(Map<Client, Integer> list1, String textSent, int ID) throws IOException {
    this.list1 = list1;
    this.textSent = textSent;
    this.ID = ID;

    for (Map.Entry<Client, Integer> con : list1.entrySet()) {
        pw = new PrintWriter(con.getKey().getSock().getOutputStream());
        if (con.getValue() != ID) {
            pw.println(textSent);
            pw.flush();
        }
    }
}
//This constructor is used only when the user of the Server click on 'Submit'
//in order to send the messages.    
public SendMessages(Map<Client, Integer> list1, String textSent) throws IOException {
    this.list1 = list1;
    this.textSent = textSent;

    for (Map.Entry<Client, Integer> con : list1.entrySet()) {
        pw = new PrintWriter(con.getKey().getSock().getOutputStream());
        pw.println(textSent);
        pw.flush();
    }
 }
}

class Client extends Thread {
private Socket sock;
private InputStreamReader dis;
private BufferedReader br;
private String textSent, clientName;
private static String bi;
private PrintWriter pw;
private static Map<Client, Integer> clientList = new HashMap<Client, Integer>();
private int ID;

public Client(Socket socki, int ID) throws IOException {
    this.ID = ID;
    this.sock = socki;

    dis = new InputStreamReader(sock.getInputStream());
    br = new BufferedReader(dis);

    this.start();
}

public Socket getSock() {
    return sock;
}

//This method gets the received messages and passes them to the SendMessages constructor
//which it's used to send the messages to all the clients of the list.
protected void receiveMessages() throws IOException {

    ServerSide ms = new ServerSide();
    while ((bi = br.readLine()) != null && bi.length() > 0) {       
        new SendMessages(ms.getClientsList(), bi, ID);
        ms.setTextReceived(bi);
    }
}

public void run() {
    try {
        receiveMessages();
    } catch (IOException e) {
        e.printStackTrace();
    }

 }
}

This is the main class of the entire app:

 import java.io.IOException;
 import javax.swing.JFrame;

public class MainClassApp extends JFrame {

private static ServerSide servSide;
private static GraphicSide gi =  new GraphicSide();

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

    while (true) {
        if (gi.isAgree() == true) {
            servSide = new ServerSide(gi.getPort());
            break;
        }
    }

    while (true) {
        try {
            Thread.sleep(2000);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        //check if there are messages to append to the TextArea
        while (servSide.getTextReceived() != null) {
            gi.appendMessage(servSide.getTextReceived());
            //Reset the method that contains the received messages
            servSide.setTextReceived(null);
        }

    }

}
//This method is called every time the user of the server presses on submit
public void sendMessages(String textSent) throws IOException {
    textSent = gi.getId() + " : " + textSent;
    System.out.println(textSent);
    servSide.send(textSent);
 }
}

And finally this is the graphic part of the app:

import java.awt.Dimension;
import java.awt.GridBagConstraints;
import java.awt.GridBagLayout;
import java.awt.Insets;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.io.IOException;
import javax.swing.AbstractAction;
import javax.swing.BorderFactory;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JOptionPane;
import javax.swing.JPanel;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;
import javax.swing.JTextField;
import javax.swing.KeyStroke;
import javax.swing.SwingConstants;
import javax.swing.border.EmptyBorder;
import javax.swing.text.DefaultCaret;



public class GraphicSide extends JFrame {
private JPanel contentPane;
private static JTextField textPort;
private static JTextField textId;
private JButton submit;
private static JTextArea textArea;
private static int port;
private static boolean agree = false;
private static String testSent,id;
private static MainClassApp log ;

public GraphicSide() {
    setTitle("Login of the Server");
    setResizable(false);
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    setLocationRelativeTo(null);
    setBounds(100, 100, 250, 355);
    setVisible(true);
    contentPane = new JPanel();
    contentPane.setBorder(new EmptyBorder(5, 5, 5, 5));
    setContentPane(contentPane);
    contentPane.setLayout(null);



    JLabel portName = new JLabel("Port:");
    portName.setHorizontalAlignment(SwingConstants.CENTER);
    portName.setBounds(106, 102, 46, 14);
    contentPane.add(portName);

    textPort = new JTextField();
    textPort.setHorizontalAlignment(SwingConstants.CENTER);
    textPort.setToolTipText("e.g. 81");
    textPort.setBounds(77, 127, 103, 20);
    contentPane.add(textPort);
    textPort.setColumns(10);

    JLabel idLabel = new JLabel("Id:");
    idLabel.setHorizontalAlignment(SwingConstants.CENTER);
    idLabel.setBounds(106, 174, 46, 14);
    contentPane.add(idLabel);

    textId = new JTextField();
    textId.setHorizontalAlignment(SwingConstants.CENTER);
    textId.setToolTipText("e.g. Mike");
    textId.setBounds(77, 199, 103, 20);
    contentPane.add(textId);
    textId.setColumns(10);

    submit = new JButton("Submit");
    submit.addActionListener(new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent arg0) {
            if (textId.getText().length() == 0 || textPort.getText().length() == 0) {
                JOptionPane.showMessageDialog(null, "Compile all the fields", "Error", JOptionPane.WARNING_MESSAGE);
                return;
            }
            setVisible(false);
            try {
                id = textId.getText();
                new ChatWindow();
                agree = true;
            } catch (Exception e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }
    });

    submit.setBounds(94, 257, 74, 23);
    contentPane.add(submit);

}

private class ChatWindow extends JFrame {
    {
        this.setTitle("Server");
        this.setSize(750, 480);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        this.setLocationRelativeTo(null);
        this.setMinimumSize(new Dimension(750, 480));
        /**
         * 
         */
        GridBagLayout gridBagLayout_1 = new GridBagLayout();
        gridBagLayout_1.columnWidths = new int[] { 0, 640, 70 };
        gridBagLayout_1.rowHeights = new int[] { 387, 0, 0, 41, 40 };
        gridBagLayout_1.columnWeights = new double[] { 1.0, 1.0 };
        gridBagLayout_1.rowWeights = new double[] { 1.0, 0.0, 0.0, 1.0, Double.MIN_VALUE };
        this.getContentPane().setLayout(gridBagLayout_1);

        textArea = new JTextArea();
        JScrollPane scroll = new JScrollPane(textArea);
        DefaultCaret caret = new DefaultCaret();
        caret.setUpdatePolicy(DefaultCaret.ALWAYS_UPDATE);
        textArea.setEditable(true);
        textArea.setBorder(BorderFactory.createEtchedBorder());
        GridBagConstraints gbc_textArea = new GridBagConstraints();
        gbc_textArea.gridwidth = 3;
        gbc_textArea.insets = new Insets(2, 2, 5, 2);
        gbc_textArea.fill = GridBagConstraints.BOTH;
        gbc_textArea.gridx = 0;
        gbc_textArea.gridy = 0;
        this.getContentPane().add(scroll, gbc_textArea);

        JTextField textField = new JTextField();
        textField.setBorder(BorderFactory.createEtchedBorder());
        GridBagConstraints gbc_textArea_1 = new GridBagConstraints();
        gbc_textArea_1.gridheight = 2;
        gbc_textArea_1.gridwidth = 2;
        gbc_textArea_1.fill = GridBagConstraints.BOTH;
        gbc_textArea_1.insets = new Insets(2, 2, 2, 5);
        gbc_textArea_1.gridx = 0;
        gbc_textArea_1.gridy = 3;
        this.getContentPane().add(textField, gbc_textArea_1);

        this.setVisible(true);
        log = new MainClassApp();

        JButton btnNewButton = new JButton("Send");
        btnNewButton.addActionListener(new ActionListener() {

            @Override
            public void actionPerformed(ActionEvent arg0) {
                testSent = textField.getText();
                textArea.append(testSent + '\n');
                try {
                    log.sendMessages(testSent);
                } catch (IOException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }

                textField.setText("");
            }
        });

        textField.getInputMap().put(KeyStroke.getKeyStroke("ENTER"), "enter");
        AbstractAction pressedAction = new AbstractAction() {

            @Override
            public void actionPerformed(ActionEvent arg0) {


                btnNewButton.doClick();
                System.out.println("Key pessed");

            }

        };
        textField.getActionMap().put("enter", pressedAction);

        GridBagConstraints gbc_btnNewButton = new GridBagConstraints();
        gbc_btnNewButton.gridheight = 2;
        gbc_btnNewButton.gridx = 2;
        gbc_btnNewButton.gridy = 4;
        this.getContentPane().add(btnNewButton, gbc_btnNewButton);

    }
 }
public static void appendMessage(String text) {
    textArea.append(text + '\n');
 }

public static boolean isAgree() {
    return agree;
 }

public static int getPort() {
    return port = Integer.parseInt(textPort.getText());
 }
public  String getId() {
    return id;
 }

}

Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. — Vogel612
– Vogel612, Commented Nov 29, 2016 at 15:18

score 5 · Accepted Answer · 2016-11-29 20:51:57Z

Default Constructors:

public ServerSide() {

}

So, you want a default constructor, but you don't instantiate the class in it? Given your primary constructor (ServerSide(int port)), your default constructor should probably call this constructor with the default port. If you do not have a good default port, you probably shouldn't have this constructor. If your class needs a port number to work correctly, then you really need to remove this constructor.

`main()`

I don't like how you do all your work in main(). Move your work to dedicated, reusable methods that do a single thing. main()'s job is to start the program, not to do all the work.

Government Issue?

private static GraphicSide gi =  new GraphicSide();

Please use understandable naming. When I saw gi used in main(), I had to go back to find what it was really doing. Give it a name that specifies that it is the graphics side of the program right in the name. The same applies to various other names in the program.

Field Declarations

private int port, ID;

Just because you can declare multiple fields/variables with a single statement doesn't mean it is a good idea. In fact, it should never be used, in my non-so-humble opinion, unless the values are closely related, like the x and y coordinates of a chess piece on a board (and maybe not even then). I'm not sure if the port and the ID are closely related enough to be placed in the same declaration construct.

One Ring to Rule them All

static fields--one instance of these fields across each instance of the class. Every time you assign a new value to one of these fields, each class starts working with the new value. This is probably a bug you will need to fix.

OOP

In a comment you state "I add a default costructor only to get getClientsList() and set setTextReceived()." This is not how instances of classes in OOP work. If, and only if, these methods should have exactly the same result when called from any instance (or even a non-instance) of a class, then you can make them static. This way, you will not need an instance of the class to work with these. However, you need a chat connection to send messages, so I don't think this will work.

OOP does not usually work like that. In OOP, you create instances of classes that have nothing to do with each other. For example, if you have a Dog class, you can't just call Dog.Bark() without an instance--you can't make a non-existent dog bark. First, you create an instance, then you make the instance bark.

Dog dog = new Dog();
dog.bark();

In this program, you need to create an instance of a server-side chat instance that contains all its state. You cannot access elements of this class's state just by instantiating another instance--you need a reference to the instance. Once you understand how OOP works a little better, you will understand why you don't create default constructors like this, and why you should avoid static fields, methods, and classes.

Thread Safety

Your threading is so messed up I don't even know how messed up it is myself. Sorry if I seem harsh, but threading is very, very hard to get right, so please understand I'm not blaming you.

For example, you are adding clients in one thread here:

Thread t1 = new Thread(new Runnable() {

    @Override
    public void run() {

        while (true) {
            try {
                sock = ss.accept();
                //Every client has an unique ID that will be use to send      the receiving messages
                //to all the clients except to the sender
                ID++;
                clientList.put(new Client(sock, ID), ID);
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }
});
t1.start();

Then, you take this list from another thread when you send a message and pass it to a new SendMessages instance:

protected void send(String textSent) throws IOException {
    new SendMessages(clientList, textSent);

}

In SendMessages, you iterate that list:

public SendMessages(Map<Client, Integer> list1, String textSent, int ID) throws IOException {
    // ...

    for (Map.Entry<Client, Integer> con : list1.entrySet()) {
        // ...
    }
}

It is never, ever a good idea to access or modify data in two threads at once. See the famous Producer/Consumer and Dining Philosophers problems as a partial example of this. You need to add proper access control so only a single thread accesses this at a given time; look up semaphores and mutexes, for example.

Default Constructors: I add a default costructor only to get " getClientsList()" and set " setTextReceived". main(): how would you suggest to organize the classes? Government Issue? and Field Declarations : I edited the code,let me know if know it's better. One Ring to Rule them All: As i said above i edited the code,i put the static fields only where necessary. Thanks for the reply. — TomCa
– TomCa, Commented Nov 29, 2016 at 15:16
"GI" stands for "Government Issue". That's what I connected the name gi with--sorry for the confusion. I don't see how getClientsList() needs a default constructor for ServerSide()--you are not implementing OOP properly--I'll update my answer later, and cover some other serious issues you are making. — user34073
– user34073, Commented Nov 29, 2016 at 17:04
As for main(), you don't need any more classes--just add more methods to the MainClassApp class to handle the work. — user34073
– user34073, Commented Nov 29, 2016 at 17:04
Answer updated. I think you need to study OOP and multithreading a bit more, maybe in some simpler programs. Keep practicing and getting reviewed! — user34073
– user34073, Commented Nov 29, 2016 at 20:46
It is certainly something you should do. I learned C# about the same way, and you are just as good as I was after a year, maybe better. Just keep posting and learning, and we'll help you get better. — user34073
– user34073, Commented Nov 29, 2016 at 21:17

Stack Exchange Network

Server side of a chat program

1 Answer 1

Default Constructors:

`main()`

Government Issue?

Field Declarations

One Ring to Rule them All

OOP

Thread Safety

You must log in to answer this question.

Linked

Hot Network Questions

Server side of a chat program

1 Answer 1

Default Constructors:

main()

Government Issue?

Field Declarations

One Ring to Rule them All

OOP

Thread Safety

You must log in to answer this question.

Linked

Related

Hot Network Questions

`main()`