First Project: JFrame Class

Question

This is my second "first" attempt at starting Java. My first was cut short when I had to go back to a PHP project that required my immediate attention. But perhaps that was a good thing. I learned quite a bit more about OOP, better coding practices, and some other topics. So, after this hiatus, I noticed quite a bit wrong with my first attempt and ended up scraping it and starting from scratch.

So here is the first phase of my Java Project: The Second "First" Attempt. Sounds like a horrible sequel to a horrible movie. The project, once complete, will convert a text file to a CSV file. The format is custom, so it is unlikely that preexisting libraries will help me here, so I didn't even bother looking, except for guidelines on how to go about it. This first class is relatively simple and creates a window with some instructions on how to use the program and a button that allows the user to begin the process by loading a text file. Not very exciting yet, but that's why its only the first phase. Please take a look at it and let me know how you think this can be improved.

I have some specific questions and concerns, located below the code, however there are a number of them and they are quite lengthy. So if you just wish to review the code and skip the questions, that's fine too.

package view;

import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JLabel;
import javax.swing.JButton;

import java.awt.FlowLayout;

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

import javax.swing.JFileChooser;
import javax.swing.filechooser.FileFilter;
import javax.swing.filechooser.FileNameExtensionFilter;

public class JavaFileParser extends JFrame implements ActionListener {
/*------------------------------------------------------------------------------
    CONSTRUCT
------------------------------------------------------------------------------*/
    public static void main( String[] args ) {
        JavaFileParser parser = new JavaFileParser();
        parser.pack();
        parser.setVisible( true );
    }

    public JavaFileParser() {
        System.out.println( "Generating user interface..." );

        this.setTitle( "Title" );
        this.setLocationRelativeTo( null );
        this.setDefaultCloseOperation( EXIT_ON_CLOSE );

        JPanel panel = addPanel();

        JLabel label = addLabel( "Contents" );
        panel.add( label );

        JButton button = addButton( "Open File", "Open a file to begin..." );
        panel.add( button );
    }

/*------------------------------------------------------------------------------
    PUBLIC METHODS
------------------------------------------------------------------------------*/

    public JPanel addPanel() {
        System.out.println( "Generating frame panel..." );

        JPanel panel = new JPanel();
        this.getContentPane().add( panel );
        panel.setLayout( new FlowLayout() );

        return panel;
    }

    public JLabel addLabel( String text ) {
        System.out.println( "Generating label with the following contents: " + text );

        return new JLabel( text );
    }

    public JButton addButton( String title, String tooltip ) {
        System.out.println( "Generating button with the value of: " + title );
        JButton button = new JButton( title );

        System.out.println( "Generating tooltip for button \"" + title + "\" with a value of: " + tooltip );
        button.setToolTipText( tooltip );
        button.addActionListener( this );

        return button;
    }

    public void actionPerformed( ActionEvent e ) {
        System.out.println( "Generating File Chooser Dialog..." );

        JFileChooser filechooser = new JFileChooser();
        FileFilter filter = new FileNameExtensionFilter( "Text Files", "txt" );
        filechooser.addChoosableFileFilter( filter );
        filechooser.setFileFilter( filter );

        int returnValue = filechooser.showDialog( this, "Open Report" );
        if( returnValue == JFileChooser.APPROVE_OPTION ) {
            System.out.println( "class and method not yet implemented" );
        }
    }
}

Some Specific Questions and Concerns

1. You might have noticed the package name is "view". Having just spent a considerable amount of time with MVC, I figured I would see if I couldn't use that design pattern here. However, I don't know how well the PHP version I'm familiar with translates to Java. It feels like there is just too much application logic in here for a view, however, at the same time, this all feels necessary to render it. Is this the proper way to go about it? Is there a similar design pattern or one better suited for this application?

2. This list of imports seems rather long. It feels like I may have too many dependencies, but I am lost as to how I could separate them, or if its even necessary.

3. I've seen a number of places say that it is not a good idea to extend common libraries, such as JFrame. Their reasoning being that JFrame is simply too heavy and methods are too easy to override by accident. On that same token, I've seen some people express that it is ok to extend JFrame if your application IS a JFrame. I wrote this program before reading these posts so I decided to leave it and see what you all think. Does this also apply to implementing common libraries, such as what I'm doing with ActionListener?

4. Normally I would name files/classes based on their purpose. So in this instance, I would want to call this file/class "main" or "index" to indicate this is the first "page" the user will arrive on. However, as this is also first class of the program, it is named after the program. Should I separate this to a "main" or "index" file/class and import it in this current file/class?

   import view.index;
   
   public class JavaFileParser {
       public static void main( String[] args ) {
           index index = new index();
           index.pack();
           index.setVisible( true );
       }
   }
   

