7
\$\begingroup\$

I have recently undertaken a project for programming practice based on a simple text editor. Normally this kind of thing wouldn't bother me, but ever since I completed a software architectures module in college, I question everything about how my code is and should be formatted! (probably a good thing?)

I currently only have one class and while on the grand scale of things, it is not huge. I think it is maybe already too big considering the size of the project I am doing. Basically, I have a window with a text area and menu bar with a "File" and "Edit" menus on it with action listeners to perform functions when clicked.

To me the class just seems like it is very cluttered and has quite a lot of if-else if statements in the actionPerformed method. I was just wondering what yer opinion on the class length might be and if it can/should be broken into several other classes?

package com.schongeproductions.texteditor;
import java.awt.BorderLayout;
import java.awt.Color;
import java.awt.Dimension;
import java.awt.Font;
import java.awt.Frame;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.print.PageFormat;
import java.awt.print.PrinterException;
import java.awt.print.PrinterJob;
import java.io.BufferedWriter;
import java.io.File;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import javax.swing.BorderFactory;
import javax.swing.JFileChooser;
import javax.swing.JFrame;
import javax.swing.JMenu;
import javax.swing.JMenuBar;
import javax.swing.JMenuItem;
import javax.swing.JOptionPane;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;
import javax.swing.border.Border;
import javax.swing.event.UndoableEditEvent;
import javax.swing.event.UndoableEditListener;
import javax.swing.undo.CannotUndoException;
import javax.swing.undo.UndoManager;
@SuppressWarnings("serial")
public class EditorGUI extends JFrame implements ActionListener {
 public static void main(String[] args) {
 new EditorGUI();
 }
 //============================================
 // FIELDS
 //============================================
 // Menus
 private JMenu fileMenu;
 private JMenu editMenu;
 private JMenuItem newFile, openFile, saveFile, saveAsFile, pageSetup, printFile, exit;
 private JMenuItem undoEdit, redoEdit, selectAll, copy, paste, cut;
 // Window
 private JFrame editorWindow;
 // Text Area
 private Border textBorder;
 private JScrollPane scroll;
 private JTextArea textArea;
 private Font textFont;
 // Window
 private JFrame window;
 // Printing
 private PrinterJob job;
 public PageFormat format;
 // Is File Saved/Opened
 private boolean opened = false;
 private boolean saved = false;
 // Record Open File for quick saving
 private File openedFile;
 // Undo manager for managing the storage of the undos
 // so that the can be redone if requested
 private UndoManager undo;
 //============================================
 // CONSTRUCTOR
 //============================================
 public EditorGUI() {
 super("JavaEdit");
 // Create Menus
 fileMenu();
 editMenu();
 // Create Text Area
 createTextArea();
 // Create Undo Manager for managing undo/redo commands
 undoMan();
 // Create Window
 createEditorWindow();
 }
 private JFrame createEditorWindow() {
 editorWindow = new JFrame("JavaEdit");
 editorWindow.setVisible(true);
 editorWindow.setExtendedState(Frame.MAXIMIZED_BOTH);
 editorWindow.setDefaultCloseOperation(EXIT_ON_CLOSE);
 // Create Menu Bar
 editorWindow.setJMenuBar(createMenuBar());
 editorWindow.add(scroll, BorderLayout.CENTER);
 editorWindow.pack();
 // Centers application on screen
 editorWindow.setLocationRelativeTo(null);
 return editorWindow;
 }
 private JTextArea createTextArea() {
 textBorder = BorderFactory.createBevelBorder(0, Color.RED, Color.RED);
 textArea = new JTextArea(30, 50);
 textArea.setEditable(true);
 textArea.setBorder(BorderFactory.createCompoundBorder(textBorder, BorderFactory.createEmptyBorder(2, 5, 0, 0)));
 textFont = new Font("Verdana", 0, 14);
 textArea.setFont(textFont);
 scroll = new JScrollPane(textArea, JScrollPane.VERTICAL_SCROLLBAR_ALWAYS, JScrollPane.HORIZONTAL_SCROLLBAR_ALWAYS);
 return textArea; 
 }
 private JMenuBar createMenuBar() {
 JMenuBar menuBar = new JMenuBar();
 setJMenuBar(menuBar);
 menuBar.add(fileMenu);
 menuBar.add(editMenu);
 return menuBar;
 }
 private UndoManager undoMan() {
 // Listener for undo and redo functions to document
 undo = new UndoManager();
 textArea.getDocument().addUndoableEditListener(new UndoableEditListener() {
 @Override
 public void undoableEditHappened(UndoableEditEvent e) {
 undo.addEdit(e.getEdit());
 }
 });
 return undo;
 }
 private void fileMenu() {
 // Create File Menu
 fileMenu = new JMenu("File");
 fileMenu.setPreferredSize(new Dimension(40, 20));
 // Add file menu items
 newFile = new JMenuItem("New");
 newFile.addActionListener(this);
 newFile.setPreferredSize(new Dimension(100, 20));
 newFile.setEnabled(true);
 openFile = new JMenuItem("Open...");
 openFile.addActionListener(this);
 openFile.setPreferredSize(new Dimension(100, 20));
 openFile.setEnabled(true);
 saveFile = new JMenuItem("Save");
 saveFile.addActionListener(this);
 saveFile.setPreferredSize(new Dimension(100, 20));
 saveFile.setEnabled(true);
 saveAsFile = new JMenuItem("Save As...");
 saveAsFile.addActionListener(this);
 saveAsFile.setPreferredSize(new Dimension(100, 20));
 saveAsFile.setEnabled(true);
 pageSetup = new JMenuItem("Page Setup...");
 pageSetup.addActionListener(this);
 pageSetup.setPreferredSize(new Dimension(100, 20));
 pageSetup.setEnabled(true);
 printFile = new JMenuItem("Print...");
 printFile.addActionListener(this);
 printFile.setPreferredSize(new Dimension(100, 20));
 printFile.setEnabled(true);
 exit = new JMenuItem("Exit");
 exit.addActionListener(this);
 exit.setPreferredSize(new Dimension(100, 20));
 exit.setEnabled(true);
 // Add items to menu
 fileMenu.add(newFile);
 fileMenu.add(openFile);
 fileMenu.add(saveFile);
 fileMenu.add(saveAsFile);
 fileMenu.add(pageSetup);
 fileMenu.add(printFile);
 fileMenu.add(exit);
 }
 private void editMenu() {
 editMenu = new JMenu("Edit");
 editMenu.setPreferredSize(new Dimension(40, 20));
 // Add file menu items
 undoEdit = new JMenuItem("Undo");
 undoEdit.addActionListener(this);
 undoEdit.setPreferredSize(new Dimension(100, 20));
 undoEdit.setEnabled(true);
 redoEdit = new JMenuItem("Redo");
 redoEdit.addActionListener(this);
 redoEdit.setPreferredSize(new Dimension(100, 20));
 redoEdit.setEnabled(true);
 selectAll = new JMenuItem("Select All");
 selectAll.addActionListener(this);
 selectAll.setPreferredSize(new Dimension(100, 20));
 selectAll.setEnabled(true);
 copy = new JMenuItem("Copy");
 copy.addActionListener(this);
 copy.setPreferredSize(new Dimension(100, 20));
 copy.setEnabled(true);
 paste = new JMenuItem("Paste");
 paste.addActionListener(this);
 paste.setPreferredSize(new Dimension(100, 20));
 paste.setEnabled(true);
 cut = new JMenuItem("Cut");
 cut.addActionListener(this);
 cut.setPreferredSize(new Dimension(100, 20));
 cut.setEnabled(true);
 // Add items to menu
 editMenu.add(undoEdit);
 editMenu.add(redoEdit);
 editMenu.add(selectAll);
 editMenu.add(copy);
 editMenu.add(paste);
 editMenu.add(cut);
 }
 // Method for saving files - Removes duplication of code
 private void saveFile(File filename) {
 try {
 BufferedWriter writer = new BufferedWriter(new FileWriter(filename));
 writer.write(textArea.getText());
 writer.close();
 saved = true;
 window.setTitle("JavaText - " + filename.getName());
 } catch (IOException err) {
 err.printStackTrace();
 }
 }
 // Method for quick saving files
 private void quickSave(File filename) {
 try {
 BufferedWriter writer = new BufferedWriter(new FileWriter(filename));
 writer.write(textArea.getText());
 writer.close();
 } catch (IOException err) {
 err.printStackTrace();
 }
 }
 // Method for opening files
 private void openingFiles(File filename) {
 try {
 openedFile = filename;
 FileReader reader = new FileReader(filename);
 textArea.read(reader, null);
 opened = true;
 window.setTitle("JavaEdit - " + filename.getName());
 } catch (IOException err) {
 err.printStackTrace();
 }
 }
 @Override
 public void actionPerformed(ActionEvent event) {
 if(event.getSource() == newFile) {
 new EditorGUI();
 } else if(event.getSource() == openFile) {
 JFileChooser open = new JFileChooser();
 open.showOpenDialog(null);
 File file = open.getSelectedFile(); 
 openingFiles(file);
 } else if(event.getSource() == saveFile) {
 JFileChooser save = new JFileChooser();
 File filename = save.getSelectedFile();
 if(opened == false && saved == false) {
 save.showSaveDialog(null);
 int confirmationResult;
 if(filename.exists()) {
 confirmationResult = JOptionPane.showConfirmDialog(saveFile, "Replace existing file?");
 if(confirmationResult == JOptionPane.YES_OPTION) {
 saveFile(filename); 
 }
 } else {
 saveFile(filename);
 }
 } else {
 quickSave(openedFile);
 }
 } else if(event.getSource() == saveAsFile) {
 JFileChooser saveAs = new JFileChooser();
 saveAs.showSaveDialog(null);
 File filename = saveAs.getSelectedFile();
 int confirmationResult;
 if(filename.exists()) {
 confirmationResult = JOptionPane.showConfirmDialog(saveAsFile, "Replace existing file?");
 if(confirmationResult == JOptionPane.YES_OPTION) {
 saveFile(filename); 
 }
 } else {
 saveFile(filename);
 }
 } else if(event.getSource() == pageSetup) {
 job = PrinterJob.getPrinterJob();
 format = job.pageDialog(job.defaultPage()); 
 } else if(event.getSource() == printFile) {
 job = PrinterJob.getPrinterJob();
 if(job.printDialog()) {
 try {
 job.print();
 } catch (PrinterException err) {
 err.printStackTrace();
 }
 }
 } else if(event.getSource() == exit) {
 System.exit(0);
 } else if(event.getSource() == undoEdit) {
 try {
 undo.undo();
 } catch(CannotUndoException cu) {
 cu.printStackTrace();
 }
 } else if(event.getSource() == redoEdit) {
 try {
 undo.redo();
 } catch(CannotUndoException cur) {
 cur.printStackTrace();
 }
 } else if(event.getSource() == selectAll) {
 textArea.selectAll();
 } else if(event.getSource() == copy) {
 textArea.copy();
 } else if(event.getSource() == paste) {
 textArea.paste();
 } else if(event.getSource() == cut) {
 textArea.cut();
 }
 }
 //============================================
 // GETTERS AND SETTERS
 //============================================
 public JTextArea getTextArea() {
 return textArea;
 }
 public void setTextArea(JTextArea text) {
 textArea = text;
 }
}
200_success
146k22 gold badges190 silver badges479 bronze badges
asked May 20, 2014 at 0:32
\$\endgroup\$
3
  • \$\begingroup\$ hey man, thanks for the code. May I use this code in my project? I'll assume the answer is yes until you explicitly say no. \$\endgroup\$ Commented Jan 22, 2015 at 17:16
  • \$\begingroup\$ I will post the improved/modified version once I'm done with it. \$\endgroup\$ Commented Jan 23, 2015 at 2:00
  • \$\begingroup\$ Sure...go nuts! \$\endgroup\$ Commented Feb 4, 2015 at 6:48

