Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The Word Processor I had in mind would be similar to Microsoft's word and OpenOffice. I am trying to keep my logic separated from the User Interface and that separated from the controller, basically using the MVC (model view controller). Other things I would like reviewed would be code layout, should I try to abstract the code more? Am I using the correct writing surface (JTextArea) so that I can later implement a font style, size change on run time? Also should I be doing something about thread safety (I understand that JFrame's are not thread safe, I am going to be honest I don't fully understand what this really means, but I am sure it has to do with a single thread running for the user input, graphics and business logic).

This is the controller :

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.io.File;



public class ProcessEvents {

    private WordFrame frame = new WordFrame();
    private DataStuff data = new DataStuff();
    private DialogBoxes dialogs = new DialogBoxes();
    private boolean fileSaved;
    String fileName = "";
    int fontSize = 0;
    public ProcessEvents(WordFrame frame, DataStuff data){
        this.frame = frame;
        this.data = data;
        this.frame.addListener(new wordProcessListener());
    }
    class wordProcessListener implements ActionListener{

        @SuppressWarnings("static-access")
        @Override
        public void actionPerformed(ActionEvent e) {
            if(e.getSource().equals(frame.openMenuItem)){
                frame.fileChooser.showOpenDialog(frame);
                File f = frame.fileChooser.getSelectedFile();

                System.out.println("Command Executed: open");

                data.loadFile(f.getAbsoluteFile());

                if(data.showText() != null){                    
                    System.out.println(data.showText());                
                    frame.textArea.append(data.showText().toString());
                }
            }

            if(e.getSource().equals(frame.FontMenuItem)){
                System.out.println("font");
            }

            if(e.getSource().equals(frame.exitMenuItem)){
                dialogs.getConfirmDialog("exitWithoutSave");
            }

            if(e.getSource().equals(frame.saveMenuItem)){
                frame.fileChooser.showSaveDialog(null);
                File f = frame.fileChooser.getSelectedFile();
                String text = frame.textArea.getText();
                data.saveFile(f.getAbsolutePath()+".txt", text);
                System.out.println(f.getName());
                fileSaved = true;           
            }           
        }       
    }
}

This is the model :

import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;

public class DataStuff {

    private File file;

    String text;
    String name;
    private File saveFile;
    int counter = 0;
    FileInputStream fis = null;
    FileOutputStream fout = null;
    StringBuilder sb = new StringBuilder(4096);

    int count = 0;
    public void loadFile(File fileName){
        this.file = fileName;
        try{
            fis = new FileInputStream(file);

            while ((counter = fis.read()) != -1) {

                System.out.print((char) counter);

                sb.append((char) counter);

            }

        }
        catch(IOException ex){
            System.out.println("file couldn't be opened, or was incorrect or you clicked cancel");
        }
        finally {
            try {
                if (fis != null)
                    fis.close();
            } catch (IOException ex) {
                ex.printStackTrace();
            }
        }
    }
    public StringBuilder showText(){

        return sb;

    }
    public void saveFile(String name, String text) {
        this.name = name;

        try{                
                fout = new FileOutputStream(name);  
                fout.write(text.getBytes());
                System.out.println("file saving worked");           
        }
        catch(IOException e){
            System.out.println("File failed to save or something went horribly wrong");
        }       
    }   
}

This is the view :

import java.awt.Font;
import java.awt.event.ActionListener;


import javax.swing.ImageIcon;
import javax.swing.JFileChooser;
import javax.swing.JFrame;
import javax.swing.JMenu;
import javax.swing.JMenuBar;
import javax.swing.JMenuItem;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;


public class WordFrame extends JFrame{
    /**
     * 
     */
    private static final long serialVersionUID = 1L;
    private JMenuBar menubar;
    private JMenu fileMenu, editMenu, viewMenu;
    JMenuItem saveMenuItem, openMenuItem, newMenuItem, exitMenuItem, FontMenuItem;
    JTextArea textArea = new JTextArea(1000,900);
    private int width = 1280, height = 980;
    private JScrollPane scrollbar = new JScrollPane(textArea);
    JFileChooser fileChooser = new JFileChooser();
    private int textHeight = 12;
    private Font yeah = new Font(Font.SANS_SERIF, 2, textHeight);

