9
\$\begingroup\$

Around June of last year, I made my very first GUI 'app.' Wish I knew of this amazing site then, I thought for posterity's sake it would be great to get it reviewed.

It's a simple I/O window just to keep track of the quantity of X exercise that was done, and if I used these two other workout apps.

I'd like to evaluate:

  1. The good vs bad practices in the code.
  2. Any pitfalls I don't account for.
  3. General feedback & suggested implementations on all aspects of the code.
import javax.swing.ImageIcon;
import javax.swing.JButton;
import javax.swing.JCheckBox;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JTextField;
import javax.swing.WindowConstants;
import java.awt.GridLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.io.File;
import java.io.FileWriter;
import java.io.PrintWriter;
import java.text.SimpleDateFormat;
import java.util.Calendar;

public class Workout extends JFrame {

    public Workout() {
        setLayout(new GridLayout(5, 3));
        // Labels
        add(new JLabel("Workout"));
        add(new JLabel("Sets"));
        add(new JLabel("Reps"));
    }

    public static void main(String[] args) throws Exception {
        // Get current date information
        Calendar cal = Calendar.getInstance();
        // Set format to month/day/year hours:minutes
        SimpleDateFormat sdf = new SimpleDateFormat("MM/dd/yy HH:mm a");

        // Creates file object and sets pointer
        File file = new File("Workout Log.txt"); // File name + path
        // Ensures file exists and is not a directory
        if (!file.exists() || file.isDirectory()) {
            PrintWriter writer = new PrintWriter("Workout Log.txt", "UTF-8");
            writer.close();
        }

        // Write to file without overwriting current contents
        final PrintWriter output = new PrintWriter(new FileWriter(file, true));
        output.println("\n" + sdf.format(cal.getTime())); // Add date to file
        // Create Frame
        JFrame frame = new Workout();
        frame.setTitle("My Workout Tracker");
        /* Commented out for CR
        frame.setIconImage(new ImageIcon("Images/Muscle.png").getImage()); */

        // workout, set, and rep fields for data
        final JTextField[] workouts = {
            new JTextField("Push ups"),
            new JTextField("Dumbell Presses"),
            new JTextField("Dumbell Curls")
        };
        final JTextField[] sets = new JTextField[3];
        final JTextField[] reps = new JTextField[3];

        // add textfield pointers + add textfields to frame
        for (int i = 0; i < 3; i++) {
            frame.add(workouts[i]);
            sets[i] = new JTextField();
            frame.add(sets[i]);
            reps[i] = new JTextField();
            frame.add(reps[i]);
        }

        // Create checkboxes
        final JCheckBox abs = new JCheckBox("Abs?");
        final JCheckBox squat = new JCheckBox("Squats?");
        // Create Listener
        ActionListener checkListener = new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                if (e.getSource() == abs) {
                    if (abs.isSelected()) {
                        output.println("Ab Workout app used!");
                    }
                }
                else if (e.getSource() == squat) {
                    if (squat.isSelected()) {
                        output.println("Squat Workout app used!");
                    }
                }
            }
        };
        // Register listener with checkboxes
        abs.addActionListener(checkListener);
        squat.addActionListener(checkListener);
        // Add checkboxes to frame
        frame.add(abs);
        frame.add(squat);
        JButton log = new JButton("Log"); // To log everything
        // Register + create Event Listener
        log.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                for (int i = 0; i < 3; i++) {
                    // Checks if fields are populated
                    if (sets[i].getText().length() > 0 && reps[i].getText().length() > 0) {
                        if (Integer.parseInt(sets[i].getText()) > 0 && Integer.parseInt(reps[i].getText()) > 0) {
                            output.println(
                                sets[i].getText() + " x " + reps[i].getText() +
                                " " + workouts[i].getText()
                            );
                        }
                    }
                }
                output.close();

                try {
                    Thread.sleep(500);
                }
                catch (InterruptedException i) {
                    i.printStackTrace();
                }
                System.exit(0);
            }
        });

        frame.add(log);
        frame.pack();
        frame.setLocationRelativeTo(null);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setVisible(true);
    }
}
\$\endgroup\$
10
\$\begingroup\$

