5
\$\begingroup\$

I built this simple application to facilitate communication between two or more machines on a local area network. As each client starts up, it broadcasts and receives broadcast packets; allowing for the omission of inputting a peer's IP address. It then creates a new connection to each discovered peer (even to itself), and characters are updated on-the-fly on each client's interface. I am posting this to get constructive feedback on the coding style, modularization, and its usability.

LANChat.java

package lanchat;

import java.awt.Font;
import java.awt.Frame;
import java.awt.TextArea;
import java.awt.event.KeyEvent;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.InetAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.ArrayList;

public class LANChat extends Frame implements Runnable {
    // text output of all connections
    private final TextArea textArea;
    // broadcast and receive of UDP; used for TCP connection(s) to peer(s)
    private final Broadcasts broadcasts;
    // list of all sockets for TCP output
    private final ArrayList<Socket> sockets;
    // storage for text data
    private StringBuilder lines;
    // continue running application?
    private boolean run = true;

    public LANChat() {
        // create field objects
        sockets = new ArrayList<>();
        lines = new StringBuilder();
        textArea = new TextArea(20, 80);
        // set focusable to false to ensure keys are captured by frame
        textArea.setFocusable(false);
        // monospace ftw
        textArea.setFont(Font.decode("monospaced"));
        // the only gui object is the text area
        add(textArea);
        pack();

        // start socket server to accept incoming connections
        new Thread(this).start();

        // instantiate and assign window listener and key listener to frame
        FrameListener frameListener = new FrameListener(this);
        addWindowListener(frameListener);
        addKeyListener(frameListener);

        // late initialize of UDP broadcast and receive, to ensure needed
        // objects are instantiated
        broadcasts = new Broadcasts(this);

        setVisible(true);
    }

    // global quit method shuts down everything and exits
    public void quit() {
        run = false;
        broadcasts.quit();
        System.exit(0);
    }

    // method called by key listener
    public void keyTyped(KeyEvent ke) {
        int i;
        synchronized (sockets) {
            // iterate through all sockets, and flush character through
            for (i = 0; i < sockets.size(); i++) {
                try {
                    Socket s = sockets.get(i);
                    PrintWriter pw = new PrintWriter(s.getOutputStream());
                    pw.print(String.valueOf(ke.getKeyChar()));
                    pw.flush();
                } catch (IOException ex) {
                    // remove socket, continue to any next if exception occurs
                    // (socket closed)
                    ex.printStackTrace();
                    sockets.remove(i);
                    continue;
                }
            }
        }
    }

    // method called by per-connection thread defined in socketStream
    public void putChar(int ch) {
        // check for backspace and space for delete,
        // otherwise put character into buffer,
        // and show updated buffer
        if (ch == 8 && lines.length() > 0)
            lines.delete(lines.length() - 1, lines.length());
        else
            lines.append((char)ch);
        synchronized (textArea) {
            textArea.setText(lines.toString() + '.');
        }
    }

    // method called by UDP listener
    // exits if connection fails
    void newAddress(InetAddress address) {
        synchronized (sockets) {
            // check if already connected to address, and exit if true
            for (Socket addr: sockets)
                if (addr.getInetAddress().getHostAddress()
                        .equals(address.getHostAddress()))
                    return;
            // create a new socket and add it to transmission pool
            Socket s;
            try {
                s = new Socket(address.getHostAddress(), Globals.TCPPORT);
            } catch (IOException ex) {
                return;
            }
            sockets.add(s);
        }
    }

    // called by socket server thread
    // defines a thread for each connection,
    // which calls putChar for every received character
    // exits thread if error occurs (socket closed)
    private void socketStream(final Socket s) {
        final InputStream is;
        try {
            is = s.getInputStream();
        } catch (IOException ex) {
            return;
        }
        final InputStreamReader isr = new InputStreamReader(is);
        final BufferedReader br = new BufferedReader(isr);
        new Thread(new Runnable() {
            public void run() {
                while (run && s.isConnected()) {
                    try {
                        if (br.ready())
                            putChar(br.read());
                    } catch (IOException ex) {
                        return;
                    }
                }
            }
        }).start();
    }

    // socket server accepts incoming connection,
    // and creates a thread to pass characters to the screen
    public void run() {
        try {
            ServerSocket ss = new ServerSocket(Globals.TCPPORT);
            while (ss.isBound() && run) {
                socketStream(ss.accept());
            }
            quit();
        } catch (IOException ex) {
            ex.printStackTrace();
            quit();
        }
    }

    // application entry
    public static void main(String[] args) {
        new LANChat();
    }
}

FrameListener.java

package lanchat;

import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;
import java.awt.event.WindowEvent;
import java.awt.event.WindowListener;

public class FrameListener implements WindowListener, KeyListener {
    private final LANChat parent;

    public FrameListener(LANChat parentArg) {
        parent = parentArg;
    }

    @Override
    public void windowOpened(WindowEvent we) {

    }

    @Override
    public void windowClosing(WindowEvent we) {
        parent.quit();
    }

    @Override
    public void windowClosed(WindowEvent we) {

    }

    @Override
    public void windowIconified(WindowEvent we) {

    }

    @Override
    public void windowDeiconified(WindowEvent we) {

    }

    @Override
    public void windowActivated(WindowEvent we) {

    }

    @Override
    public void windowDeactivated(WindowEvent we) {

    }