    public WordFrame(){
        setUI();
        addMenuBar();   
        textArea.setFont(yeah);
    }

    public void setUI(){
        this.setTitle("Word Processor");
        this.setIconImage(new ImageIcon(this.getClass().getResource("Bridge.jpg")).getImage());
        this.setSize(width, height);
        this.setLocation(0,0);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        this.add(scrollbar);

    }

    public void addMenuBar(){
        menubar = new JMenuBar();
        fileMenu = new JMenu(" File ");
        editMenu = new JMenu("Edit ");
        viewMenu = new JMenu("View ");

        newMenuItem = new JMenuItem("New");
        fileMenu.add(newMenuItem);
        fileMenu.addSeparator();
        fileMenu.setMnemonic('f');

        openMenuItem = new JMenuItem("Open");

        fileMenu.add(openMenuItem);

        saveMenuItem = new JMenuItem("Save");
        fileMenu.add(saveMenuItem);

        fileMenu.addSeparator();
        exitMenuItem = new JMenuItem("Exit");
        fileMenu.add(exitMenuItem);

        FontMenuItem = new JMenuItem("Font");
        editMenu.add(FontMenuItem);
        menubar.add(fileMenu);
        menubar.add(editMenu);
        menubar.add(viewMenu);      

        this.setJMenuBar(menubar);
    }

    public void setFontSize(int i){
        this.textHeight = i;
    }
    public void addListener(ActionListener listener){
        openMenuItem.addActionListener(listener);
        exitMenuItem.addActionListener(listener);
        saveMenuItem.addActionListener(listener);
    }
}

and finally this is the Main class

public class Main {
    /*
     * @Version: 0.002
     * not much in terms of commenting but a lot of this stuff is very obvious
     */
    public Main(){
        WordFrame mainFrame = new WordFrame();
        DataStuff data = new DataStuff();
        @SuppressWarnings("unused")
        ProcessEvents process = new ProcessEvents(mainFrame, data );
        mainFrame.setVisible(true);
    }

    public static void main(String args[]){
        @SuppressWarnings("unused")
        Main m = new Main();
    }
}
share|improve this question
2  
You shouldn't extends JFrame: stackoverflow.com/questions/1143923/… –  Landei Feb 24 at 12:13
    
Thanks Landei, i have adjusted my code to remove that, WordFrame no longer extends JFrame but instantiates it. –  tommy knocker Feb 24 at 15:45
add comment

4 Answers

up vote 7 down vote accepted

At face value the code looks OK. I want to focus on one area, though: File Management

You use FileInputStream and FileOutputStream to read and write your files.

Input/Output streams are designed for byte data. You do not have bytes, you have characters.

But, you are trying to shoe-horn these characters in to single byte values, and this will cause all sorts of problems for non-ascii characters.... This code here:

        fis = new FileInputStream(file);
        while ((counter = fis.read()) != -1) {
            System.out.print((char) counter);
            sb.append((char) counter);
        }

is the problem. fis.read() reads just a single byte, and then you shoe-horn that in to a char... which it may not be.

ALSO ... doing it that way is probably the slowest possible way to read a file ... ;-)

Readers/Writers are designed for characters, and they do all the smarts of character encoding for you.

By default, Readers/Writers will use the platform encoding for your files. I recommend that you force them to use the UTF-8 encoding so that you have consistency from one platform to the next.

Additionally, you can save a fair amount of error handling if you use the Java7 try-with-resources structures.

The write code will be something like:

public void saveFile(String name, String text) {
    this.name = name;

    try (Writer writer = new BufferedWriter(new OutputStreamWriter(
            new FileOutputStream(name), StandardCharsets.UTF_8))) {

        writer.write(text);              
        writer.flush();
        System.out.println("file saving worked");           
    } catch(IOException e){
        // do at least a little more than just print a useless message
        e.printStackTrace();
        System.out.println("File failed to save or something went horribly wrong");
    }       
}   

and the read code will look something like:

public void loadFile(File fileName){
    //        this.file = fileName;
    try (BufferedReader reader = new BufferedReader(new InputStreamReader(
            new FileInputStream(fileName), StandardCharsets.UTF_8))) {

        char[] buffer = new char[8192]; // decent size buffer.
        sb.setLength(0);
        int len = 0;
        while ((len = reader.read(buffer)) >= 0) {
            sb.append(buffer, 0, len);
        }

    } catch(IOException ex){
        // do at least a little more than just print a useless message
        ex.printStackTrace();
        System.out.println("file couldn't be opened, or was incorrect or you clicked cancel");
    }
}
share|improve this answer
    
Thanks rolfl i will be adjusting my code to reflect your recommendations. I am still reading your answer over just so i can compare the difference and understand the effects it will have, performance etc. –  tommy knocker Feb 24 at 15:47
add comment

Some general note about your program.

Comments

   /**
     * 
     */
    private static final long serialVersionUID = 1L;

Never leave an empty comment. Comment are already something I'm not a fan of, but an empty one is useless.

    /*
     * @Version: 0.002
     * not much in terms of commenting but a lot of this stuff is very obvious
     */

You don't have to say that there isn't much to say. When I read a comment, I want to have useful information about the code, not that I won't find anything of value. I would suggest that if you have nothing to say then just say nothing.

Naming convention

JMenuItem saveMenuItem, openMenuItem, newMenuItem, exitMenuItem, FontMenuItem;

One of your variable don't respect the naming convention. Pay close attention to that, when you don't respect convention your code is less readable.

private JMenuBar menubar;

Camel-case is not respected here.

private Font yeah = new Font(Font.SANS_SERIF, 2, textHeight);

Is yeah the name of the font ? If not, than I don't find yeah a good name. Something like defaultFont or the name of the font you'll use would be better.

Constant

private int width = 1280, height = 980;

I didn't find a place in the code where you change the width and height. Those should probably be a constant with proper definition and naming.

Suppress warnings

public static void main(String args[]){
    @SuppressWarnings("unused")
    Main m = new Main();
}

Suppress warnings is something that I take particular attention. Why have you suppress the warning ? Warning should ring a bell and raise the question : Am I doing something wrong ? In your case, the warning is probably unused variable. But is it really an unused variable ? No, the action to start the application is in the constructor. Is it a good design, I'm wondering myself. I don't find it clear when an application start in a constructor. IMHO, constructor should initialize a class, not really much. If I were you I would probably have a method to actually start the application.

But in general warning should pop the question, am I doing something wrong? A suppress warning should be the last option to that though process. In many case, it's acceptable to have a @SupressWarning but sometimes it's just an easy solution that could lead to maintenance problem later on. (Bad design decision etc..)

share|improve this answer
1  
Hi Marc, i have taken on board your comments and have amended my code to reflect proper naming conventions as well as changed private int width = 1280, height = 980; to static final int WIDTH = 1280, HEIGHT = 980; –  tommy knocker Feb 24 at 15:38
add comment

I see you have a lot of different kinds of data defined as members of a single class:

private JMenuBar menubar;
private JMenu fileMenu, editMenu, viewMenu;
JMenuItem saveMenuItem, openMenuItem, newMenuItem, exitMenuItem, FontMenuItem;
JTextArea textArea = new JTextArea(1000,900);
private int width = 1280, height = 980;
private JScrollPane scrollbar = new JScrollPane(textArea);
JFileChooser fileChooser = new JFileChooser();
private int textHeight = 12;
private Font yeah = new Font(Font.SANS_SERIF, 2, textHeight);

I think I am asking a lot as is but I am predicting my code will bloat a lot more than it is.

I suggest that you define more classes, which don't depend on being run as members of a JFrame.

For example, you might have a Document class which encapsulates the contents (data) of the document. Methods of the Document class could let you edit the document, for example:

  • Inject key-strokes (alpha-numeric keys, cursor keys)
  • Select regions of (a.k.a. ranges in) the document (a letter, a word, a paragraph)
  • Change properties of the selected range (font, bold, etc.)

A reason for making this Document independent of (decoupled from) JFrame is so that you can use it in two places:

  • Inside JFrame (driven by an interactive user)
  • Outside JFrame (driven by an automated test suite)