Okay, lets begin at the very beginning (since that's a very good place to start. Allegedly).

Swing is the Java GUI framework (now EOL, so probably not worth learning really). As with most GUI frameworks, it's single threaded. It replaces AWT, which was an attempt as a thread safe framework. It was decided, for a number of reasons, that thread safety and GUIs don't mix.

For more information on why exactly, consider reading this excellent article. But for now, lets just take that as read.

So, Swing is single threaded. What does that mean for your code. Well, Swing requires that all interactions with Swing components happen from the Event Dispatch Thread (EDT). You application runs the initialisation and design of the GUI on the application's main thread.

The usual idiom for this is to put your startup logic somewhere else, maybe a GUI class, many an init method and call SwingUtilities.invokeLater. This queues up the initialisation logic on the EDT event queue for later.


I will assume Java 8 is being used for the rest of this review.

Now onto the code. We have 3 distinct processes that the code goes through:

  1. startup
  2. user input
  3. writing output

Currently all these are bundled in the same class, and actually the same method.

Lets begin by extracting a Model, we won't be aiming for a full Model View Presenter pattern here, but merely try to illustrate code separation.

The Model stores the data and contains the business logic for that data. What you have are 3 different workout types, where each workout has a name a reps and a sets. This gives us a starting point.

To avoid confusion lets rename your Workout class to WorkoutApplication.

Now, lets create a Workout class:

public class Workout {

    private String name;
    private int reps;
    private int sets;

    //getters and setters

    //equals and hashCode

    //toString
}

We need to add all the standard Java gumpf: getters and setters, equals and hashCode and toString. I won't cover those here, lets just assume they exist and are correct.

Now, our model has one more role that it needs to perform, it needs to write itself to a file. Since a file is simply a number of lines, each of which is a String, lets add a method to the model to turn itself into a file representation:

public String toFileRepresentation() {
    return String.format("%s x %s %s", sets, reps, name);
}

This simply uses a format string to define how to stick together the details of this particular Workout, for storing in a File.


Now lets move onto our interface Presenter. What does that need to do?

I will ignore the abs & sqat checkboxes because they don't seem to do anything.

The Presenter needs to present inputs for some number of workouts. It then needs to ability to save the current state of those inputs back onto the workouts in question.

Lets define two methods on our interface:

public interface WorkoutPresenter {

    void edit(List<Workout> workouts);

    List<Workout> flush();
}

So we can edit a List of workouts onto out presenter, so show them for editing. And we can flush our presenter, to return the List of workouts as it currently stands.

We also, however, need another interface. This we can call a UIHandler interface. This is the interface that the Presenter will use to fire events back to the View. This only needs one method, to be called when log is clicked.

public interface WorkoutUiHandler {
    void doLog();
}

So, now we can implements our WorkoutPresenter. Lets create a WorkoutPresenterImpl class.

public class WorkoutPresenterImpl extends JFrame implements WorkoutPresenter {

    private final WorkoutUiHandler workoutUiHandler;
    private final JPanel workoutPanel = new JPanel();
    private WorkoutTableModel workoutTableModel;

    public WorkoutPresenterImpl(WorkoutUiHandler workoutUiHandler) {
        this.workoutUiHandler = workoutUiHandler;
        setLayout(new BorderLayout());
        setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        setTitle("My Workout Tracker");
        add(workoutPanel, BorderLayout.CENTER);
        final JButton log = new JButton("Log");
        log.addActionListener(e -> workoutUiHandler.doLog());
        add(log, BorderLayout.SOUTH);
        setVisible(true);
        pack();
    }

    @Override
    public void edit(List<Workout> workouts) {
        workoutPanel.removeAll();
        workoutTableModel = new WorkoutTableModel(workouts);
        final JTable jTable = new JTable(workoutTableModel);
        final JScrollPane scrollPane = new JScrollPane(jTable);
        workoutPanel.add(scrollPane);
        pack();
    }

    @Override
    public List<Workout> flush() {
        return workoutTableModel.getWorkouts();
    }

    private static class WorkoutTableModel extends AbstractTableModel {

        private static final int WORKOUT_COL = 0;
        private static final int SET_COL = 1;
        private static final int REP_COL = 2;
        private static final String[] COLUMNS = {"Workout", "Sets", "Reps"};
        private final List<Workout> workouts;

        private WorkoutTableModel(List<Workout> workouts) {
            this.workouts = workouts;
        }

        public List<Workout> getWorkouts() {
            return workouts;
        }

        @Override
        public int getRowCount() {
            return workouts.size();
        }

        @Override
        public int getColumnCount() {
            return COLUMNS.length;
        }

        @Override
        public Object getValueAt(int rowIndex, int columnIndex) {
            final Workout workout = workouts.get(rowIndex);
            switch (columnIndex) {
                case WORKOUT_COL:
                    return workout.getName();
                case SET_COL:
                    return workout.getSets();
                case REP_COL:
                    return workout.getReps();
                default:   
                    throw new IllegalArgumentException("Unknown column " + columnIndex);
            }
        }

        @Override
        public String getColumnName(int column) {
            return COLUMNS[column];
        }

        @Override
        public Class<?> getColumnClass(int columnIndex) {
            if (columnIndex == SET_COL || columnIndex == REP_COL) {
                return Integer.class;
            }
            return super.getColumnClass(columnIndex);
        }

        @Override
        public boolean isCellEditable(int rowIndex, int columnIndex) {
            return columnIndex == SET_COL || columnIndex == REP_COL;
        }

        @Override
        public void setValueAt(Object aValue, int rowIndex, int columnIndex) {
            final Workout workout = workouts.get(rowIndex);
            switch (columnIndex) {
                case SET_COL:
                    workout.setSets((Integer) aValue);
                    break;
                case REP_COL:
                    workout.setReps((Integer) aValue);
                    break;
                default:
                    throw new IllegalArgumentException("Cannot edit " + columnIndex);
            }
        }
    }
}

That's a mighty chunk of code. The main components are that I have used a JTable for the editor. This allows an arbitrary number of different types of workout.

I have used the JTable editor functionality to provide editing services, rather than implementing that manually.

So the Presenter is given a List<Workout> to edit. The Presenter simply wraps that into a TableModel and creates a JTable. When time comes to save those changes, it simply returns the underlying data.

The separation could be greater, and you could make more use of the UIHandler class to callback to the view if changes are made. This will do for now.


So now into the View. This will be out WorkoutApplication. As we have moved the GUI code to the Presenter and the model to the Model we have very little left to do here:

public class WorkoutApplication implements WorkoutUiHandler {
    private static final Path file = Paths.get("Workout Log.txt");
    private final DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern("MM/dd/yy HH:mm a");

    private final LocalDateTime start;
    private WorkoutPresenter workoutPresenter;

    public WorkoutApplication() {
        start = LocalDateTime.now();
    }

    public static void main(String[] args) throws Exception {
        if (!Files.isWritable(file)) {
            throw new IOException("Output path is not writeable.");
        }
        SwingUtilities.invokeLater(() -> {
            final WorkoutApplication application = new WorkoutApplication();
            application.init();
        });
    }

    public void init() {
        workoutPresenter = new WorkoutPresenterImpl(this);

        final List<Workout> workouts = new ArrayList<>();
        final Workout pushUps = new Workout();
        pushUps.setName("Push ups");
        workouts.add(pushUps);
        final Workout dumbellPresses = new Workout();
        dumbellPresses.setName("Dumbell Presses");
        workouts.add(dumbellPresses);
        final Workout dumbellCurls = new Workout();
        dumbellCurls.setName("Dumbell Curls");
        workouts.add(dumbellCurls);

        workoutPresenter.edit(workouts);
    }

    @Override
    public void doLog() {
        final List<Workout> workouts = workoutPresenter.flush();
        try (final PrintWriter printWriter = new PrintWriter(Files.newBufferedWriter(file, StandardOpenOption.APPEND))) {
            printWriter.println();
            printWriter.println(start.format(dateTimeFormatter));
            workouts.stream()
                    .filter(w -> w.getReps() > 0 && w.getSets() > 0)
                    .map(Workout::toFileRepresentation)
                    .forEach(printWriter::println);
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}

So the WorkoutApplication creates the List<Workout> to be edited, this is currently hardcoded, but could be read from a file. It then sends that List to the Presenter to show it.

The doLog method is a callback from the Presenter to store the data to a file. This has much the same logic as your original method, except that we use Java 8 streams.

Finally, I have remove the sleep and System.exit as it makes little sense. To exit, simply close the window. Anything else will be surprising to the end user.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ A++. I'm humbled to receive such a great answer to my question, you have my gratitude. I really appreciate the time you spent on this, it's an entire lesson for me. I can't thank you enough. \$\endgroup\$ – Legato Feb 1 '15 at 16:02
8
\$\begingroup\$

One big point is separation of concerns.

Mixing GUI-Code with IO-Code is not recommended.

Some pseudo Code.

Make classes for Workout where just the information of your Workout is Stored.

class Workout { 
  List<WorkoutPart> parts;
  boolean abs;
  boolean squat;
}

class WorkoutPart {
  String description;
  int sets;
  int reps;
}

Make a class WorkoutWriter

class WorkoutWriter {
  WorkoutWriter(String path) {
    // prepare FileWriting
  }
  void write(Workout workout) {
    // Write workout to File
  }
}

In your GUI's ActionListener, save the Data in a new Workout-Instance. Then save the Workout-Instance with a WorkoutWriter.

A big benefit with this aproach is, that you can switch your persistence easily to e.g. a Database or an Restful-Webservice.

Don't inherit from JFrame. Just create a JFrame like in your main-Method.

| improve this answer | |
\$\endgroup\$
7
\$\begingroup\$

Don't put everything in your main method. It means that you can't:

  • Reuse the code without editing it.
  • Test the code without running the entire application.

The UI should not be coupled with the business logic. The IO should not coupled with the UI or the business logic.


Extract magic numbers from your code and replace them with named constants. The length of your array is hard coded all over the place. What happens if you want to change the reps array to be a different size, but leave the sets array the same? Not all of the 3s mean the same thing. You have to find each one and decide what is means.


Why is there a Thread.sleep() call in the code that writes out the log file?


The code that writes out the log file closes the application. What if I want to write multiple different workouts? There is nothing to tell me that clicking this button will exit the application. Explicitly calling System.exit() should happen very sparingly and not tucked into business logic.

| improve this answer | |
\$\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.