    @Override
    public void keyTyped(KeyEvent ke) {
        parent.keyTyped(ke);
    }

    @Override
    public void keyPressed(KeyEvent ke) {

    }

    @Override
    public void keyReleased(KeyEvent ke) {

    }

}

Broadcasts.java

package lanchat;

import java.io.IOException;
import java.net.DatagramPacket;
import java.net.DatagramSocket;
import java.net.SocketException;

public class Broadcasts {

    private final Runnable receiver;
    private final Runnable sender;
    private boolean run = true;

    public Broadcasts(LANChat parent) {
        receiver = new Runnable() {
            public void run() {
                byte data[] = new byte[0];
                DatagramSocket socket = null;
                try {
                    socket = new DatagramSocket(Globals.UDPPORT);
                } catch (SocketException ex) {
                    ex.printStackTrace();
                    parent.quit();
                }
                DatagramPacket packet = new DatagramPacket(data, data.length);
                while (run) {
                    try {
                        socket.receive(packet);
                    } catch (IOException ex) {
                        ex.printStackTrace();
                        parent.quit();
                    }
                    parent.newAddress(packet.getAddress());
                }
            }
        };
        sender = new Runnable() {
            public void run() {
                byte data[] = new byte[0];
                DatagramSocket socket = null;
                try {
                    socket = new DatagramSocket();
                } catch (SocketException ex) {
                    ex.printStackTrace();
                    parent.quit();
                }
                DatagramPacket packet = new DatagramPacket(
                        data, 
                        data.length, 
                        Globals.broadcastAddress, 
                        Globals.UDPPORT);
                while (run) {
                    try {
                        socket.send(packet);
                        Thread.sleep(Globals.UDPINTERVAL);
                    } catch (IOException ex) {
                        ex.printStackTrace();
                        parent.quit();
                    } catch (InterruptedException ex) {
                        ex.printStackTrace();
                        parent.quit();
                    }
                }
            }
        };
        new Thread(receiver).start();
        new Thread(sender).start();
    }

    public void quit() {
        run = false;
    }
}

Globals.java

package lanchat;

import java.net.InetAddress;
import java.net.InterfaceAddress;
import java.net.NetworkInterface;
import java.net.SocketException;
import java.util.ArrayList;
import java.util.Collections;

public class Globals {
    public static final int UDPPORT = 9090;
    public static final int TCPPORT = 9091;
    // delay in milliseconds between broadcasts
    public static final int UDPINTERVAL = 1000;
    public static final InetAddress broadcastAddress;
    static {
        // create broadcast address object refrencing the local machine's
        // broadcasting address for use with UDP
        broadcastAddress = getBroadcastAddress();
        assert (broadcastAddress != null);
    }
    private static InetAddress getBroadcastAddress() {
        ArrayList<NetworkInterface> interfaces = new ArrayList<>();
        try {
            interfaces.addAll(Collections.list(
                    NetworkInterface.getNetworkInterfaces()));
        } catch (SocketException ex) {
            ex.printStackTrace();
            return null;
        }
        for (NetworkInterface nic: interfaces) {
            try {
                if (!nic.isUp() || nic.isLoopback())
                    continue;
            } catch (SocketException ex) {
                continue;
            }
            for (InterfaceAddress ia: nic.getInterfaceAddresses()) {
                if (ia == null || ia.getBroadcast() == null)
                    continue;
                return ia.getBroadcast();
            }
        }
        return null;
    }
}
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

Separation of responsibilities

It would be better if LANChat was in charge of less stuff.

public class LANChat extends Frame implements Runnable {

Currently it's a GUI, and it also manages its peers (the sockets). It would be better to split these responsibilities to multiple classes.

Don't System.exit

It's not cool to System.exit. It would be better to reorganize the code in a way that when your main logic is finished, and you cleaned up all resources (close sockets, file handles), the program would naturally exit.

Bug: skipping sockets

There's a bug here:

        for (i = 0; i < sockets.size(); i++) {
            try {
                Socket s = sockets.get(i);
                PrintWriter pw = new PrintWriter(s.getOutputStream());
                pw.print(String.valueOf(ke.getKeyChar()));
                pw.flush();
            } catch (IOException ex) {
                // remove socket, continue to any next if exception occurs
                // (socket closed)
                ex.printStackTrace();
                sockets.remove(i);
                continue;
            }
        }

When you iterate over the indexes, for example 1, 2, 3, 4, 5, and at i=3 you remove a socket (sockets.remove(3)), but the loop counter still advances to i=4. The original item-4 is now item-3 since the original item-3 was removed. In other words, the socket after a removed socket will be skipped.

Here's one way to fix that:

        List<Socket> toRemove = new LinkedList<>();

        for (Socket s : sockets) {
            try {
                PrintWriter pw = new PrintWriter(s.getOutputStream());
                pw.print(String.valueOf(ke.getKeyChar()));
                pw.flush();
            } catch (IOException ex) {
                ex.printStackTrace();
                toRemove.add(s);
            }
        }

        sockets.removeAll(toRemove);

Avoid magic numbers

    // check for backspace and space for delete, [...]
    if (ch == 8 && lines.length() > 0)

Although the comment (sort of) explains that 8 is the backspace character, it would be better to use the more idiomatic '\b' instead. If '\b' is not obviously backspace to some readers, you could even define a BACKSPACE constant for it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.