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 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 = new ImageIcon("Images/fici_logo1.jpg");
    private JLabel logoLabel = new JLabel(logoIcon);

    private JLabel authorizedBy = new JLabel("AUTHORIZED BY:_____________________________________________________________");
    private JLabel toCertify = new JLabel("THIS IS TO CERTIFY THAT THE ABOVE LABOR HAS BEEN PERFORMED.");

    private JLabel laborRecordNO = new JLabel("NO.");
    private JLabel nameOfJob = new JLabel("NAME OF JOB:");
    private JLabel customerPO = new JLabel("CUSTOMER PO #:");
    private JLabel contractNO = new JLabel("CONTRACT NO.");
    private JLabel weekEnding = new JLabel("WEEK ENDING");

    private JLabel personnelList = new JLabel("<html>LIST SUPERVISION &<br> CRAFT LABOR BELOW:</html>");
    private JLabel locAndDescriptionLabel = new JLabel("LOCATION AND DESCRIPTION:");

    private JLabel personnelTitle = new JLabel("TITLE");

    private JLabel[] titleSpace= new JLabel[10];

    private JLabel calendarTitle = new JLabel("NUMBER OF HOURS WORKED @ SITE");
    private JLabel dayHeading = new JLabel("DAY");
    private JLabel dateHeading = new JLabel("DATE");
    private JLabel[] ot2 = new JLabel[10];
    private JLabel[] ot1 = new JLabel[10];
    private JLabel[] st = new JLabel[10];

    private JLabel mon = new JLabel("MON");
    private JLabel tues = new JLabel("TUES");
    private JLabel wed = new JLabel("WED");
    private JLabel thur = new JLabel("THUR");
    private JLabel fri = new JLabel("FRI");
    private JLabel sat = new JLabel("SAT");
    private JLabel sun = new JLabel("SUN");

    private JLabel totalHours = new JLabel("<html>TOTAL<br> HOURS</html>");
    private JLabel ratePerHour = new JLabel("<html>RATE<br> PER<br> HOUR</html>");
    private JLabel totalAmount = new JLabel("<html>TOTAL<br> AMOUNT</html>");
    private JLabel grandTotal = new JLabel("TOTAL");

    JButton printTest = new JButton("PrintTest");

    JPanel rp = new JPanel();
    JPanel outer = new JPanel();
    JPanel buttonPanel = new JPanel();
    JScrollPane scrollPane = new JScrollPane(outer);

    LaborRecordPanel(){

        outer.setLayout(new BoxLayout(outer,BoxLayout.Y_AXIS ));
        outer.add(rp);
        outer.add(buttonPanel);
        printTest.addActionListener(this);
        buttonPanel.add(printTest);
        rp.setBorder(null);
        rp.setPreferredSize(new Dimension(1295,1830 ));
        rp.setMinimumSize(new Dimension(1295,1830 ));
        rp.setMaximumSize(new Dimension(1295,1830 ));

        scrollPane.setPreferredSize(new Dimension(1295,800 ));

        scrollPane.getVerticalScrollBar().setUnitIncrement(50); //increase the scroll speed

        for (int i = 0; i <= 9; i++) titleSpace[i] = new JLabel(); // create mechanic labels

        for (int i = 0; i <= 9; i++) //create labels for work time
        {
            ot2[i] = new JLabel("OT-2");
            ot1[i] = new JLabel("OT-1");
            st[i] = new JLabel("S.T.");
        }
        //create empty labels for gridlines
        for (int i = 0; i <= 9; i++) personnelSpace[i] = new JLabel(); 
        for (int i = 0; i <= 6; i++) dateSpace[i] = new JLabel();
        for (int i = 0; i <= 299; i++) calendarGridLines[i] = new JLabel();

        GridBagLayout gridbag = new GridBagLayout();
        rp.setBackground(Color.WHITE);
        GridBagConstraints gbc = new GridBagConstraints();
        rp.setLayout(gridbag);

        //row 0////////////////////////////////////////////////////////////
        gbc.fill = GridBagConstraints.BOTH;
        gbc.gridx = 10;
        gbc.gridy = 0;
        gbc.weightx = 0;
        gbc.weighty = 1;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        laborRecordNO.setHorizontalAlignment(JLabel.CENTER);
        laborRecordNO.setFont(new Font("Dialog", Font.PLAIN, 18));
        gridbag.setConstraints(laborRecordNO, gbc);
        rp.add(laborRecordNO, gbc);

        //row 1////////////////////////////////////////////////////////////
        gbc.gridx = 0;
        gbc.gridy = 1;
        gbc.gridwidth = 13;
        gridbag.setConstraints(logoLabel, gbc);
        rp.add(logoLabel, gbc);

        //row 2////////////////////////////////////////////////////////////
        gbc.gridx = 0;
        gbc.gridy = 2;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        nameOfJob.setFont(nameOfJob.getFont().deriveFont(18.0f));
        gridbag.setConstraints(nameOfJob, gbc);
        rp.add(nameOfJob, gbc);

        gbc.gridx = 5;
        gbc.gridy = 2;
        gbc.gridheight = 1;
        gbc.gridwidth = 10;
        contractNO.setFont(contractNO.getFont().deriveFont(18.0f));
        gridbag.setConstraints(contractNO, gbc);
        rp.add(contractNO, gbc);

        //row 3////////////////////////////////////////////////////////////
        gbc.gridx = 0;
        gbc.gridy = 3;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        customerPO.setFont(customerPO.getFont().deriveFont(18.0f));
        gridbag.setConstraints(customerPO, gbc);
        rp.add(customerPO, gbc);

        gbc.gridx = 5;
        gbc.gridy = 3;
        gbc.gridheight = 1;
        gbc.gridwidth = 10;
        weekEnding.setFont(weekEnding.getFont().deriveFont(18.0f));
        gridbag.setConstraints(weekEnding, gbc);
        rp.add(weekEnding, gbc);

        //row 4////////////////////////////////////////////////////////////
        gbc.gridx = 0;
        gbc.gridy = 4;
        gbc.gridheight = 3;
        gbc.gridwidth = 1;
        gbc.weightx = .01;
        personnelList.setHorizontalAlignment(JLabel.CENTER);
        personnelList.setBorder(BorderFactory.createLineBorder(Color.BLACK, 1));
        personnelList.setFont(personnelList.getFont().deriveFont(18.0f));
        gridbag.setConstraints(personnelList, gbc);
        rp.add(personnelList, gbc);

        gbc.gridx = 1;
        gbc.gridy = 4;
        gbc.gridheight = 3;
        gbc.gridwidth = 1;
        gbc.weightx = .42;
        personnelTitle.setHorizontalAlignment(JLabel.CENTER);
        personnelTitle.setBorder(BorderFactory.createMatteBorder(1, 0, 1, 0, Color.BLACK));
        personnelTitle.setFont(personnelTitle.getFont().deriveFont(18.0f));
        gridbag.setConstraints(personnelTitle, gbc);
        rp.add(personnelTitle, gbc);

        gbc.gridx = 2;
        gbc.gridy = 4;
        gbc.gridwidth = 8;
        gbc.gridheight = 1;
        gbc.weightx = .5;
        calendarTitle.setHorizontalAlignment(JLabel.CENTER);
        calendarTitle.setBorder(BorderFactory.createMatteBorder(1, 1, 1, 0, Color.BLACK));
        calendarTitle.setFont(calendarTitle.getFont().deriveFont(18.0f));
        gridbag.setConstraints(calendarTitle, gbc);
        rp.add(calendarTitle, gbc);

        gbc.gridx = 10;
        gbc.gridy = 4;
        gbc.gridheight = 3;
        gbc.gridwidth = 1;
        gbc.weightx = .05;
        totalHours.setHorizontalAlignment(JLabel.CENTER);
        totalHours.setBorder(BorderFactory.createMatteBorder(1, 1, 1, 1, Color.BLACK));
        totalHours.setFont(totalHours.getFont().deriveFont(18.0f));
        gridbag.setConstraints(totalHours, gbc);
        rp.add(totalHours, gbc);

        gbc.gridx = 11;
        gbc.gridy = 4;
        gbc.gridheight = 3;
        gbc.gridwidth = 1;
        gbc.weightx = .07;
        ratePerHour.setHorizontalAlignment(JLabel.CENTER);
        ratePerHour.setBorder(BorderFactory.createMatteBorder(1, 0, 1, 0, Color.BLACK));
        ratePerHour.setFont(ratePerHour.getFont().deriveFont(18.0f));
        gridbag.setConstraints(ratePerHour, gbc);
        rp.add(ratePerHour, gbc);

        gbc.gridx = 12;
        gbc.gridy = 4;
        gbc.gridheight = 3;
        gbc.gridwidth = 1;
        gbc.weightx = .07;
        totalAmount.setHorizontalAlignment(JLabel.CENTER);
        totalAmount.setBorder(BorderFactory.createMatteBorder(1, 1, 1, 1, Color.BLACK));
        totalAmount.setFont(totalAmount.getFont().deriveFont(18.0f));
        gridbag.setConstraints(totalAmount, gbc);
        rp.add(totalAmount, gbc);

        //row 5//////////////////////////////////////////////////////////////////
        gbc.gridx = 2;
        gbc.gridy = 5;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        gbc.weightx = .07;
        dayHeading.setHorizontalAlignment(JLabel.CENTER);
        dayHeading.setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
        dayHeading.setFont(dayHeading.getFont().deriveFont(18.0f));
        gridbag.setConstraints(dayHeading, gbc);
        rp.add(dayHeading, gbc);

        gbc.gridx = 3;
        gbc.gridy = 5;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        gbc.weightx = .07;
        mon.setHorizontalAlignment(JLabel.CENTER);
        mon.setBorder(BorderFactory.createMatteBorder(0, 0, 1, 0, Color.BLACK));
        mon.setFont(mon.getFont().deriveFont(18.0f));
        gridbag.setConstraints(mon, gbc);
        rp.add(mon, gbc);

        gbc.gridx = 4;
        gbc.gridy = 5;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        gbc.weightx = .07;
        tues.setHorizontalAlignment(JLabel.CENTER);
        tues.setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
        tues.setFont(tues.getFont().deriveFont(18.0f));
        gridbag.setConstraints(tues, gbc);
        rp.add(tues, gbc);

        gbc.gridx = 5;
        gbc.gridy = 5;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        gbc.weightx = .07;
        wed.setHorizontalAlignment(JLabel.CENTER);
        wed.setBorder(BorderFactory.createMatteBorder(0, 0, 1, 0, Color.BLACK));
        wed.setFont(wed.getFont().deriveFont(18.0f));
        gridbag.setConstraints(wed, gbc);
        rp.add(wed, gbc);

        gbc.gridx = 6;
        gbc.gridy = 5;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        gbc.weightx = .07;
        thur.setHorizontalAlignment(JLabel.CENTER);
        thur.setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
        thur.setFont(thur.getFont().deriveFont(18.0f));
        gridbag.setConstraints(thur, gbc);
        rp.add(thur, gbc);

        gbc.gridx = 7;
        gbc.gridy = 5;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        gbc.weightx = .09;
        fri.setHorizontalAlignment(JLabel.CENTER);
        fri.setBorder(BorderFactory.createMatteBorder(0, 0, 1, 0, Color.BLACK));
        fri.setFont(fri.getFont().deriveFont(18.0f));
        gridbag.setConstraints(fri, gbc);
        rp.add(fri, gbc);

        gbc.gridx = 8;
        gbc.gridy = 5;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        gbc.weightx = .07;
        sat.setHorizontalAlignment(JLabel.CENTER);
        sat.setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
        sat.setFont(sat.getFont().deriveFont(18.0f));
        gridbag.setConstraints(sat, gbc);
        rp.add(sat, gbc);

        gbc.gridx = 9;
        gbc.gridy = 5;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        gbc.weightx = .07;
        sun.setHorizontalAlignment(JLabel.CENTER);
        sun.setBorder(BorderFactory.createMatteBorder(0, 0, 1, 0, Color.BLACK));
        sun.setFont(sun.getFont().deriveFont(18.0f));
        gridbag.setConstraints(sun, gbc);
        rp.add(sun, gbc);

        //row 6//////////////////////////////////////////////////////////////
        gbc.gridx = 2;
        gbc.gridy = 6;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        gbc.weightx = 0;
        dateHeading.setHorizontalAlignment(JLabel.CENTER);
        dateHeading.setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
        dateHeading.setFont(dateHeading.getFont().deriveFont(18.0f));
        gridbag.setConstraints(dateHeading, gbc);
        rp.add(dateHeading, gbc);   

        int dateSpaceIndex = 3;
        boolean flip = true;
        for (int k = 0; k <= 6; k++)//create gridlines for day area
        {
            gbc.gridx = dateSpaceIndex;
            gbc.gridy = 6;
            gbc.gridheight = 1;
            gbc.gridwidth = 1;
            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));
            gridbag.setConstraints(dateSpace[k], gbc);
            rp.add(dateSpace[k], gbc);
            dateSpaceIndex++;
            flip = !flip;
        }



        //row 13-36 plus 7-12 time worked labels//////////////////////////
        for (int r = 0; r <= 9; r++)
        {
            gbc.gridx = 1;
            gbc.gridy = 7 + (3*r);
            gbc.gridheight = 3;
            gbc.gridwidth = 1;
            titleSpace[r].setHorizontalAlignment(JLabel.CENTER);
            titleSpace[r].setBorder(BorderFactory.createMatteBorder(0, 0, 1, 0, Color.BLACK));
            titleSpace[r].setFont(titleSpace[r].getFont().deriveFont(18.0f));
            gridbag.setConstraints(titleSpace[r], gbc);
            rp.add(titleSpace[r], gbc);
        }

        for (int c = 0; c <= 9; c++){
            gbc.gridx = 2;
            gbc.gridy = 7 + (3*c);
            gbc.gridheight = 1;
            gbc.gridwidth = 1;
            ot2[c].setHorizontalAlignment(JLabel.CENTER);
            ot2[c].setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
            ot2[c].setFont(ot2[c].getFont().deriveFont(18.0f));
            gridbag.setConstraints(ot2[c], gbc);
            rp.add(ot2[c], gbc);

            gbc.gridx = 2;
            gbc.gridy = 8 + (3*c);
            gbc.gridheight = 1;
            gbc.gridwidth = 1;
            ot1[c].setHorizontalAlignment(JLabel.CENTER);
            ot1[c].setOpaque(true);
            ot1[c].setBackground(shade);
            ot1[c].setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
            ot1[c].setFont(ot1[c].getFont().deriveFont(18.0f));
            gridbag.setConstraints(ot1[c], gbc);
            rp.add(ot1[c], gbc);

            gbc.gridx = 2;
            gbc.gridy = 9 + (3*c);
            gbc.gridheight = 1;
            gbc.gridwidth = 1;
            st[c].setHorizontalAlignment(JLabel.CENTER);
            st[c].setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
            st[c].setFont(st[c].getFont().deriveFont(18.0f));
            gridbag.setConstraints(st[c], gbc);
            rp.add(st[c], gbc);
        }
        //row 37/////////////////////////////////////////////////////////////////
        gbc.gridx = 0;
        gbc.gridy = 37;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        locAndDescriptionLabel.setFont(locAndDescriptionLabel.getFont().deriveFont(18.0f));
        gridbag.setConstraints(locAndDescriptionLabel, gbc);
        rp.add(locAndDescriptionLabel);

        gbc.gridx = 11;
        gbc.gridy = 37;
        gbc.gridheight = 1;
        gbc.gridwidth = 1;
        grandTotal.setHorizontalAlignment(JLabel.CENTER);
        grandTotal.setOpaque(true);
        grandTotal.setBackground(Color.BLACK);
        grandTotal.setForeground(Color.WHITE);
        grandTotal.setBorder(BorderFactory.createLineBorder(Color.BLACK,1));
        grandTotal.setFont(grandTotal.getFont().deriveFont(18.0f));
        gridbag.setConstraints(grandTotal, gbc);
        rp.add(grandTotal);

        gbc.gridx = 12;
        gbc.gridy = 37;
        gbc.gridwidth = 1;
        gbc.gridheight = 1;
        grandTotalSpace.setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
        gridbag.setConstraints(grandTotalSpace, gbc);
        rp.add(grandTotalSpace);

        //row 38////////////////////////////////////////////////////////////////
        gbc.gridx = 0;
        gbc.gridy = 38;
        gbc.gridwidth = 10;
        gbc.gridheight = 1;
        gbc.ipady = 80;
        locAndDescSpace.setBorder(BorderFactory.createLineBorder(Color.BLACK,1));
        gridbag.setConstraints(locAndDescSpace, gbc);
        rp.add(locAndDescSpace);

        //row 39////////////////////////////////////////////////////////////////
        gbc.ipady = 0; //reset to default
        gbc.gridx = 0;
        gbc.gridy = 39;
        gbc.gridheight = 1;
        gbc.gridwidth = 13;
        toCertify.setFont(toCertify.getFont().deriveFont(18.0f));
        gridbag.setConstraints(toCertify, gbc);
        rp.add(toCertify);

        //row 40///////////////////////////////////////////////////////////////
        gbc.gridx = 0;
        gbc.gridy = 40;
        gbc.gridheight = 1;
        gbc.gridwidth = 13;
        authorizedBy.setFont(authorizedBy.getFont().deriveFont(18.0f));
        gridbag.setConstraints(authorizedBy, gbc);
        rp.add(authorizedBy);


        for (int r = 0; r <= 9; r++)//gridlines for personnel space
        {
            gbc.gridx = 0;
            gbc.gridy = 7 + (3*r);
            gbc.gridheight = 3;
            gbc.gridwidth = 1;
            personnelSpace[r].setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
            gridbag.setConstraints(personnelSpace[r], gbc);
            rp.add(personnelSpace[r]);
        }
        //create calendar grid lines
        int yPointer = 7;
        int xPointer = 3;
        int shadePtr = 8;
        for (int j = 0; j <= 299; j++)
        {
            gbc.gridx = xPointer;
            gbc.gridy = yPointer;
            gbc.gridheight = 1;
            gbc.gridwidth = 1;
            calendarGridLines[j].setBorder(BorderFactory.createMatteBorder(0, 0, 1, 0, Color.BLACK));
            if (yPointer == shadePtr){ //if row number = shadePtr then color the cell
                calendarGridLines[j].setOpaque(true);
                calendarGridLines[j].setBackground(shade);
            }
            gridbag.setConstraints(calendarGridLines[j], gbc);
            rp.add(calendarGridLines[j]);
            xPointer++; //go to next cell in row
            j++; //use the next jlabel
            gbc.gridx = xPointer;
            gbc.gridy = yPointer;
            gbc.gridheight = 1;
            gbc.gridwidth = 1;
            calendarGridLines[j].setBorder(BorderFactory.createMatteBorder(0, 1, 1, 1, Color.BLACK));
            if (yPointer == shadePtr){ //if row number = shadePtr then color the cell
                calendarGridLines[j].setOpaque(true);
                calendarGridLines[j].setBackground(shade);
            }
            gridbag.setConstraints(calendarGridLines[j], gbc);
            rp.add(calendarGridLines[j]);

            xPointer++; //go to next cell in row

            if(xPointer == 13) //if end of column then go to next row and reset column pointer to 3 and increment shade pointer by 3
            {
                yPointer++; //go down a row
                xPointer = 3;
                if((j % 3) == 0) {
                    shadePtr = yPointer;
                }
            }
        }

    }
    @Override
    public void actionPerformed(ActionEvent e) {
        PrinterJob printJob = PrinterJob.getPrinterJob();
        printJob.setPrintable(this);
        if (printJob.printDialog()){
            try{
                printJob.print();
            }
            catch(Exception ex){
                throw new RuntimeException(ex);
            }
        }

    }
    @Override
    public int print(Graphics g, PageFormat pf, int index)
            throws PrinterException {
        Graphics2D g2 = (Graphics2D)g;
        if (index >= 1){
            return Printable.NO_SUCH_PAGE;
        }
        else {
            AffineTransform originalTransform = g2.getTransform();

            double scaleX = pf.getImageableWidth() / rp.getWidth();
            double scaleY = pf.getImageableHeight() / rp.getHeight();
            // Maintain aspect ratio
            double scale = Math.min(scaleX, scaleY);
            g2.translate(pf.getImageableX(), pf.getImageableY());
            g2.scale(scale, scale);
            rp.printAll(g2);

            g2.setTransform(originalTransform);
            return Printable.PAGE_EXISTS;
        }
    }

}