For example you might want an automated/regression test which:

  • Creates a new document
  • Inserts "aa"
  • Positions the cursor between the two letters
  • Inserts carriage return to break it into two paragraphs
  • Asserts that the resulting document contains two paragraphs, each consisting of "a"

Apart from the Document class, you might want another class which encapsulates the toolbar settings. For example when you insert "aa" into a document, the document might want to know which font you're using. One way to do that could be to pass Font information as a parameter to the Document.insertText method ... but that might mean adding a lot of extra parameters to a lot of extra functions ... a simpler way might be to pass an EditorSettings class (which contains all the properties like font, margins, grammar/dictionary, etc.) to the Document constructor, so that instead of needing to tell the Document (by pushing a parameter) what the settings are each time, the Document can discover the settings whenever it needs to (by reading the properties of the EditorSettings class).

Apart from Document's using EditorSettings, your JMenuBar would be the other object that accesses your EditorSettings: to display existing settings and to let the user change existing settings.

Another independent class is the rendered view of the document. For example, if the document is HTML, then the "rendered view" must calculate where to place each word (measuring the size of words in a given font, splitting paragraphs into multiple lines, etc.). The rendered view might take, as input, the document and the window size.

In summary, perhaps your WordFrame should include the following data members:

  • A Document instance
  • A EditorSettings instance
  • A RenderedView instance
  • Various Swing components, e.g. toolbar etc.
share|improve this answer
    
Hi Chris, that is an interesting answer. Interesting in the fact that it's going to take me some time to dissect my code and to understand how a class like Document should be defined. Would Document itself be a subclass of JTextArea since it would be adding functionality or would i be extracting the data from the JTextArea to the class Document. At the moment the Document class (to me) would look like it would need to be available to the Controller class and that WordFrame class would need to be available to the Document class –  tommy knocker Feb 24 at 15:43
1  
I think that Document is the "Model" in an MVC pattern. It knows nothing about the UI. It contains the document's text and styling information. It supports methods like "insertWord", "deleteRange", etc. RenderedView also know nothing about Swing. Its contents are a list of words to be rendered, with the font and (X,Y) coordinates associated with each word. Your program: a) paints word from RenderedView onto its Swing control b) calls methods of Document when the user types-in text. When Documents content changes it notifies RenderedView, which recalculates itself and then repaints. –  ChrisW Feb 25 at 7:13
add comment

I get the feeling that you are awkwardly confusing nouns and verbs, and, judging by the warnings, the compiler agrees with me.


Let's start with ProcessEvents. Classes should be named as nouns; ProcessEvents is a verb phrase. EventProcessor would be more appropriate. WordProcessorActionListener would be more Javalicious.

Another symptom of the confusion is that the wordProcessorListener inner class is actually the noun I was expecting (why the lowerCaseNaming for a class, by the way?), and the ProcessEvents constructor just attaches it to the frame. The outline should be:

public class WordProcessorActionListener implements ActionListener {
    private WordFrame frame;
    private DataStuff data;

    public WordProcessorEventListener(WordFrame frame, DataStuff data) {
        this.frame = frame;
        this.data = data;
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        …
    }
}

The event handler, which acts as a controller, has no business keeping fileName, fontSize, or any of the other state — all of that belongs in the model.


Main could use an analogous cleanup. In accordance with the advice that classes should be named as nouns, I'd also rename it to WordProcessorLauncher.

public class WordProcessorLauncher {
    private WordFrame mainFrame;

    public WordProcessorLauncher() {
        this.mainFrame = new WordFrame();
        DataStuff data = new DataStuff();
        WordProcessorActionListener listener = new WordProcessorActionListener(this.mainFrame, data);
        this.mainFrame.addListener(listener);
    }

    public void launch() {
        this.mainFrame.setVisible(true);
    }

    public static void main(String[] args) {
        (new WordProcessorLauncher()).launch();
    }
}

Keep your classes as nouns and methods as verbs, and voilà — there's no more need for @SuppressWarning("unused").

share|improve this answer
    
thanks for that, i was not aware until you used it, that you could create an anonymous instance of a class. Just read up on it on the Oracle java tutorial site –  tommy knocker Feb 25 at 15:12
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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