3 Answers 3

11
\$\begingroup\$
  1. You do a editorWindow.setDefaultCloseOperation(EXIT_ON_CLOSE); which means if I open multiple windows using New menu item and close any one of the windows program exits. This is because you do not differentiate between a window and an application instance. you should separate your window code from your application instance your application instance should manage its open windows. A window should inform the application instance when it is closed and the application should exit when its last window is closed. If you do not exit at the first window-closed event, you should release memory, file handles, (network, db connectionsin the future?) etc whatever resources associated with the window.

  2. You get the file name before you show the file chooser you get a NullPointerException, which you do not catch and swing prints it to console, provided you run the program from console.

    JFileChooser save = new JFileChooser();
    File filename = save.getSelectedFile();
    if(opened == false && saved == false) {
     save.showSaveDialog(null);
    
  3. In your file I/O operation you do this:

    // Method for opening files
    private void openingFiles(File filename) {
     try {
     openedFile = filename;
     FileReader reader = new FileReader(filename);
     textArea.read(reader, null);
     opened = true;
     window.setTitle("JavaEdit - " + filename.getName());
     } catch (IOException err) {
     err.printStackTrace();
     }
    }
    

    Apart from possible resource leaks, as previously pointed out, you give the user no indication whether his command completed successfully or not. Probably when you try to save a file you would expect the editor to give some indication of the result: if successful a "file saved" notification in the status bar, the star indicating the file is modified to disappear etc. If unsuccessful you would want even more obvious indication, such as a message box, saying "File is open by another process" or at least "Could not save buffer".

  4. Your event handling code seems very AWT, with a single if/else if block. A better way is to use Actions instead. Check out the swing Action tutorial; it also has a demo, although not perfect, shows how a single action can be associated with a menu item, toolbar button, a keyboard shortcut etc.

answered May 20, 2014 at 9:26
\$\endgroup\$
7
\$\begingroup\$

The only thing that sticks out to me is in your save methods your not calling finally, and that is where your close call should be. This way if the IOException happens before close gets called your possibly holding a file handler open somewhere.

// Method for saving files - Removes duplication of code
private void saveFile(File filename) {
 try {
 BufferedWriter writer = new BufferedWriter(new FileWriter(filename));
 writer.write(textArea.getText());
 saved = true;
 window.setTitle("JavaText - " + filename.getName());
 } catch (IOException err) {
 err.printStackTrace();
 }
 finally
 {
 writer.close();
 }
}
// Method for quick saving files
private void quickSave(File filename) {
 try {
 BufferedWriter writer = new BufferedWriter(new FileWriter(filename));
 writer.write(textArea.getText());
 } catch (IOException err) {
 err.printStackTrace();
 }
 finally
 {
 writer.close();
 }
}

See "The finally Block"

Do the same for your FileReaders as well.

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
answered May 20, 2014 at 3:52
\$\endgroup\$
0
\$\begingroup\$

Code like this should raise a flag:

 // Add file menu items
 undoEdit = new JMenuItem("Undo");
 undoEdit.addActionListener(this);
 undoEdit.setPreferredSize(new Dimension(100, 20));
 undoEdit.setEnabled(true);
 redoEdit = new JMenuItem("Redo");
 redoEdit.addActionListener(this);
 redoEdit.setPreferredSize(new Dimension(100, 20));
 redoEdit.setEnabled(true);

If you can squint your eyes and two paragraphs look the same, you are not DRY.

I usually look at these and try to figure out what is similar/different. In your case, with a single exception, the only difference is the menu name and the action that menu name takes. So that entire block can become

"Undo", "Redo", "Select All", "Copy", "Paste", "Cut".

for the most part. However, you must still "bind" the word "Undo" with some code.

So one way to do this might be with a Lambda:

addMenuItem("Undo", {->undo.undo()})
addMenuItem("Redo", {->undo.redo()})
...

the addMenuItem is a method you would define that adds the method name and lambda to a data structure for your actionPerformed method to use. It will probably need to create a map of <JMenuItem, Runnable> so that your actionPerformed can just:

eventMap.get(event.getSource()).call();

If you do it right, this can be your ENTIRE actionPerformed method (Well, you probably want a try/catch around this line to catch/report failures).

There are other complexities that this EXACT solution doesn't solve--nesting comes to mind (sub-menus) but those can be overcome by adding additional parameters to the addMenuItem function (such as "Parent" to specify a parent menu to place this menu under).

This kind of coding/refactor is important for a few reasons:

  1. copy/paste code tends to be very difficult to debug, the differences meld into the pattern.
  2. The supporting code is not difficult to write once you get in the habit and will probably save you more time than it takes.
  3. It's more fun to write code where you have to think than copy/paste/debug
  4. Once in this form it is often possible to both find higher level refactors and/or externalize the data (put it in a config file).

One trick here--don't FORCE every instance to use the new addMenuItem method. By just not converting exception cases (In your case, consider "Edit" an exception case) you can keep addMenuItem quite simple. The hardest thing about coding is exception cases, the less you have the simpler your code can be.

answered Aug 27, 2018 at 16:29
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.