EDIT: Images of GUI may help. Top and bottom half respectively.

top

bottom

share|improve this question
    
Is there a reason for your constructor being the package visibility instead of public ? And this is not really a review but working with Swing there is so variable and lines of codes. –  Marc-Andre Feb 20 at 15:24
    
Thanks for your comment @Marc-Andre, however I'm a little confused. Either by miscommunication or my lack of Java understanding. Should LaborRecordPanel have a modifier? –  solleks Feb 20 at 15:32
    
It is not an obligation, if you don't specify public or private, a package level is automatically applied. But it just uncommon to see a real package level modifier that I was wondering if it's a real one or a public disguise in a package. –  Marc-Andre Feb 20 at 15:39
    
Please roll back your edit and add the updated code below the original. We do not want original code updated from answers; that invalidates them. If you'd like further review instead, post a new question. –  Jamal Feb 20 at 21:06
    
@Jamal, I tried to roll back my edits but it wasn't cooperating and my browser kept locking up. I apologize. I'll close this question and open another concerning the EDT –  solleks Feb 20 at 21:27
show 1 more comment

3 Answers

up vote 4 down vote accepted

METHOD EXTRACTION that's the key to fixing a lot of what you do....

Consider this code:

    //row 2////////////////////////////////////////////////////////////
    gbc.gridx = 0;
    gbc.gridy = 2;
    gbc.gridheight = 1;
    gbc.gridwidth = 1;
    nameOfJob.setFont(nameOfJob.getFont().deriveFont(18.0f));
    gridbag.setConstraints(nameOfJob, gbc);
    rp.add(nameOfJob, gbc);

    gbc.gridx = 5;
    gbc.gridy = 2;
    gbc.gridheight = 1;
    gbc.gridwidth = 10;
    contractNO.setFont(contractNO.getFont().deriveFont(18.0f));
    gridbag.setConstraints(contractNO, gbc);
    rp.add(contractNO, gbc);

