12
\$\begingroup\$

I'm a beginner to Java and programming in general and since I'm on a holiday to the US, I thought it would be a fun idea to write a program for converting Fahrenheit to Celsius and the other way around. I'm using WindowBuilder for the gui part so I suppose my code could use some improvement.

The main thing that's bothering me is that my code doesn't look very OO, I'm just using one class and my layout and logic isn't separated at all.

Thank you very much for any hints you could give me!

import java.awt.Component;
import java.awt.EventQueue;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.JButton;
import javax.swing.JCheckBox;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JOptionPane;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.border.EmptyBorder;

public class Converter extends JFrame implements ActionListener 
{
    private static final long serialVersionUID = 1L;

    private JPanel contentPane = new JPanel();

    private JTextField textFieldCelsius = new JTextField();
    private JTextField textFieldFahrenheit = new JTextField();

    private JCheckBox checkBoxCelsiusToFahrenheit = new JCheckBox(" °C to  °F");
    private JCheckBox checkBoxFahrenheitToCelsius = new JCheckBox(" °F to  °C");

    private JLabel lblCelsius = new JLabel(" °C");
    private JLabel lblFahrenheit = new JLabel(" °F");

    private JButton btnClear = new JButton("Clear");
    private JButton btnConvert = new JButton("Convert");

    private Object[] myObjects =
    { textFieldCelsius, textFieldFahrenheit, checkBoxCelsiusToFahrenheit,
        checkBoxFahrenheitToCelsius, lblCelsius, lblFahrenheit, btnClear,
        btnConvert };

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

    /**
     * Create the frame.
     */
    public Converter()
    {
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setBounds(100, 100, 450, 300);
        setLocationRelativeTo(null);
        setResizable(false);
        contentPane.setBorder(new EmptyBorder(5, 5, 5, 5));
        setContentPane(contentPane);
        contentPane.setLayout(null);

        positionComponents();

        btnConvert.addActionListener(this);
        btnClear.addActionListener(this);

        for (Object object2 : myObjects)
        {
            contentPane.add((Component) object2);
        }
    }

    public void positionComponents()
    {
        lblCelsius.setBounds(305, 96, 70, 15);
        checkBoxCelsiusToFahrenheit.setBounds(37, 8, 129, 23);
        checkBoxFahrenheitToCelsius.setBounds(254, 8, 129, 23);
        textFieldCelsius.setBounds(156, 94, 114, 19);
        textFieldFahrenheit.setBounds(156, 121, 114, 19);
        btnConvert.setBounds(254, 223, 117, 25);
        btnClear.setBounds(37, 223, 117, 25);
        lblFahrenheit.setBounds(305, 123, 70, 15);
        textFieldCelsius.setColumns(10);
        textFieldFahrenheit.setColumns(10);
    }

    public void clear()
    {
        textFieldCelsius.setText("");
        textFieldFahrenheit.setText("");
        checkBoxCelsiusToFahrenheit.setSelected(false);
        checkBoxFahrenheitToCelsius.setSelected(false);
    }

    public void convertToFahrenheit()
    {
        String text = textFieldCelsius.getText();
        int textInt = Integer.parseInt(text);
        int result = textInt * 9 / 5 + 32;
        String resultString = Integer.toString(result);
        textFieldFahrenheit.setText(resultString);
    }

    public void convertToCelsius()
    {
        String text = textFieldFahrenheit.getText();
        int textInt = Integer.parseInt(text);
        int result = (textInt - 32) * 5 / 9;
        String resultString = Integer.toString(result);
        textFieldCelsius.setText(resultString);
    }

    @Override
    public void actionPerformed(ActionEvent e)
    {
        if (e.getSource() == btnConvert)
        {
            try
            {
                if (checkBoxCelsiusToFahrenheit.isSelected())
                {
                    convertToFahrenheit();
                }
                else if (checkBoxFahrenheitToCelsius.isSelected())
                {
                    convertToCelsius();
                }
                else if (!checkBoxCelsiusToFahrenheit.isSelected()
                        && !checkBoxFahrenheitToCelsius.isSelected())
                {
                    JOptionPane.showMessageDialog(null,
                            "Please select a checkbox!");
                }
            }
            catch (Exception e2)
            {
                JOptionPane.showMessageDialog(null, "Invalid input!");
            }

        }
        else if (e.getSource() == btnClear)
        {
            clear();
        }
    }
}
\$\endgroup\$
1
  • 4
    \$\begingroup\$ From a UX standpoint, it doesn't make much sense to use checkboxes for mutually exclusive options. I would have used radiobuttons instead. \$\endgroup\$ – Mathieu Guindon Jul 19 '15 at 20:38
5
\$\begingroup\$

The main thing that's bothering me is that my code doesn't look very OO, I'm just using one class and my layout and logic isn't separated at all.

Yes, I think that's the crux of the issues identified too. :)

Formatting

With the exception of your myObjects declaration (perhaps it can start from the same line too?), your code's spacing is readable and quite consistent, which is good. Java's convention on braces is colloquially known as the 'Egyptian' style, which is different from what you are using. Your main() method's braces looks a little... out-of-place though. Perhaps that's due to the Markdown formatting?

UX/UI (a brief touch)

From a UX standpoint, it doesn't make much sense to use checkboxes for mutually exclusive options. I would have used radiobuttons instead. - Mat's Mug

Radio buttons are preferred for either-or options as they ensure only one state can be enabled. In your case, the user will not be able to check both checkboxes.

You should also use a layout manager to position your Swing UI components, instead of setting bounds explicitly inside positionComponents(). A layout manager tend to reduce the code you need to write, does away with 'magic numbers', and is more flexible when you want to resize your windows.

Using methods effectively

Currently, your conversion methods simply act on your textboxes directly, which makes it non-trivial for doing unit testing. Furthermore, this clubs together the presence of UI elements with calculations, which is not ideal. Instead, you can think of your conversion methods more like a function, that takes in an input and returns an output:

private static double convertToCelsius(double fahrenheit) {
    // calculate and return value
}

I'm not sure why you intended to lose precision by using ints in your question, but you're probably looking for the double type here. Anyways, your UI code can then call this method as such:

public void convertToCelsius() {
    double result = convertToCelsius(Double.parseInt(textFieldFahrenheit.getText()));
    textFieldCelsius.setText(Double.toString(result));
}

You can also see that I have cut down the number of temporary variables, as I often find that the more temporary variables a method has, the harder is it to read it (literally).

\$\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.