1
\$\begingroup\$

Can this time converter be written more efficiently? I'm new to Java, and I would like to know.

main

import javax.swing.JFrame;

public class main {
        public static void main(String[] args) {
    window wn = new window();
        wn.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        wn.setSize(275, 170);
        wn.setVisible(true);
    }
}

time

public class time {
    private int hour;
    private int minute;
    private String ampm;

    public void setTime(int h, int m, String t) {
        hour = ((h>=0 && h<=24) ? h : 0);
        minute = ((m>=0 && m<=60) ? m : 0);
        ampm = t;
    }

    public String to24h() {
        if (ampm == "PM") {
            hour += 12;
        }
        return String.format("%02d:%02d", hour, minute);
    }
}

window

import java.awt.FlowLayout;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JTextField;
import java.awt.event.ActionListener;
import java.awt.event.ActionEvent;
import javax.swing.JButton;
import javax.swing.JComboBox;

@SuppressWarnings("serial")
public class window extends JFrame {

    private JLabel label;
    private JTextField hours;
    private JTextField minutes;
    private JTextField output;
    private JButton twelve_button;
    public time timeConvert = new time();
    int tHours;
    int tMinutes;
    private String[] times = {"AM", "PM"};
    private JComboBox combo;
    String tAMPM;

    public window() {
        super("Time Converter");
        setLayout(new FlowLayout());

        label = new JLabel("Time");
        add(label);

        hours = new JTextField(2);
        add(hours);

        label = new JLabel(":");
        add(label);

        minutes = new JTextField(2);
        add(minutes);

        combo = new JComboBox(times);
        add(combo);

        twelve_button = new JButton("Convert to 24 hours");
        add(twelve_button);

        output = new JTextField(10);
        output.setEditable(false);
        add(output);

        label = new JLabel("It's my 'hello world' of java development!");
        add(label);

        label = new JLabel("by jackwilsdon");
        add(label);

        theHandler handler = new theHandler();

        hours.addActionListener(handler);
        minutes.addActionListener(handler);
        twelve_button.addActionListener(handler);
    }

    private class theHandler implements ActionListener {
        public void actionPerformed(ActionEvent event) {
            if (event.getSource() == twelve_button) {
                if (hours.getText() != null && minutes.getText() != null) {
                    try {
                        tHours = Integer.parseInt(hours.getText());
                        tMinutes = Integer.parseInt(minutes.getText());
                        tAMPM = (String)combo.getSelectedItem();
                        timeConvert.setTime(tHours, tMinutes, tAMPM);
                        if (tHours > 12) {
                            output.setText("Please enter a 12 hour number!");
                        } else if (tMinutes > 60) {
                            output.setText("Please enter a 12 hour number!");
                        } else {
                            output.setText(timeConvert.to24h());
                        }
                    }
                    catch(NumberFormatException nFE) {
                        output.setText("Please enter a number!");
                    }
                }
            }
        }
    }
}
\$\endgroup\$
7
\$\begingroup\$

You asked about efficiency, which I am not an expert in. However, if you are going to share your code here, it would be nice if you could adapt it to the Java code style conventions.
In short: you should use PascalCase for your class names, so...

  • Main.java instead of main.java
  • Time.java instead of time.java
  • Window.java instead of window.java

Also, using single-letter names for your parameters is discouraged (borderline in the case of the fairly obvious int h and int m, bad in the case of String t). Why not hours, minutes, and halfOfDaySuffix? I'm sure you could find an even better term for the latter with a little research.

If you haven't already, make sure you organize your code into one or more package(s), e.g. put package com.jackwilsdon.TimeConversion; at the beginning of your files. This helps avoid name clashing problems (for instance with java.sql.Time).

In regard to to24h(): Never compare two Strings with the == (reference equals) operator in Java. This will only work in a narrow range of scenarios, read the full explanation on StackOverflow. You need to use the String.equals method instead.

But while we are at it, you shouldn't be using a String to represent AM/PM anyway - it will stop working if you pass in the wrong string, for instance pm instead of PM by mistake. Instead, restrict the possible values by creating an Enum:

public enum DayHalf {
    AM,
    PM
}

Then (in Window), you can change tAMPM's type from String to DayHalf - the following line will stop working:

tAMPM = (String)combo.getSelectedItem();

So you need to use combo = new JComboBox(DayHalf.values()); instead of combo = new JComboBox(times); and change the above line to

tAMPM = (DayHalf)combo.getSelectedItem();

If you are passing in AM or PM, hours above 12 are not acceptable - neither are minutes above 59! Hence, your conditions are erroneous; also, silently failing by setting the fields to 0 isn't a robust solution. You should throw an IllegalArgumentException if the values are out of range.

Your updated setTime method would look something like this:

public void setTime(int hours, int minutes, DayHalf halfOfDaySuffix) {
    if (hours < 0 || hours > 12 || minutes < 0 || minutes > 59)
        throw new IllegalArgumentException("hours or minutes out of allowed range!");
    hour = hours;
    minute = minutes;
    dayHalf = halfOfDaySuffix;
}

Regarding your JFrame (Window.java, should probably be called TimeConversionWindow.java), you should adjust the obviously copied-and-pasted error message:

else if (tMinutes > 60) {
    output.setText("Please enter a 12 hour number!");
}

to "Please enter less than 60 minutes!" or similar. Note that your condition (tMinutes > 60) is incorrect once again and should be changed to > 59.

A couple of other adjustments will be required, but they are easy to figure out. I'd argue that performance is nearly irrelevant in this scenario, read up on bottlenecks.

\$\endgroup\$
  • \$\begingroup\$ There are many magic numbers like 0, 12, 59 this all should be final fields like maxMinutesInHour etc. \$\endgroup\$ – Anirban Nag 'tintinmj' Jul 29 '13 at 18:56
2
\$\begingroup\$
hours.getText() != null && minutes.getText() != null

You should also assert if the string is empty using method hours.getText().isEmpty()

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