Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote a small Java class with documentation that is going to be a part of a library I am writing. I learned Java as my first programming language from many different sources on the web, and therefore I have developed my own style without any direct criticism along the way. So I would like some feedback on the class because I am unsure of what is good practice and what is just unnecessary. I am thinking mainly of the design and style, especially on the javadoc, but any other corrections or suggestions are also welcome.

Some specific questions:

  1. Are the DEFAULT_ACCEPT_TEXT and DEFAULT_CANCEL_TEXT variables
    overkill?
  2. Should I create methods for common operations on hidden
    objects (e.g. setAcceptText()) or should I just add a method that
    returns the object? In this case I did both.
  3. Should I merge the accept() and cancel() methods to something like: setAccepted(boolean accepted)? Like java.awt.Window's show() and hide() methods were replaced by setVisible(boolean visible).

I realize that a lot of what I am asking is about personal preference and that as long as I am persistent, it is fine.

Test.java

import java.awt.FlowLayout;

import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JTextField;

import com.sakratt.gutil.ConfirmDialog;
import com.sakratt.gutil.LookAndFeel;

public class Test {
        public static void main(String[] args) {
                JPanel content = new JPanel(new FlowLayout());
                JTextField input = new JTextField(12);
                content.add(new JLabel("Enter name:"));
                content.add(input);
                ConfirmDialog dialog = new ConfirmDialog("Test dialog", content);
                boolean yes = dialog.display();
                System.out.println(input.getText() + " " + (yes ? "accepted" : "did not accept") + ".");
        }
}

ConfirmDialog.java

package com.sakratt.gutil;

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

import javax.swing.JButton;
import javax.swing.JComponent;
import javax.swing.JDialog;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.border.EmptyBorder;

/**
 * A dialog that lets the user either confirm or cancel.
 * 
 * Display the dialog to the user by calling the {@link #display()} method. The text on the option
 * buttons can be changed by modifying the buttons returned by {@link #getAcceptButton()} and
 * {@link #getCancelButton()}.
 * 
 * @author Peter André Johansen
 * 
 */
public class ConfirmDialog extends JDialog {

    /**
     * The default accept button text.
     */
    private static final String DEFAULT_ACCEPT_TEXT = "Accept";

    /**
     * The default cancel button text.
     */
    private static final String DEFAULT_CANCEL_TEXT = "Cancel";

    /**
     * Whether the dialog is going to be accepted or not.
     */
    private boolean accepted;

    /**
     * The main component of the dialog.
     */
    private JComponent body;

    /**
     * The accept button.
     */
    private JButton acceptButton;

    /**
     * The cancel button.
     */
    private JButton cancelButton;

    /**
     * Creates a new confirm dialog without a body.
     */
    public ConfirmDialog() {
        this(null, null, null);
    }

    /**
     * Creates a new confirm dialog without a body.
     * 
     * @title the title
     */
    public ConfirmDialog(String title) {
        this(title, null, null);
    }

    /**
     * Creates a new confirm dialog.
     * 
     * @title the title
     * @param body the main component
     */
    public ConfirmDialog(String title, JComponent body) {
        this(title, body, null);
    }

