Program for a packing company

Question

It's my first year in college studying Java so please excuse my ignorance in programming. This is my 5th Task in my Sequential Programming assignment, where we had to design a program for a packing company to place orders and print a receipt. Could you please give me any advice you may have on my code?

package Lab;

import java.util.ArrayList;
import javax.swing.JFrame;
import java.awt.*;
import javax.swing.*;
import java.text.DecimalFormat;
import javax.swing.UIManager.*;

public class Taskk5 extends JFrame {

    private static String seller, qStr;
    private static int quantity = 0, ppu = 0, bdiscount = 0, discount = 0, cinput, input;
    int price = quantity * ppu, TotalCost = price - (price / 100 * discount);

    //============================================== instance variables
    JTextArea print = new JTextArea(10, 40);
    JTextArea nopurchase = new JTextArea(10, 10);

    //====================================================== constructor
    public Taskk5() {
        DecimalFormat d = new DecimalFormat("'£'0.00");
        DecimalFormat p = new DecimalFormat("0'%'");

        //... Set textarea's initial text, scrolling, and border.
        print.setText("Type of Seller\tQuantity \tPrice per Unit\n"
                + "----------------------------------"
                + "--------------------------------\n"
                + seller + "\t   " + quantity + " \t    " + ppu
                + "\n--------------------------------"
                + "----------------------------------"
                + "\nSpecial Customer Discount: " + p.format(discount)
                + "\n--------------------------------"
                + "----------------------------------\n"
                + "\nTotal Cost: " + d.format(TotalCost));

        JScrollPane scrollingArea = new JScrollPane(print);

        //... Get the content pane, set layout, add to center
        JPanel content = new JPanel();
        content.setLayout(new BorderLayout());
        content.add(scrollingArea, BorderLayout.CENTER);

        //... Set window characteristics.
        this.setContentPane(content);
        this.setTitle("Print");
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        this.pack();
    }

    public static void main(String[] args) {

        JFrame frame = new JFrame();
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

        Object rowData[][] = {
            {"No. Units", "Price per unit £", "No. Units", "Price per unit £"},
            {"1- 6", "50", "1-4", "60"},
            {"7-11", "40", "5-9", "50"},
            {"12-21", "30", "10-15", "40"},
            {"22-60", "20", "16-50", "30"},
            {"Over 60", "15", "Over 50", "25"}};
        Object columnNames[] = {"Wholesalers", "", "Retailers", ""};
        JTable table = new JTable(rowData, columnNames);

        JScrollPane scrollPane = new JScrollPane(table);
        frame.add(scrollPane, BorderLayout.CENTER);
        frame.setSize(370, 160);
        frame.setVisible(true);

        try {
            for (LookAndFeelInfo info : UIManager.getInstalledLookAndFeels()) {
                if ("Nimbus".equals(info.getName())) {
                    UIManager.setLookAndFeel(info.getClassName());
                    break;
                }
            }
        } catch (Exception e) {
    // If Nimbus is not available, you can set the GUI to another look and feel.
        }
        ArrayList<String> MenuList = new ArrayList<String>();
        MenuList.add("Wholesaler");
        MenuList.add("Retailer");

        Object[] options = MenuList.toArray();

        ArrayList<String> CustomerList = new ArrayList<String>();
        CustomerList.add("Special Customer");
        CustomerList.add("Normal Customer");

        Object[] customer = CustomerList.toArray();

        do {
// Output message for the user to select which unit they wish to convert
// The input variable is the result of the optiondialog, this allows Java to identify which option the user has selected
            int input = JOptionPane.showOptionDialog(null,
                    "Which kind of distributor are you?", "Packing Company Orders",
                    JOptionPane.YES_OPTION, JOptionPane.NO_OPTION,
                    null, options, MenuList.get(1));

            if (input == JOptionPane.CLOSED_OPTION) {
                System.exit(0);
            }

// This is the if statement for the first option, which is "Fahrenheit to Celsius"
            try {

                if (input == 0) {

                    seller = "Wholesaler";
                    qStr = JOptionPane.showInputDialog(null, "Please enter the number of units you require");
                    quantity = Integer.parseInt(qStr);

                    if (quantity >= 1 && quantity <= 6) {

                        ppu = 50;

                    } else if (quantity >= 7 && quantity <= 11) {

                        ppu = 40;

                    } else if (quantity >= 12 && quantity <= 20) {

                        ppu = 30;

                    } else if (quantity >= 21 && quantity <= 60) {

                        ppu = 20;

                    } else if (quantity > 60) {

                        ppu = 15;

                    }
                    cinput = JOptionPane.showOptionDialog(null,
                            "Which kind of Customer are you?", "Packing Company Orders",
                            JOptionPane.YES_OPTION, JOptionPane.NO_OPTION,
                            null, customer, CustomerList.get(0));

                    if (cinput == JOptionPane.YES_OPTION) {
                        discount = 10;

                    }

                    JFrame win = new Taskk5();
                    win.setVisible(true);
                } else if (input == 1) {
                    seller = "Retailer";
                    qStr = JOptionPane.showInputDialog(null, "Please enter the number of units you require");
                    if (qStr == null) {
                        System.exit(0);
                    }
                    quantity = Integer.parseInt(qStr);

                    if (quantity >= 1 && quantity <= 4) {
                        ppu = 60;
                        ;
                    } else if (quantity >= 5 && quantity <= 9) {
                        ppu = 50;
                        ;
                    } else if (quantity >= 10 && quantity <= 15) {
                        ppu = 40;
                        ;
                    } else if (quantity >= 16 && quantity <= 50) {
                        ppu = 30;
                        ;
                    } else if (quantity > 50) {
                        ppu = 25;
                        ;
                    }

                    cinput = JOptionPane.showOptionDialog(null,
                            "Which kind of Customer are you?", "Packing Company Orders",
                            JOptionPane.YES_OPTION, JOptionPane.NO_OPTION,
                            null, customer, CustomerList.get(0));

                    if (cinput == JOptionPane.YES_OPTION) {
                        discount = 10;

                    }

                    JFrame win = new Taskk5();
                    win.setVisible(true);
                }
            } catch (NumberFormatException e) {
                //... Output message for empty input box
                JOptionPane.showMessageDialog(null, "Please input a value before pressing enter");
            }
        } while (input != JOptionPane.CLOSED_OPTION);

    }
}

