HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Open a comm port and send an ASCII string

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
opencommasciiportandsendstring

Problem

This is my first ever project and first GUI. I have finished it (with help from Stack Overflow) and it works but is very slow. Can anyone suggest improvements for speed, best practices or usability.?

It currently takes 16 seconds from pressing run in Eclipse to something appearing on my screen. You will need RXTX lib to run it.

This is my GUI:

```
import gnu.io.CommPort;
import gnu.io.CommPortIdentifier;
import gnu.io.NoSuchPortException;
import gnu.io.SerialPort;

import java.util.HashMap;
import java.awt.EventQueue;
import java.awt.event.ItemEvent;
import java.awt.event.ItemListener;
import java.awt.event.MouseAdapter;
import java.io.IOException;
import java.io.OutputStream;
import java.io.PrintStream;
import java.util.ArrayList;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.List;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.border.EmptyBorder;
import javax.swing.JButton;
import java.awt.event.ActionListener;
import java.awt.event.ActionEvent;
import java.awt.GridBagLayout;
import java.awt.GridBagConstraints;
import java.awt.Insets;
import javax.swing.JTextArea;
import javax.swing.JLabel;
import javax.swing.JComboBox;
import javax.swing.SwingUtilities;
import java.awt.Window.Type;
import java.awt.Frame;
import java.awt.Rectangle;
import java.io.IOException;
import javax.swing.JTextField;
import javax.swing.JScrollPane;

public class EBIAlarmUI extends JFrame {

private static final long serialVersionUID = 16022014;

public TwoWaySerialComm twoWaySerCom;

/**
* Start the GUI
*/

public static void main(String[] args) {
EventQueue.invokeLater(new Runnable() {
public void run() {

try {
EBIAlarmUI frame = new EBIAlarmUI();
frame.setVisible(true);
} catch (Exception e) {
e.printStackTrace();
}
}
});

}

/**
* Create the frame.
*/
public EBIAlarmUI() {
initComponents();
twoWaySerCom = new Tw

Solution

Finding Performance Problems


It currently takes 16 seconds from pressing run in Eclipse to something appearing on my screen.

The first step to fixing this is to find out what is causing the delay. My first assumption would be that it is the hardware on the other end of the connection, but who knows. To figure this out, you can do two things:

  • Mock the TwoWaySerialComm object, so that there is no real communication with any external device. If your performance problems go away, you know it's not the GUI, but the external device, or the way you communicate with it*.



  • Profile your code and see which methods take the longest.



Before you don't know what the problem is, it will be hard to suggest improvements.

* the communication itself seems to be happening in a different thread, but not the initial connection. As you say you have performance problems on startup, this might be an issue.

Structure

It's really good that you put the connection stuff in it's own TwoWaySerialComm class. But right now, this class is hard to use, as you are printing in it, and swallowing exceptions. As you have a GUI, it would be nice to report errors in the GUI, not on the command line. So your TwoWaySerialComm class should throw exceptions instead of printing the errors.

Your fields should also be private and accessed with getters.

The biggest problem with your code structure is your GUI class though: It's way too big, and it has a method with 360 lines, which is just way too much. Nobody is going to read or understand that later on, and it makes your code very hard to extend, debug, profile, reuse, etc.

This gigantic method also contains a lot of duplication, for example the alarmXButtonActionPerformed ActionListener are pretty much the same. Create an actual class for them, and pass the button as argument. This would already help quite a bit.

If you then extract some of the GUI init code that logically belongs to each other to well-named functions, this would already be a great first step.

Misc

  • your naming could be better in parts. Eg it took me a bit to figure out what gbc_lblParity are. Don't shorten names, and use java naming conventions (camelCase). parityLabel and parityGridBagConstraints (or parityConstraints) aren't all that long, but a lot clearer

Context

StackExchange Code Review Q#85365, answer score: 5

Revisions (0)

No revisions yet.