views:

994

answers:

11

Hi folks,

Just wondering if the following is considered to be good programming practice or not? I like to keep my individual source files as concise and uncluttered as possible, but I'm wondering what more experienced coders would think of it. I especially like the idea of the Settings.java class to keep all of my "Magic Numbers" in the one place. Has anyone any suggestions as to how I might improve on things?

Thanks for your help.

Happy coding :-)

class ApplicationLauncher 
{
    public static void main(String[] args) 
    {
     SwingApplication mySwingApplication = new SwingApplication();
    }
}

//////////////

import javax.swing.*;

public class SwingApplication extends JFrame
{
    public SwingApplication()
    {  
     JFrame myJFrame = new JFrame();
     myJFrame.setSize(Settings.frameWidth, Settings.frameHeight);
     myJFrame.setVisible(true);  
    }
}

//////////////

class Settings 
{
    static int frameWidth = 100;
    static int frameHeight = 200;
}
+5  A: 

Having special classes with magic numbers as static members is a good Java practice.

As programs grow, multiple settings classes, each with descriptive names, can be used.

Macker
+3  A: 

Macker covered it well. In addition, doing it this way will allow you to, in the future, move some of the settings easily into actual user preferences so they can customize different parts of the program. Since all your settings are already isolated into their own classes, this will require a minimal amount of effort on your part.

Marc W
+6  A: 

There's nothing wrong with having a settings class; however, in your example the settings are rather ambigious in terms of what frame they apply to, neither are they actual settings, but rather default values that strictly belong in the SwingApplication class.

Another thing which we don't have much control over is how the constructor call in Swing cascades into the program's message pump loop.

To me it has never made sense with a constructor that never returns (unless the frame is closed) and does more than initialize an object.

Cecil Has a Name
+4  A: 

Some like to group all of this stuff, the magic numbers etc... in a big and ugly XML file which will be read (and made sense of) at runtime. Your approach is obviously okay for a small project (e.g. general coursework) but think about the obvious advantage of getting these Settings from an XML file: you will not need to recompile your source code in order to reflect changes made to your settings :)

Peter Perháč
or a properties file
digitaljoel
+2  A: 

I think that can be a good practice as long as the settings are unlikely to change and you document the relationship between the settings. If these are likely to change then config files and/or command line arguments make more sense, since they don't require recompilation.

e5
+5  A: 

You are using what some folks call the "dreaded constants interface antipattern", though usually the constants are in an interface that is imported. I don't have a problem with it, especially since the advent of static imports, but perhaps someone will fill us in on the terrible evils. One of them seems to be "that is not what interfaces are for".

Of more concern is that you should be starting your GUI in a thread:

    //Schedule a job for the event-dispatching thread: creating
    //and showing this application's GUI.
    SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                JFrame myJFrame = new JFrame();
                myJFrame.setSize(Settings.frameWidth, Settings.frameHeight);
                myJFrame.setVisible(true);
            }
        });
Glenn
The antipattern was defining an interface and then importing it, not using it as a static class like this. The static imports have particularly made importing and interface both unnecessary as will as "that is not what interfaces are for".
Kathy Van Stone
+1  A: 

Mutable statics are a really bad idea. Stick with "Parameterisation from Above".

It was directly asked, but the example code has other problems. You have extended JFrame (a bad practice), but then ignored that and created another JFrame to actually use. Also you need to include the boilerplate to always access Swing components on the AWT Event Dispatch Thread (EDT).

Tom Hawtin - tackline
+2  A: 

If you are going to use statics for your magic numbers, make sure they're also final, if you mean for them not to change.

dj_segfault
Be careful! If they're final, the compiler will copy the values into the code that references them (as an optimization). This means if you change the values of the constants, you need to recompile all the code that uses those constants! (This is especially bad if the constants are in a separate jar)
Scott Stanchfield
A: 

You might want to look into JSR 296 (Swing Application Framework) for dealing with your GUI settings/launching/properties.

willcodejavaforfood
A: 

Another solution, which does not require static imports, would be to create a complete "ApplicationSettings" class with fields, getters and setters, and pass an instance of this class to the constructor of the class that needs the parameters. This allows you to keep a configuration object which can easily be persisted or modified if you want to save the new size if the user resizes the window, for example.

+4  A: 

As others have said, this is perfectly good practice, but there are some things you can do to improve the code:

  • Give the Settings class a private no-argument constructor. This makes it impossible to instantiate and makes its intent as a repository of constants clearer.
  • The settings should be final (immutable) as well as static.
  • Typically, constants in Java are written LIKE_THIS rather than likeThis.
  • If you're using Java 1.5 or greater, you can use import static Settings.FRAME_WIDTH; in your classes to be able to use FRAME_WIDTH directly instead of having to write Settings.FRAME_WIDTH.

This ends you up with:

class Settings
{
    /** Do not instantiate! */
    private Settings() {}

    static final int FRAME_WIDTH = 100;

    static final int FRAME_HEIGHT = 200;
}
Zarkonnen
Be careful! If they're final, the compiler will copy the values into the code that references them (as an optimization). This means if you change the values of the constants, you need to recompile all the code that uses those constants! (This is especially bad if the constants are in a separate jar)
Scott Stanchfield
Wow, I didn't know that, but it explains some things I had previously blamed on NetBeans. I still think they should be made final for sanity's sake though.
Zarkonnen