HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Reducing repetition and utlizing EDT

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
edtreducingrepetitionandutlizing

Problem

I am working on the largest Java program I have undertaken to date and feel uncertain in my current code practices. I think that I state and repeat things that don't need to be which can become a headache as development goes on. Specifically, in my GridBag Constraints. I have worked with GridBagLayout before and had gotten what I needed without declaring the width and height of cells that are only (1,1), yet here I was having problems so I decided to define everything I could think of for every cell and I got what I wanted, but now I probably have unnecessary code.

My other concern is in regards to utilizing the EDT. I've done my best to find lessons and videos on the subject but still feel that I have a lack of sufficient understanding.

FrontierMain.java:

public class FrontierMain  {

    public FrontierMain(){
        JFrame frame = new JFrame();
        frame.setTitle("Frontier Insulation Labor Record Tool");
        frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
        LaborRecordPanel recordPanel = new LaborRecordPanel();
        frame.add(recordPanel.scrollPane);
        frame.pack();
        frame.setLocationRelativeTo ( null );
        frame.setVisible(true);

    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable(){
            public void run(){
                FrontierMain fm = new FrontierMain();   
            }
        });

        System.out.println("Hello, World");
    }

}


LaborRecordPanel.java:

```
public class LaborRecordPanel implements
Printable, ActionListener {

private Color shade = new Color(201,201,201); // color for shaded cells

private JLabel dateSpace[] = new JLabel[7];
private JLabel grandTotalSpace = new JLabel();
private JLabel locAndDescSpace = new JLabel();
private JLabel personnelSpace[] = new JLabel[10]; // empty cells
private JLabel calendarGridLines[] = new JLabel[300]; //empty labels for gridlines

private ImageIcon logoIcon = ne

Solution

From a quick look at your code, I can say that your constructor is too big. I would suggest to refactored your constructor into smaller functions with only one task to do. I would probably have different method for each "zone" or "group" of elements.

There is a lot of magic numbers in your code. Why 5? Why not 6 ? You could use variable with names instead of directly use the number. It would help grasp easily why you're using 5 (Maybe it's the size of a ideal row, etc). A good example of a magic number would be this code :

private JLabel[] ot2 = new JLabel[10];
private JLabel[] ot1 = new JLabel[10];
private JLabel[] st = new JLabel[10];


Why are you using 10 ? I guess is the number of columns or somethings like that. But what happens if you need to change it to 11 ? You need to replace each occurence of 10 to 11. But 10 conveys a meaning, it's the number of columns, so you could have private final int NUMBERS_OF_COLUMNS = 10. It would clear to anyone reading your code that you're creating an of labels for each columns.

if(flip) dateSpace[k].setBorder(BorderFactory.createMatteBorder(0, 0, 1, 0, Color.BLACK));
else dateSpace[k].setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));


It was hard at first glance to see that there was an if-else here. You should use {} to help see at first glance that there is an if statement. This will improve readability and will help a lot with unsuspected bugs or changing the code later on.

Use only one style of formatting. Sometimes you're using :

for (...)
{

}


And I see that you use this variant too :

for (...) {

}


Choose only one and keep it in all your code (this is something that an IDE can do automatically).

One final tips that I could give you, find relevant names for your variables. You're doing a good job for the majority of your code, but sometimes you'r using contraction that are not that helpful. For example : JPanel rp = new JPanel(); What is an rp ? An other example : GridBagLayout gridbag = new GridBagLayout();. We already know that it's an grid bag by the declaration of the class. You could use something more meaningful. Is the principal container or a container for the data of the weeks or anything else? I always find more helpful to know at glance what the container is used for, just by reading his name.

Code Snippets

private JLabel[] ot2 = new JLabel[10];
private JLabel[] ot1 = new JLabel[10];
private JLabel[] st = new JLabel[10];
if(flip) dateSpace[k].setBorder(BorderFactory.createMatteBorder(0, 0, 1, 0, Color.BLACK));
else dateSpace[k].setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
for (...)
{

}
for (...) {

}

Context

StackExchange Code Review Q#42309, answer score: 6

Revisions (0)

No revisions yet.