What if you had a method:

public static addGridComponent(JComponent container, JComponent component, 
                 GridBagLayout gridbag, GridBagConstraints gbc,
                 int x, int y, int height, int width, float font) {
    gbc.gridx = x;
    gbc.gridy = y;
    gbc.gridheight = height;
    gbc.gridwidth = width;
    component.setFont(component.getFont().deriveFont(18.0f));
    gridbag.setConstraints(component, gbc);
    container.add(component, gbc);
}

Then your code would become:

//row 2////////////////////////////////////////////////////////////
addGridComponent(rp, nameOfJob,  gridbag, gbc, 0, 2, 1, 1,  18.0f);
addGridComponent(rp, contractNO, gridbag, gbc, 5, 2, 1, 10, 18.0f);

//row 3////////////////////////////////////////////////////////////
addGridComponent(rp, customerPO, gridbag, gbc, 0, 3, 1, 1,  18.0f);
addGridComponent(rp, weekEnding, gridbag, gbc, 5, 3, 1, 10, 18.0f)

This does not solve the problem with the magic numbers, but Marc-Andre has already addressed that. What it does solve is a huge amount of code repetition.

share|improve this answer
    
thanks @rolfl, that's very helpful. I'm in the process of refactoring my code according to your and Marc-Andre's answers. –  solleks Feb 20 at 18:17
    
