3
\$\begingroup\$

Creating my first program in Swing and I was hoping to get some feedback. I'm looking for constructive criticism and tips for my swing code and my code in general. Don't really know what I'm doing so any advice would be helpful. The program is a simple text editor like notepad. Everything works the way I want it to so far but I'd like to know if I'm doing something wrong or if I could do something better.

import java.awt.BorderLayout;
import java.awt.Color;
import java.awt.Font;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.ComponentEvent;
import java.awt.event.ComponentListener;
import java.awt.event.MouseEvent;
import java.awt.event.MouseListener;
import javax.swing.*;
import javax.swing.text.SimpleAttributeSet;
import javax.swing.text.StyleConstants;

public class test2 extends JFrame implements ActionListener, ComponentListener, MouseListener {
    JMenuBar menuBar;
    JPanel textPanel;

    public test2() {
        //set up the Frame
        setTitle("Simple Text Editor");
        setLayout(null);
        setSize(1280, 720);
        setDefaultCloseOperation(EXIT_ON_CLOSE);
        UIManager.put("Menu.selectionBackground", Color.LIGHT_GRAY);

        //create the menu bar
        menuBar = new JMenuBar();
        menuBar.setBounds(0, 0, getWidth(), 25);
        menuBar.setBackground(Color.WHITE);
        menuBar.setFont(new Font("Arial", Font.PLAIN, 14));
        add(menuBar);

        //create the menus
        JMenu fileMenu = new JMenu(" File ");
        JMenu editMenu = new JMenu(" Edit ");
        JMenu formatMenu = new JMenu(" Format ");
        JMenu helpMenu = new JMenu(" Help ");
        fileMenu.setFont(new Font("Arial", Font.PLAIN, 14));
        editMenu.setFont(new Font("Arial", Font.PLAIN, 14));
        formatMenu.setFont(new Font("Arial", Font.PLAIN, 14));
        helpMenu.setFont(new Font("Arial", Font.PLAIN, 14));
        menuBar.add(fileMenu);
        menuBar.add(editMenu);
        menuBar.add(formatMenu);
        menuBar.add(helpMenu);
        fileMenu.addMouseListener(this);
        editMenu.addMouseListener(this);
        formatMenu.addMouseListener(this);
        helpMenu.addMouseListener(this);

        //create an editable text pane
        JTextPane textPane = new JTextPane();
        Font font = new Font("Helventica", Font.PLAIN, 14);
        textPane.setFont(font);
        SimpleAttributeSet attributeSet = new SimpleAttributeSet();
        StyleConstants.setAlignment(attributeSet, StyleConstants.ALIGN_CENTER);
        textPane.setCharacterAttributes(attributeSet, true);
        textPane.setEditable(true);
        String testString = "";
        textPane.setText(testString);
        JScrollPane scrollPane = new JScrollPane(textPane);
        scrollPane.setHorizontalScrollBarPolicy(JScrollPane.HORIZONTAL_SCROLLBAR_ALWAYS);
        scrollPane.setVerticalScrollBarPolicy(JScrollPane.VERTICAL_SCROLLBAR_ALWAYS);
        textPanel = new JPanel();
        textPanel.setLayout(new BorderLayout());
        textPanel.setBounds(0, 25, getWidth() - 16, getHeight() - 71);
        textPanel.add(scrollPane);
        add(textPanel);

        addComponentListener(this);

        setVisible(true);
    }

    public static void main(String[] args){
        test2 a = new test2();
    }

    @Override
    public void componentResized(ComponentEvent e) {
        menuBar.setBounds(0, 0, getWidth(), 25);
        textPanel.setBounds(0, 25, getWidth() - 16, getHeight() - 71);
        revalidate();
    }

    @Override
    public void mouseEntered(MouseEvent arg0) {
        arg0.getComponent().setFont(new Font("Arial", Font.BOLD, 14));
    }

    @Override
    public void mouseExited(MouseEvent arg0) {
        arg0.getComponent().setFont(new Font("Arial", Font.PLAIN, 14));
    }

    //unimplemented methods
    @Override
    public void actionPerformed(ActionEvent arg0) {}

    @Override
    public void componentHidden(ComponentEvent e) {}

    @Override
    public void componentMoved(ComponentEvent e) {}

    @Override
    public void componentShown(ComponentEvent e) {}

    @Override
    public void mouseClicked(MouseEvent arg0) {}

    @Override
    public void mousePressed(MouseEvent arg0) {}

    @Override
    public void mouseReleased(MouseEvent arg0) {}
}
\$\endgroup\$
0

2 Answers 2

6
\$\begingroup\$

Violating important threading practices

test2 a = new test2();

The swing documentation state all swing actions should be done from the event dispatch thread. If these rules are broken, weird bugs can occur with your application.

The documentation states:

Swing event handling code runs on a special thread known as the event dispatch thread. Most code that invokes Swing methods also runs on this thread. This is necessary because most Swing object methods are not "thread safe": invoking them from multiple threads risks thread interference or memory consistency errors. Some Swing component methods are labelled "thread safe" in the API specification; these can be safely invoked from any thread. All other Swing component methods must be invoked from the event dispatch thread. Programs that ignore this rule may function correctly most of the time, but are subject to unpredictable errors that are difficult to reproduce.

Wrap the call in a SwingUtilities.invokeLater call:

SwingUtilities.invokeLater(new Runnable(){
    @Override
    public void run() {
        test2 a = new test2();
    }
});

Creating to much new font objects

    menuBar.setFont(new Font("Arial", Font.PLAIN, 14));
    fileMenu.setFont(new Font("Arial", Font.PLAIN, 14));
    editMenu.setFont(new Font("Arial", Font.PLAIN, 14));
    formatMenu.setFont(new Font("Arial", Font.PLAIN, 14));
    helpMenu.setFont(new Font("Arial", Font.PLAIN, 14));
    Font font = new Font("Helventica", Font.PLAIN, 14);

If you ever wanted to change the font size or change the font type, you must change at least 6 different lines. While this amount doesn't look like much, all these lines are spread throughout the program making it a mess.

By placing these fonts as constants at the top of your class, you can get a cleaner code.

private static final Font MENU_FONT = new Font("Helventica", Font.PLAIN, 14);
private static final Font BODY_FONT = new Font("Arial", Font.PLAIN, 14);

Then do:

helpMenu.setFont(BODY_FONT); 

Delegate the setVisible call to the object creator

By delegating the call to the method that creates the gui, it allows the other class to modify things in the gui before its being displayed, this will enable a variety of useful features.

test2 a = new test2();
a.setVisible(true);
\$\endgroup\$
3
\$\begingroup\$

I'll just review a single line.

public class test2 extends JFrame implements ActionListener, ComponentListener, MouseListener {
  • Line Length:

The line is longer than the 80 Characters specified in the Oracle Conventions.

  • Capitalization:

Classes are to be capitalized in Upper CamelCase. That's Test2 then.

  • Composition:

This class is doing significantly too many things at once. Additionally it fails the principle: "Favor Composition over Inheritance".

Instead of this whole mess, you should only have

public class Test2 {

And implement the rest as Adapters (MouseAdapter, ComponentAdapter). For an ActionListener you could use a Lambda Expression.

In addition to that the JFrame should be an encapsulated private field in that class.

\$\endgroup\$

Your Answer

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

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