1
\$\begingroup\$

So I learned from YouTube videos and online tutorials and they gave me a task to do a simple program. I need to pick 1 button from 4 buttons (radio buttons) and each time I pick a button he's changing a text in a text box.

The program is working great ! I'm just asking you guys to check if I did something that is considered wrong in programming or if there's a better way to do this task.

So that's my main program.

import javax.swing.JFrame;

public class Main {

    public static void main(String args[])
    {
        gui go = new gui();
        go.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        go.setSize(317,220);
        go.setVisible(true);
    }

}

and that's the class i'm using (gui class)

package learning;

import java.awt.FlowLayout;
import java.awt.Font;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.ButtonGroup;
import javax.swing.JFrame;
import javax.swing.JRadioButton;
import javax.swing.JTextField;

public class gui extends JFrame implements ActionListener  {

    private JTextField tf = new JTextField( 25);
    private JRadioButton regularRadio = new JRadioButton("Regular",true);
    private JRadioButton boldRadio = new JRadioButton("Bold",false);
    private JRadioButton italicRadio = new JRadioButton("Italic",false);
    private JRadioButton boldNitalicRadio = new JRadioButton("Bold And Italic",false);;
    private ButtonGroup group = new ButtonGroup();



    public  gui(){

        super("Changing text program");
        setLayout (new FlowLayout());

        add(tf);
        add (regularRadio);
        add (boldRadio );
        add (italicRadio);
        add (boldNitalicRadio);

        group.add(boldRadio);
        group.add(boldNitalicRadio);
        group.add(italicRadio);
        group.add(regularRadio);

        regularRadio.addActionListener(this);
        boldRadio.addActionListener(this);
        italicRadio.addActionListener(this);
        boldNitalicRadio.addActionListener(this);

    }

    public void actionPerformed(ActionEvent e){

        if (e.getActionCommand() == "Regular"){
       tf.setText("You picked regular radio button");
       tf.setFont(new Font ("David" , Font.PLAIN , 14));
        }
        else if (e.getActionCommand() == "Bold") {
            tf.setText("You picked bold radio button");
               tf.setFont(new Font ("David" , Font.BOLD , 14));
        }
        else if (e.getActionCommand() == "Italic") {
            tf.setText("You picked italic radio button");
               tf.setFont(new Font ("David" , Font.ITALIC , 14));
        }
        else{
            tf.setText("You picked Bold and Italic radio button");
               tf.setFont(new Font ("David" , Font.ITALIC + Font.BOLD , 14));
        }

    }
}
\$\endgroup\$
1
\$\begingroup\$

Java naming convention

You should read the Java naming convention. Class require PascaleCase, so public class gui should be public class Gui.

Indentation and style

You should use an IDE or a tools to automatically correct any styling or indentation error. The spacing of your variables declarations are not correct and your indentation of your if-else are not correct either.

There are plenty of good free to use IDE and tools. This goes in pair with the Java naming convention/ Java Style recommendation.

Use better names

You're not limited in size for your variable names. Go for clarity over brievty! This will go a long way to help you, at first glance, know what the variable is about. For the next person that read your code it will be easier too.

private JTextField tf = new JTextField( 25);
private JRadioButton regularRadio = new JRadioButton("Regular",true);
private JRadioButton boldRadio = new JRadioButton("Bold",false);
private JRadioButton italicRadio = new JRadioButton("Italic",false);
private JRadioButton boldNitalicRadio = new JRadioButton("Bold And Italic",false);;
private ButtonGroup group = new ButtonGroup();

Should probably be :

private JTextField textField = new JTextField( 25);
private JRadioButton regularRadio = new JRadioButton("Regular",true);
private JRadioButton boldRadio = new JRadioButton("Bold",false);
private JRadioButton italicRadio = new JRadioButton("Italic",false);
private JRadioButton boldAndItalicRadio = new JRadioButton("Bold And Italic",false);;
private ButtonGroup group = new ButtonGroup();

init

In most GUI code I've seen, you can create an init method and put all the initialization code in there. It helps to keep the constructor clean, but it's no big deals. It's what I'm mostly used too, but it's been a long time I've done any Swing related code.

Constant

If you see a value used over and over, ask yourself : Does this value represent the same thing?

Take "David" for example, it does represent the same named font in all your case. You could then have a private static String DAVID = "David" to represent the font.

Action command

This is a special case from your previous point. Since you hard-coded the action command names, if you change it at one place you have to remember to change it in other places.

This is a perfect example of the Java Enum class. Take a look at https://docs.oracle.com/javase/tutorial/java/javaOO/enum.html, it's a good tutorial on how to use it.

Using an enum you make it clearer and should be easier to use in the long run.

Other point

I don't think you need to create the Font object every time, you may use an const Font object to reduce object creation. It's a small thing that not improve really but it could help in other cases.

\$\endgroup\$
1
\$\begingroup\$

In addition to @Mark-Andres good anwser:

favor composition over inheritance

do not inherit from JFrame

Your class gui extends JFrame, but it does not add new behavior to it. You just configure its content, but this can be done from "outside".

You should rather create a separate JFrame object in your main method pass that as parameter to your classes and use this for configuration:

public static void main(String args[])
    {
        JFrame theFrame = new JFrame("Changing text program");
        gui go = new gui(theFrame);
      /// do all the configuration at the same place...
      //  theFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
      //  theFrame.setSize(317,220);
        theFrame.setVisible(true);
    }

public class gui  {
  // ...
   public  gui(JFrame theFrame){
       theFrame.setLayout (new FlowLayout());
       theFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
       theFrame.setSize(317,220);

       theFrame. add(tf);
       theFrame. add (regularRadio);
       // ...
   }
}

do not implement *Listener interfaces on class level

Your class implements the ActionListener interface. But you have more that one action source to listen to.

The consequence is the nasty if cascade in the overridden method.

Almost all *Listener interfaces have only few methods to implement, usually only one. They are meant to be implemented as anonymous inner classes.

So instead having your top level class implementing the interface you should do it like this:

    // "legacy style"
    regularRadio.addActionListener(new ActionListener(){
        public void actionPerformed(ActionEvent e){
          tf.setText("You picked regular radio button");
          tf.setFont(new Font ("David" , Font.PLAIN , 14));
        };
    });
    // Java8 Lambda-style
    boldRadio.addActionListener(e -> {
        tf.setText("You picked bold radio button");
        tf.setFont(new Font ("David" , Font.BOLD , 14));
      });
\$\endgroup\$
  • \$\begingroup\$ Wow I forgot about the inheritance of JFrame! It used to be something I saw immediately! The Java 8 style for JFrame is rather nice looking, it make it a bit less painful to read! \$\endgroup\$ – Marc-Andre Jan 5 '17 at 20:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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