4
\$\begingroup\$

I made a simple temperature converter to serve as an introduction to swing. I used Eclipse WindowBuilder and did not attempt to make the components more modular. I'm curious as to how you guys would organize this code to improve readability.

Also, if you have any other criticism I'd love to hear it.

import java.awt.BorderLayout;
import java.awt.EventQueue;

import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.border.EmptyBorder;
import javax.swing.JTextField;
import java.awt.TextField;
import java.awt.FlowLayout;
import javax.swing.JTextPane;
import javax.swing.JButton;
import javax.swing.JRadioButton;
import javax.swing.JLabel;
import javax.swing.UIManager;
import java.awt.event.ActionListener;
import java.awt.event.ActionEvent;
import javax.swing.ButtonGroup;

public class TempConvert extends JFrame {

private JPanel contentPane;
private final ButtonGroup buttonGroup = new ButtonGroup();

/**
 * Launch the application.
 */
public static void main(String[] args) {
    EventQueue.invokeLater(new Runnable() {
        public void run() {
            try {
                TempConvert frame = new TempConvert();
                frame.setVisible(true);
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    });
}

/**
 * Create the frame.
 */
public TempConvert() {

    setTitle("Temperature Converter");
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    setBounds(0, -33, 385, 272);
    contentPane = new JPanel();
    contentPane.setBorder(new EmptyBorder(5, 5, 5, 5));
    setContentPane(contentPane);
    contentPane.setLayout(null);

    JTextPane txtTempIn = new JTextPane();
    txtTempIn.setBounds(12, 46, 124, 26);
    contentPane.add(txtTempIn);

    JRadioButton celRadio = new JRadioButton("Celcius");
    buttonGroup.add(celRadio);
    celRadio.setBounds(212, 34, 67, 24);
    contentPane.add(celRadio);

    JRadioButton fahrenRadio = new JRadioButton("Fahrenheit");
    buttonGroup.add(fahrenRadio);
    fahrenRadio.setBounds(212, 62, 85, 24);
    contentPane.add(fahrenRadio);

    JLabel lblTem = new JLabel("Degrees");
    lblTem.setBounds(154, 49, 78, 16);
    contentPane.add(lblTem);

    JTextPane tempOutput = new JTextPane();
    tempOutput.setBackground(UIManager.getColor("Button.background"));
    tempOutput.setEditable(false);
    tempOutput.setBounds(12, 138, 267, 22);
    contentPane.add(tempOutput);

    JButton btnNewButton = new JButton("Convert");
    btnNewButton.setBounds(12, 84, 98, 26);
    contentPane.add(btnNewButton);
    btnNewButton.addActionListener(new ActionListener() {
        public void actionPerformed(ActionEvent arg0) {

            if(celRadio.isSelected()){
                try{
                    double temp=Double.parseDouble(txtTempIn.getText());
                    temp = temp * 1.8 + 32;
                    tempOutput.setText(Double.toString(temp));
                }catch(Exception e){
                    tempOutput.setText("Please enter a valid temperature.");
                }
            }else{
                try{
                    double temp=Double.parseDouble(txtTempIn.getText());
                    temp = (temp - 32) * (5.0/9.0);
                    tempOutput.setText(Double.toString(temp));
                }catch(Exception e){
                    tempOutput.setText("Please enter a valid temperature.");
                }
            }
        }
    });
}
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Simon Forsberg Nov 22 '16 at 21:37
  • \$\begingroup\$ Thank you. I hope my new post adheres to the guidelines. \$\endgroup\$ – MrHanSolo Nov 22 '16 at 21:56
4
\$\begingroup\$

Inheritance

Do not extend any class unless you you have an "is a" relationship and add new behavior to the extended class.

You extend JFrame without adding Frame related behavior, you just configure it.

I don't understand the objection to extending JFrame here. The program needs a top level container.

Right, but you program does not need to inherit form JFrame to get one:

public class TempConvert {

private JPanel contentPane;
private final ButtonGroup buttonGroup = new ButtonGroup();

/**
 * Launch the application.
 */
public static void main(String[] args) {
    EventQueue.invokeLater(new Runnable() {
        public void run() {
            try {
                TempConvert frame = new JFrame();
                new TempConvert(frame);
                frame.setVisible(true);
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    });
}

/**
 * Create the frame.
 */
public TempConvert(JFrame frame) {

    frame.setTitle("Temperature Converter");
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.setBounds(0, -33, 385, 272);
    frame.contentPane = new JPanel();
    frame.contentPane.setBorder(new EmptyBorder(5, 5, 5, 5));
    frame.setContentPane(contentPane);
    frame.contentPane.setLayout(null);

Layouting

When using NULL layout you throw away any convenience Layoutmanager could give you. Your UI will be bound to the "ideal" Frame size. The problem is that the definition of "ideal" changes with every user and every device your program runs on.

Magic Numbers

Because of your wrong interpretation of "Flexibility" that results in using the NULL Layout you need to specify bound of your buttons. And you do this by using random literal numbers (from the readers perspective). You should at least define constants for them.

Code duplication

Your code has lots of duplicated code.

You create a bunch of buttons. you could extract the creation of a single button to a parameterized method.

Also in your anonymous Listener class the try/catch block is duplicated. Here you could simply move one of then around the whole if and delete the other one.

\$\endgroup\$
1
  • \$\begingroup\$ I don't understand the objection to extending JFrame here. The program needs a top level container. The role of top level container is usually pretty minimal--they mostly ARE configured and hold things, like JPanels rather than DO things. In this case, the TempConvert class IS-A top-level container. That seems fine for something this small. My two cents. \$\endgroup\$ – Phil Freihofner Nov 22 '16 at 8:22
1
\$\begingroup\$

It would be helpful to use proper indenting. The main() and constructor really should be indented 4 spaces. Otherwise the eye may assume that these are additional classes rather than class members. (The eye is always trying to take shortcuts!)

When I first started with Swing, I stuck with the NULL layout approach for quite a while. One thing that got me out of it was figuring out that it is okay to nest layouts. You can do a lot with putting JPanels within JPanels, each a simple layout (e.g., break out the structure into horizonals and verticals and use nested BoxLayouts, for starters). I've switched over to JavaFX instead of Swing and it is even easier to do this there.

I'd give btnNewButton a more descriptive name, such as btnConvert, since it triggers the conversion. In the action listener, in both cases you read in the temperature, so consider making this a single method that returns the double temp. Then, you can do whatever is needed within that method to validate the input and try/catch if something goes wrong, in just that one place.

After getting your input, then do the branch. I've taken to listing each branch option explicitly rather than using if/else. I like to be able to quickly see what the section I'm looking at is supposed to do. It is much easier to know what is going on if the second option is

if (fahrenRadio.isSelected()) { ... 

rather than

else { ...

and having to remember what "else" refers to or having to search the code for the meaning of the "else".

Those are my suggestions for some small improvements.

\$\endgroup\$
1
  • \$\begingroup\$ This was my first stackexchange post, so the code formatting is a little new to me. In my IDE, I use the formatting that you're describing. I haven't played around with the layout much, but I get what you're saying with multiple panels; Just like divs in HTML. \$\endgroup\$ – MrHanSolo Nov 22 '16 at 21:37
0
\$\begingroup\$

Thanks for the response guys. I ended up taking the advice, as well as redoing the entire program without using WindowsBuilder.

Main

public class Main {



    public static void main(String[] args) {

        javax.swing.SwingUtilities.invokeLater(new Runnable() {
            public void run(){
                ConvertFrame frame = new ConvertFrame();
                frame.show();
            }
        });
    }

}

Converter

public class Converter{

    private static String radioSelected;
    private static String textInput;
    private static String textOutput;
    private static Double temp;

    public Converter(String radioSelected, String textInput){
        Converter.radioSelected = radioSelected;
        Converter.textInput = textInput;
        convertTemp();
    }

    public static void convertTemp(){
        try{
            if(radioSelected==""){
                textOutput= "Select temperature unit.";
            }
            if(radioSelected=="celcius"){
                temp=Double.parseDouble(textInput);
                temp = temp * 1.8 + 32;
                textOutput=Double.toString(temp);
            }
            if(radioSelected=="fahrenheit"){
                temp=Double.parseDouble(textInput);
                temp = (temp - 32) * (5.0/9.0);
                textOutput=Double.toString(temp);
            }
        }catch(Exception e){
            textOutput = "Enter valid temperature.";
        }
    }

    public String toString(){
        return textOutput;
    }

}

UI

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

import javax.swing.BoxLayout;
import javax.swing.ButtonGroup;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JRadioButton;
import javax.swing.JTextPane;
import javax.swing.border.EmptyBorder;

public class ConvertFrame{

    private JFrame frame;
    private JPanel panel;

    private final ButtonGroup radioButtons;

    private JButton button;

    private JRadioButton celciusRadio;
    private JRadioButton fahrenheitRadio;

    private JTextPane tempIn;
    private JTextPane tempOutput;

    private Converter converter;

    public ConvertFrame(){
        //Frame
        frame = new JFrame("Temperature Converter");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

        //JPanel
        panel = new JPanel();
        panel.setBorder(new EmptyBorder(5,5,5,5));
        panel.setLayout(new BoxLayout(panel, BoxLayout.PAGE_AXIS));
        panel.setOpaque(true);

        createTextBoxIn();

        radioButtons = new ButtonGroup();

        celciusRadio = new JRadioButton("Celcius");
        addRadioBtn(celciusRadio);

        fahrenheitRadio = new JRadioButton("Fahrenheit");
        addRadioBtn(fahrenheitRadio);

        createBtn("Convert");

        createTextBoxOut();
    }

    public void createTextBoxIn(){
        tempIn = new JTextPane();
        panel.add(tempIn);
    }

    public void createBtn(String btnName){
        button = new JButton(btnName);
        panel.add(button);

        button.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent arg0){
                String tempInput = tempIn.getText();
                String radioSelected = "";
                if(celciusRadio.isSelected()){
                    radioSelected = "celcius";
                } 
                if(fahrenheitRadio.isSelected()){
                    radioSelected = "fahrenheit";
                }
                converter = new Converter(radioSelected,tempInput);
                tempOutput.setText(converter.toString());
            }
        });
    }

    public void addRadioBtn(JRadioButton radioBtn){
        radioButtons.add(radioBtn);
        panel.add(radioBtn);
    }

    public void createTextBoxOut(){
        tempOutput = new JTextPane();
        tempOutput.setEditable(false);
        panel.add(tempOutput);
    }

    public void show() {
        frame.setVisible(true);
        frame.setContentPane(panel);
    }
}
\$\endgroup\$

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.