@Marc-Andre good spotting. Thanks, Fixed –  rolfl Feb 21 at 14:46
add comment

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.

share|improve this answer
    
Thanks for the advice. I'm not exactly sure what you mean by "magic numbers". I think it would help to show my gui, I edited my post with the top and bottom half of the gui. After seeing this, do my numbers still seem arbitrary? If so, I will take action on it. –  solleks Feb 20 at 15:47
    
@solleks I've made an edit to better explain my point. –  Marc-Andre Feb 20 at 15:56
    
ah yes, your edit helped very much. I'll be making your suggested changes. If you choose to go into deeper review, I would very much appreciate any suggestions or comments regarding the usage of EDT and GridBagConstraints. –  solleks Feb 20 at 16:05
    
For the moment I can't elaborate much, but if I can be sure that I will do it! –  Marc-Andre Feb 20 at 16:07
add comment
  1. The variable declaration is unnecessary here:

    public void run() {
        final FrontierMain fm = new FrontierMain();
    }
    

    The following is the same:

    public void run() {
        new FrontierMain();
    }
    

    (Eclipse also puts a yellow exclamation mark on the line.)

  2. I think you are using EDT correctly. Just make sure that creation and every access (read or modify) to Swing components happens on EDT. Swing calls event handlers on EDT, including actionPerformed() and Printable.print().

    If you are not sure you can always check that a method runs on EDT or not with SwingUtilities.isEventDispatchThread(). You could create a custom method for that:

    public static void verifyEventDispatchThread() {
        if (!SwingUtilities.isEventDispatchThread()) {
            Thread.dumpStack();
        }
    }
    

    but there are other, already existing solutions: Swing verify code on event dispatch thread at runtime

share|improve this answer
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.