5. Because this class extends JFrame I can use those methods inherited from it without needing to reference an instance. I found this uncomfortable. Having come from PHP I was so used to doing $this->method() that I kept finding myself thinking and then typing $this before stopping myself. That was until I found a post where the author used this.method(). I adopted it and haven't had an issue since. What are the pro's and con's of this? Are there any, or is this just a style preference? Am I practicing a bad habit?

6. Is it considered bad practice to leave println()s in the code? I find it very convenient during debugging to see the progress of the program, but is it something I should remove from the program when it is done? Does it matter? I was actually thinking of eventually converting them to logging methods so that I could easily get log dumps if any errors occured, is this the traditional way this is done, or is there some better way?

7. I originally was not implementing the ActionListener class and was using a lamda function in the addActionListener() method called within the addButton() method. I didn't like how bulky it made my code, nor am I too terribly impressed with lamda functions in general. I can foresee reasons why I might need to implement lamda functions in Java in the future, say i need multiple action listeners, but is this implementation ok here?

Answer 1

Addressing Questions and Concerns

1. What you have so far is typical for a Java view. The ActionListener (or any listener for that matter) should be moved to the Controller.
2. Your imports ain't nothing. I'm not familiar with PHP, but in Java, every class is its own file (for the most part), so imports get bulky fast. Good job on not using wildcard imports (like javax.swing.*) that pollute the namespace.
3. When people say you should only extend JFrame when your class IS a JFrame, they mean you will be fiddling with stuff that only apply to a frame top-level container, such as modding the title bar. The additions you have could easily go into a JApplet. I would switch to subclassing JPanel. BTW, if you plan on doing custom drawing (not applicable here but maybe in the future), it's customary to subclass JComponent rather than JPanel. (see Subclass JComponent or JPanel) As for the ActionListener, I would put it in the Controller as I mentioned before. UI goes with the Controller, not the View.
4. I would separate the View from the main program. Naming varies for the class with the main method. Some people favor Main, some like suffixing with Program, it's really up to you. (see this discussion on possible names) However, I would avoid naming it index because it's lowercased, and because it's typically used for web-related stuff.
5. I tend to see people use less of the this keyword in Java. Sun's tutorial only mentions two uses: disambiguating a field and calling a constructor from another constructor. (see also When Should I Use "This" In a Class) Personally, I don't see any problem with using this to make instance variables stand out. But I've also seen people use prefixing (e.g. mButton) to distinguish them. The this keyword before methods is kinda strange.
6. For this one I'm not sure. This question kinda gets at what you're asking sorta. It's probably safer to remove them just in case. Sorry.
7. I think when you say lambda function you mean anonymous inner class. Java doesn't have lambdas. I've seen some dang long anonymous listeners in my time that made sense in context, but in your case, I'd go with that Controller class I keep mentioning.

Additional Notes

• Most people just prefix with J rather than Java, e.g. JFrame or JTetris.

• Everything related to swing should be in the event-dispatching thread. If you put your call to parser.setVisible(true) in a call to SwingUtilites.invokeLater(Runnable), it will execute your code in the event-dispatching thread. (see Threads and Swing) This is where either an anonymous inner class or a separate main class will come in handy. You can either do this

  public class JFileParserProgram implements Runnable {
  
       @Override
       public void run() {
           JFrame frame = new JFrame("Title");
           frame.setContentPane(new JFileParserView());
           frame.pack();
           frame.setLocationRelativeTo(null);
           frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
           frame.setVisible(true);
       }
  
       public static void main(String[] args) {
           SwingUtilities.invokeLater(new JFileParserProgram());
       }
  
  }
  

  or this

  public class JFileParserProgram {
  
      public static void main(String[] args) {
          SwingUtilities.invokeLater(new Runnable() {
  
              @Override
              public void run() {
                  JFrame frame = new JFrame("Title");
                  frame.setContentPane(new JFileParserView());
                  frame.pack();
                  frame.setLocationRelativeTo(null);
                  frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                  frame.setVisible(true);
              }
  
          }
      }
  
  }
  

  I personally use the first way because when I think of things to run, I think programs.

Answer 2

1) About the package name, it would probably be a good idea to adopt a more standard and conflict-free naming scheme, like the conventional tld.organization_name.packagename. This minimizes the possibility of namespace clash and harmonizes your packages with others.

You're quite right that it's a good idea to break up UI and logic into different modules. Here are some patterns to look into: presentation model, model-view-controller

You can do it like this:

mynamespace.JFileParserApplication (just contains main)

mynamespace.ui.ApplicationController (the go-between bridging UI and model)
mynamespace.ui.ApplicationFrame (extends JFrame)
mynamespace.ui.FileSaver (extends JFileChooser)
...and other UI stuff