Answer 1

By no means an exhaustive review, but here are a few general pointers:

1. Break up the code responsibilities into methods.
   • This not only makes your code more readable, but much easier to test and debug!
2. Use variable names that have meaning to people other than yourself.
   • This makes it easier for you to understand your code if you should ever need to come back to it later. It also makes it easier to get help (like now!)
   • eg. static variable qStr on line 12, d and p on lines 22 and 23.
3. Don't use Object as the type if you don't need to. You know these are going to be String arrays.
4. Magic numbers = bad
5. I know that you're not using float or double values anywhere, but since you're dealing with money it made me think of it as something you can just keep in the back of your mind for the future: https://stackoverflow.com/questions/3730019/why-not-use-double-or-float-to-represent-currency
6. I know it can be tempting, but try not to make all your variables private static... if they don't need to be! It definitely makes things convenient when you're new to programming, but it can get you in a lot of trouble down the road. https://stackoverflow.com/questions/7026507/why-are-static-variables-considered-evil
7. Miscellaneous:
   • Extra ; on 157, 160, etc...
   • bdiscount is never used
   • Line 105 has a comment about Fahrenheit and Celsius :)

I have very little experience with Swing, so I can't really comment on that. Also, sorry if my line numbers are off, I just copied and pasted pretty quickly... Hope this helps!

Answer 2

Import only what you need

If you find yourself using a lot of a library's classes or are dealing with others who are familiar with it, you may opt to use the wildcard. I personally prefer importing individual, no matter how numerous the calls, but whichever you choose, don't be redundant.

i.e when you're importing javax.swing.*; it's unnecessary to also import javax.swing.JFrame; Do note, however, that importing javax.swing.UIManager.* is still correct because you want all UIManager's subclasses, which wouldn't be included in the initial wildcard import.

Keep variables camelCase & use more meaningful names

For the most part, you do a decent job of this, but there are some cases that stand out, like CustomerList, which should be customerList, and cinput which should be cInput, and should have a better name like customerInput, ask yourself what do you lose by calling something like cinput with a more expressive name, albeit slightly longer, weigh that against people reading this code, even yourself after a time, and wonder just what in the word is a cinput?

Consider adding an option to continue rather than looping

Right now, although it works and that's great, it's not intuitive and simply catches a user by surprise when it continues again.

For a parallel to illustrate my point imagine if you somehow made a calculator program with three separate OptionPanes, one which asks for first input, a second for additional input, a third which asked if you needed additional input and another which finally asked which operation you wanted to apply to the input.

Imagine using this program once, and finding out you can't close it till you go through all those popups.

But my calculator example was also meant to point out an even more obvious improvement.

Don't excessively use JOPtionPane

They are pop-ups which you should save for when you really need to grasp the user's attention, like a sudden error message, or when their particular input is important.

Not to mention since you're only presented a binary choice a much better implementation would be to make use of JComboBox and have the user choose the one that's appropriate for their needs.

Use your favorite horizontal layout manager, place the two Comboboxes for your current 'pop-ups' and have a textfield for the input with a button that could calculate and display the result.

Improving succinctness & checking fail conditions first

Why do you include extraneous semi-colons? Remove them.

Your if blocks suffer a lot from this, and outside of that issue they merit improvement themselves, in addition to checking if a quantity is <= x, you have to ensure that is >= z, this is tedious. It is the best practice to handle unwanted conditions and error checks first, if you instead ensure that the input is positive e.g.

if (quantity < 1) {
// tell user that quantity need be positive
}

Then all you proceeding if blocks only have one conditional check, this is more readable, writable, and increases performance.

If you added my fail-first check, you could continue the rest of the conditional blocks like so:

else if (quantity <= 6) {
  ppu = 50;
} else if (<= 11) {
  ppu = 40;
} else if (quantity <= 20) {
 ppu = 30;
}