patternjavaMinor
Reducing repetition and utlizing EDT
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:
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
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 :
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
It was hard at first glance to see that there was an if-else here. You should use
Use only one style of formatting. Sometimes you're using :
And I see that you use this variant too :
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 :
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.