mynamespace.model.FileParser (implements actual logic, UI-agnostic)
...any other stuff you may need for your model

2) Separating UI from model should significantly reduce coupling. To go further, it's good to make sure a class doesn't do much beyond what is logical for what it represents. For example, a child class of JFrame ought to delegate tasks like acting as a button listener and handling files to more fitting classes.

But there's no need to get too anxious. Peek at some of the Swing source files. The imports just go on and on and on.

3) Some of this is addressed in 2. I think it's not that JFrame shouldn't be extended, but rather an extension of JFrame ought to be essentially that. E.g., MyJFrame shouldn't be running the whole UI, and on the other hand, there's no need to extend JFrame just to give it a custom title.

4) Application, ApplicationStart, Starter, Launcher, Main, et al should all be fine. For extra clarity, put it either as the only class at the top level under your namespace or in a logical subpackage, e.g. .app or .main.

5) It's generally a matter of preference, but sometimes used for disambiguation. Java is a very verbose language, so it's natural that many just dispense with the often extraneous and syntactically redundant specifier that is frequently redundant, when the situation in context would make the meaning fairly unambiguous and clear without much of a possibility or chance of confusion.

koff

But it's not that bad a habit. You end up with syntactic noise, not unlike redundant parentheses, which may not be a bad thing if kept at a reasonable level.

6) I'm alright with using println's and leaving them in and commented out in small projects, but if you want a more principled approach, java.util.logging.Logger might be what you're looking for.

7) Java doesn't have lambda expressions yet. What you're describing sounds like anonymous inner classes, which are indeed bulky and inelegant. If they're of a sufficient size and complexity, consider turning them into inner classes or standalone classes.

I don't see any anonymous inner class in your code though. Did you mean implementing ActionListener in the frame class? I agree that's not the best way to do it, and you could either make an inner class or anonymous inner class for that, like so:

private MyActionListener myListener = new MyActionListener();
//...
private class MyActionListener implements ActionListener
{
    @Override
    public void actionPerformed(ActionEvent e)
    {
        //implement
    }
}

//anonymous inner class
ActionListener myListener =
    new ActionListener()
    {
        @Override
        public void actionPerformed(ActionEvent e)
        {
            //implement
        }
    };

Answer 3

+1 to @Eva and @ROBOKiTTY and some additional notes:

1. Comments like the following are just noise:

   /*------------------------------------------------------------------------------
       CONSTRUCT
   ------------------------------------------------------------------------------*/
   

   I'd remove them. (See Clean Code by Robert C. Martin, Chapter 4: Comments, Position Markers)

   It's not a performance issue. (Anyway, don't do premature optimizations.) It's only for readability and maintainability. If I have to maintain some old code I usually don't believe comments, they are too often outdated and useless. Furthermore, it's really easy to decide whether something is a constructor, a main method, or a public method so comments like this do not really help too much.

   If a class getting bigger and bigger and you need these comments to see the structure, um, it's a smell. Consider separating the responsibilities and creating smaller classes, methods. (Clean Code is really a great book about this topic.)

2. This:

   parser.setVisible( true );
   

   should be

   parser.setVisible(true);
   

   The Code Conventions for the Java Programming Language does not say explicitly (or at least I couldn't find it) that you should not put space around the parameters but I as far as I see it's the standard among the Java developers to omit these spaces and the examples in the linked documentation also omit them.

3. Calling overridable methods (like addPanel) from constructors is not a good practice. Child classes could see partially initialized objects in the overridden method. The addPanel should be final here. See What's wrong with overridable method calls in constructors? and Effective Java 2nd Edition, Item 17: Design and document for inheritance, or else prohibit it

4. Methods like addLabel and addButton actually do not add the created label and button to anything. I'd call the former createLabel and createRegisteredButton the latter (since it sets the listener of the button too).

5. It's usually good practice to put @Override annotation to overridden methods, so the actionPerformed method should have one. See: When do you use Java's @Override annotation and why?

6. Using this. when it's not really necessary also just noise. Modern IDEs use highlighting to separate local variables from instance variables.

7. I'm not too familiar with Swing, so the setLocationRelativeTo(null) looks really weird to me. What does the null parameter means here? It looks like a magic number even it's in the javadoc. I'd create a setLocationToCenter() method for that:

   private void setLocationToCenter() {
       setLocationRelativeTo(null);
   }
   

   It would document the intention of the developer and easier to read and maintain. The program does the same but the maintainer don't have to know every detail of the API nor check the documentation to understand the code.

8. The title could be a little bit descriptive than the following:

   setTitle( "Title" );
   

9. Instead of using System.out or java.util.logging consider a better one. I strongly suggest you SLF4J + Logback. It's worth to learn it, it's useful in small projects as well as in bigger ones.