    /**
     * Creates a new confirm dialog with a location relative to the parent.
     * 
     * @param title the title
     * @param body the main component
     * @param parent the parent
     */
    public ConfirmDialog(String title, JComponent body, JFrame parent) {

        // Accept button
        acceptButton = new JButton(DEFAULT_ACCEPT_TEXT);
        acceptButton.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent evt) {
                accept();
            }
        });

        // Cancel button
        cancelButton = new JButton(DEFAULT_CANCEL_TEXT);
        cancelButton.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent evt) {
                cancel();
            }
        });

        // Add the option buttons
        JPanel optionPanel = new JPanel();
        optionPanel.add(acceptButton);
        optionPanel.add(cancelButton);

        // Create the panel
        JPanel panel = new JPanel();
        panel.setBorder(new EmptyBorder(5, 5, 5, 5));
        panel.setLayout(new BorderLayout(0, 5));
        if (body != null) panel.add(body, BorderLayout.CENTER);
        panel.add(optionPanel, BorderLayout.SOUTH);

        // Prepare the dialog
        setContentPane(panel);
        setDefaultCloseOperation(JDialog.DISPOSE_ON_CLOSE);
        getRootPane().setDefaultButton(acceptButton);
        pack();
        setLocationRelativeTo(parent);
        setMinimumSize(getPreferredSize());
        setModal(true);
        setTitle(title);

    }

    /**
     * Accepts the dialog.
     */
    public void accept() {
        accepted = true;
        dispose();
    }

    /**
     * Cancels the dialog.
     */
    public void cancel() {
        accepted = false;
        dispose();
    }

    /**
     * Display the dialog until it is closed.
     * 
     * @return true if it closed because the accept option was chosen
     */
    public boolean display() {
        setVisible(true);
        return accepted;
    }

    /**
     * Returns the accept button.
     * 
     * @return the button
     */
    public JButton getAcceptButton() {
        return acceptButton;
    }

    /**
     * Returns the cancel button.
     * 
     * @return the button
     */
    public JButton getCancelButton() {
        return cancelButton;
    }

    /**
     * Returns the main component.
     * 
     * @return the body
     */
    public JComponent getBody() {
        return body;
    }

    /**
     * Sets the text of the accept button.
     * 
     * @param text the text
     */
    public void setAcceptText(String text) {
        acceptButton.setText(text);
    }

    /**
     * Sets the text of the cancel button.
     * 
     * @param text the text
     */
    public void setCancelText(String text) {
        cancelButton.setText(text);
    }

}
share|improve this question
    
Variables DEFAULT_ACCEPT_TEXT, DEFAULT_CANCEL_TEXT follows java naming convention for constants. –  Dark Knight Nov 2 '13 at 13:53
add comment

1 Answer

up vote 3 down vote accepted

You ask specific questions and the answers you get to them will be opinionated and subjective... but ...

  1. Using constants for the Defaults is a good thing. This will make subsequent efforts like i18n (internationalization) easier as the values can be replaced with resource lookups in just one place.
  2. You should not do both. Object Oriented programming guidelines suggest that you should encapsulate the internals of your object. You should not expose the inner workings. 'publishing' the actual inner objects is likely going to lead to issues later on. You should only allow others to do things to your object that you need to let them do, and they don't need. In your case I would remove the getAcceptButton() method, but this is subjective, and you may just need to remove the setAcceptText() and let the user customize the button fully. Doing both is a mistake though.
  3. The accept and cancel methods are fine, in principal, but what I worry about is why they are public. They should be private since the only place they should be called from is the actionPerformed() methods. How you do things in the private methods is up to you.

As a general comment I think you have too many comments.... ;-) When methods and fields are private, and not part of the JavaDoc of the class then I suggest you use a less verbose style for your comments. This is especially true for obvious methods and variables.

There is a lot of clutter in your code that relates to obvious things that just make it harder to read.

All your private members have good names, and, for example, the code:

/**
 * Whether the dialog is going to be accepted or not.
 */
private boolean accepted;
........
/**
 * The cancel button.
 */
private JButton cancelButton;

is much better to be expressed as :

private boolean accepted;
private JComponent body;
private JButton acceptButton;
private JButton cancelButton;

This is obvious, and there's no way for this to become a problem over time where the comments do not match the code. Your code should be self-documenting, which your code is, and as a result the actual comments are redundant.

share|improve this answer
    
There are reasons to write javadoc for a private field. accepted might benefit from javadoc describing when and how the value gets set. body is not immediately obvious, so Peter's javadoc is beneficial. I agree that the javadoc for acceptButton and cancelButton is redundant; to quote Sun's How to Write Doc Comments: Add description beyond the API name.… The ideal comment goes beyond those words and should always reward you with some bit of information that was not immediately obvious from the API name. –  VGR Nov 3 '13 at 14:34
    
Yes, there are reasons for private field comments, my point is that if you use 'self documenting' variable names (which essentially these are) then the comment is redundant. Adding a comment means you have two places to change when maintaining code. Getting the right balance is an art, and a fuzzy boundary. Perhaps I err too much to one side. –  rolfl Nov 3 '13 at 14:40
    
The accepted variable name is not self-documenting. The comment should stay or the variable be renamed. The cancelButton comment is completely redundant. –  Dave Jarvis Nov 3 '13 at 21:44
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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