2
\$\begingroup\$

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.myname.gutil.ConfirmDialog;
import com.myname.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.myname.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()}.
 * 
 */
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);
    }

}
\$\endgroup\$
1
  • \$\begingroup\$ Variables DEFAULT_ACCEPT_TEXT, DEFAULT_CANCEL_TEXT follows java naming convention for constants. \$\endgroup\$ Commented Nov 2, 2013 at 13:53

2 Answers 2

4
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$
    – VGR
    Commented Nov 3, 2013 at 14:34
  • \$\begingroup\$ 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. \$\endgroup\$
    – rolfl
    Commented Nov 3, 2013 at 14:40
  • \$\begingroup\$ The accepted variable name is not self-documenting. The comment should stay or the variable be renamed. The cancelButton comment is completely redundant. \$\endgroup\$ Commented Nov 3, 2013 at 21:44
1
\$\begingroup\$

My Points:

  1. No
  2. Yes
  3. Yes

Other points:

  • Cohesion is low. Please extrude the Listeners to seperate classes using an interface to change accepted/cancel captions.
  • If you use a WeakReference to the dialog and call dispose, you never find out if the user clicked Accept or Cancel.
  • Open-Close Principle sais you must depend on abstractions, so parent should be Component because Component is more abstract that JComponent. Also JButton is not abstract. The constructors should be AbstractButton instead. (I advise to not use CharSequence over String by the way).
  • Demeters law is respected by 80%, thats another good thing. To improve that, drop the setAcceptText and setCancelText methods.
  • I like to know more about the fields, you could documentate if null values are allowed and what happens if they are (in example a null title will be an empty title).
  • Constructors are initialized async, if you call all constructors in one block, they will be executed mostly synthetic paralell.
  • The package ends with gutil... Is gutil a abbrev for something else than Utils? If not that lets assume ConfirmDialog is a component, not a util class.
  • ISP not affected here.
  • SRP solved good.
  • In the class javadoc please use a <p> tag for the second paragraph (only the open tag, not the close tag).
  • Class is not thread-safe, please point that out.
  • Class is not final, a possible superclass might be ConfirmRetryDialog but as I have no access to the optionPanel, I cant add a Retry-Button.
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.