4
\$\begingroup\$

I have created a program that allows the user to view/add/delete employees and also view employee payslip, either weekly employee or monthly employee, which is all done in the console.

My programme runs fine and does as asked, but I feel it is not OOP. Can anyone suggest any changes to make it more OO?

Please see the 5 classes below that I use.

package payslips;

import java.util.*;

import payslips.Employee;
import payslips.Payslip;

public class MainProgramme 
{
    public static String name;
    public static String street;
    public static String town;
    public static String postcode;
    public static int payrollNo;
    public static char taxcode;
    public static String type;

    static Scanner sc = new Scanner(System.in);
    static Scanner sd = new Scanner(System.in);
    static int tempvar;
    static int temppayrollNo;

    static ArrayList<Employee> list = new ArrayList<Employee>();

    static String names[] = { "John Hepburn", "David Jones", "Louise White",
        "Harry Martin", "Christine Robertson" };

    static String streets[] = { "50 Granton Road", "121 Lochend Park",
            "100 Govan Avenue", "51 Gorgie Road", "1 Leith Street" };

    static String towns[] = { "Edinburgh", "Edinburgh", "Glasgow", "Edinburgh",
            "Edinburgh" };

    static String postcodes[] = { "EH6 7UT", "EH1 1BA", "G15 5GG", "EH5 2PR",
            "EH4 4ST" };

    static int payrollNos[] = { 10001, 10002, 10003, 10004, 10005 };

    static char taxcodes[] = { 'C', 'B', 'C', 'C', 'B' };

    static String types[] = { "Monthly", "Weekly", "Monthly", "Monthly","Weekly" };

    public static void main(String[] args) 
    {
        for (int i = 0; i < 5; i++) {
            name = names[i];
            street = streets[i];
            town = towns[i];
            postcode = postcodes[i];
            payrollNo = payrollNos[i];
            taxcode = taxcodes[i];
            type = types[i];
            Employee e = new Employee(name, street, town, postcode, payrollNo,taxcode, type);

            list.add(e);

        }

        // statements and prompts within the console for the user to follow
        System.out.println("Welcome to your Payroll System");
        System.out.println();
        System.out.println("Please enter your choice below from the following options");
        System.out.println();
        System.out.println("View all current weekly employees = 1 ");
        System.out.println("View all current monthly employees = 2 ");
        System.out.println("Delete an employee = 3 ");
        System.out.println("Add an employee = 4 ");
        System.out.println("Print an employee payslip = 5");
        System.out.println("To exit the system = 0 ");

        // allows user to enter number of choice and this reflects which statement is ran in userChoice method
        tempvar = sc.nextInt();

        // runs the userChoice method
        userChoice();
    }

    // method to determine what statement runs according to which choice user makes
    public static void userChoice() 
    {
        Employee tempEmployee = new Employee();
        boolean foundEmployee = false;

        // if user enters 1 it prints out the employee list.
        if (tempvar == 1) 
        {
            Weekly.printWeekly();

        } 
        else if (tempvar == 2) 
        {
            Monthly.printMonthly();

        } 
        else if (tempvar == 3) 
        { 
            printEmployeelist();
            System.out.println("");
            System.out.println("Above are a list of all employees.");
            System.out.println("Please enter the payroll number of the employee you wish to delete from the system");
            temppayrollNo = sc.nextInt();

            // while loop to search on payroll number, deletes the employee if correct, error message if not
            if (list.isEmpty() == false) 
            {
                int a = 0;
                while (a < list.size()) 
                {
                    tempEmployee = list.get(a);
                    if (tempEmployee.payrollNo == temppayrollNo) 
                    {
                        foundEmployee = true;
                        break;
                    }
                    a++;
                }
                if (foundEmployee == true) 
                {
                    System.out.println("You have deleted : "+ tempEmployee.getName());
                    System.out.println();
                    list.remove(tempEmployee);
                    printEmployeelist();
                } 
                else 
                {
                    System.out.println("The payroll number you have entered is incorrect");
                }
            }
        }

        else if (tempvar == 4) // allows the user to add an employee to the employee list, entering details using scanner
        {
            // initialises variables for entering title
            String tempstring1;
            int stringlength;
            int whitespace;
            String tempstring2;
            String tempstring3;
            // initialises variables for entering title
            String tempstring4;
            int stringlength2;
            int whitespace2;
            String tempstring5;
            String tempstring6;

            String tempstring7;

            System.out.println("You have chosen to add an employee to the system");
            System.out.println();

            // block of code that builds string together to get employee name
            System.out.println("Please enter the name of the new employee: ");
            tempstring1 = sd.nextLine(); // takes in string using scanner
            stringlength = tempstring1.length(); // saves length of string
            if (tempstring1.contains(" ")) // if statement to see if the string contains a space
            {
                whitespace = tempstring1.indexOf(" "); // finds the whitespace
                tempstring2 = tempstring1.substring((0), (whitespace));// creates string from start of input to whitespace
                tempstring3 = tempstring1.substring((whitespace) + 1,(stringlength));// creates string from whitespace plus one and adds on rest of the string
                tempEmployee.setName(tempstring2 + " " + tempstring3); // combines tempstring1 and tempstring2 together to complete full string
            } 
            else // else statement that just enters the string if it is just one word
            {
                tempEmployee.setName(tempstring1);
            }

            // block of code that repeats same process as above to get street name
            System.out.println("Please enter the address of the employee: ");
            tempstring4 = sd.nextLine();
            stringlength2 = tempstring4.length();
            if (tempstring4.contains(" ")) {
                whitespace2 = tempstring4.indexOf(" ");
                tempstring5 = tempstring4.substring((0), (whitespace2));
                tempstring6 = tempstring4.substring((whitespace2) + 1,(stringlength2));
                tempEmployee.setStreet(tempstring5 + " " + tempstring6);
            } 
            else 
            {
                tempEmployee.setStreet(tempstring4);
            }

            System.out.println("Please enter the town: ");
            tempEmployee.setTown(sd.nextLine());// takes in town using scanner
            System.out.println("Please enter the postcode: ");
            tempstring7 = sd.nextLine(); //post code using scanner

            if (tempstring7.length() > 5 && tempstring7.length() < 9) // sets the length of string

            {
                tempEmployee.setPostcode(tempstring7);
            } 
            else 
            {
                tempEmployee.setPostcode("You have not entered a valid UK postcode");
            }

            tempEmployee.setPayrollNo(payrollNo + 1); // sets payroll number to next in sequence

            System.out.println("Please enter your Tax code (A, B or C): ");
            tempEmployee.setTaxcode(sd.next().charAt(0));// takes in tax code using scanner

            System.out.println("Please enter Employee Type (Weekly or Monthly): ");
            tempEmployee.setType(sd.next()); //takes in type of employee

            list.add(tempEmployee);// creates temp employee and adds to list
            printEmployeelist();// prints employee list to view
        }

        else if (tempvar == 5) 
        {
            Payslip.Payslips(); //runs payslip method from payslip class
        }

        else if (tempvar == 0) // if user hits 0 it allows them to exit the programme

        {
            System.out.println("You have exited the system");
            System.exit(0);
        }

        else // if any other choice entered they will be met with this message
        {
            System.out.println("You have entered the wrong choice");
        }
    }

    // method to create the book list using a for loop
        public static void printEmployeelist() {
        for (int i = 0; i < list.size(); i++)
            System.out.println(list.get(i));
    }
}

package payslips;

    import java.util.Scanner;

    public class Payslip extends MainProgramme

{
    static int tempSalary;
    static double tempHours;
    static Scanner ss = new Scanner(System.in);

    public static void Payslips() 
    {

        {
            Employee tempEmployee = new Employee();
            boolean foundEmployee = false;

            {
                System.out.println("Please enter the employee payroll number to view payslip");
                temppayrollNo = sc.nextInt();

                if (list.isEmpty() == false) 
                {
                    int a = 0;
                    while (a < list.size()) 
                    {
                        tempEmployee = list.get(a);
                        if (tempEmployee.payrollNo == temppayrollNo) 
                        {
                            foundEmployee = true;
                            break;
                        }
                        a++;
                    }
                    if (foundEmployee == true) 
                    {

                        tempEmployee.getType();
                        if (tempEmployee.type == "Weekly") 
                        {
                            System.out.println("Please enter hours worked this week: ");
                            tempHours = ss.nextDouble();
                            System.out.println();
                            System.out.println("PAYSLIP");
                            System.out.println("Week No:"+ (int) (Math.random() * 52));
                            System.out.println("Name: "+ tempEmployee.getName());
                            System.out.println("Payroll No: "+ tempEmployee.getPayrollNo());
                            System.out.println("Address: "+ tempEmployee.getStreet() + ", "+ tempEmployee.getTown() + ", "+ tempEmployee.getPostcode());
                            System.out.println("Tax Code: "+ tempEmployee.getTaxcode());
                            System.out.println("Weekly Pay: £"+ (tempHours * 8.50));
                        }

                        else 
                        {
                            System.out.println("Please Enter Salary (£): ");
                            tempSalary = ss.nextInt();
                            System.out.println();
                            System.out.println("PAYSLIP");
                            System.out.println("Month No:"+ (int) (Math.random() * 12));
                            System.out.println("Name: "+ tempEmployee.getName());
                            System.out.println("Payroll No: "+ tempEmployee.getPayrollNo());
                            System.out.println("Address: "+ tempEmployee.getStreet() + ", "+ tempEmployee.getTown() + ", "+ tempEmployee.getPostcode());
                            System.out.println("Tax Code: "+ tempEmployee.getTaxcode());
                            System.out.println("Salary : £" + tempSalary + " Monthly Pay: £"+ (tempSalary / 12));
                        }
                    } 
                    else 
                    {
                        System.out.println("The payroll number you have entered is incorrect");
                    }
                }
            }
        }
    }
}

package payslips;

import payslips.MainProgramme;


public class Employee extends MainProgramme 

{


    public Employee() 
    {

    }
    //initialises variables
    public String name;
    public String street;
    public String town;
    public String postcode;
    public int payrollNo;
    public char taxcode;
    public String type;



    public Employee(String name, String street, String town, String postcode, int payrollNo, char taxcode, String type) 
    {
        //sets constructors
        this.name = name;
        this.street = street;
        this.town = town;
        this.postcode = postcode;
        this.payrollNo = payrollNo;
        this.taxcode = taxcode;
        this.type = type;

    }

    //sets the getters and setters
    public String getName() 
    {
        return name;
    }

    public void setName(String name) 
    {
        this.name = name;
    }

    public String getStreet()
    {
        return street;
    }

    public void setStreet(String street) 
    {
        this.street = street;
    }

    public String getTown() 
    {
        return town;
    }

    public void setTown(String town) 
    {
        this.town = town;
    }

    public String getPostcode() 
    {
        return postcode;
    }

    public void setPostcode(String postcode) 
    {
        this.postcode = postcode;
    }

    public int getPayrollNo()
    {
        return payrollNo;
    }
    public void setPayrollNo(int payrollNo)
    {
        this.payrollNo = payrollNo;
    }

    public char getTaxcode()
    {
        return taxcode;
    }
    public void setTaxcode(char taxcode)
    {
        this.taxcode = taxcode;
    }

    public String getType()
    {
        return type;
    }
    public void setType(String type)
    {
        this.type = type;
    }

    //when printing employee list method it sets the input to display as below
    public String toString() 
    {
        return "Name: " + this.name + " / "  + "Street: " + this.street + " / "

        + "Town: " + this.town + " / " + "Postcode: " + this.postcode + " / " + "Payroll Number: " + this.payrollNo
        + " / " + "Tax Code: " + this.taxcode + " / " + "Type: " + this.type;
    }

}

package payslips;

public class Monthly extends MainProgramme

{
    public static void printMonthly() 
    {
        for (int i = 0; i < list.size(); i++)
            if (types[i] == "Monthly") 
        {
        System.out.println(list.get(i));
        }
    }
}

package payslips;

public class Weekly extends MainProgramme

{
    public static void printWeekly() 
    {
        for (int i = 0; i < list.size(); i++)
            if (types[i] == "Weekly") 
        {
        System.out.println(list.get(i));
        }
    }
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ As a start: You have the hierarchy reversed. Normally the main application depends on the Objects...in your case the objects depend on the main application. Every Object should be able to stand on their own. \$\endgroup\$
    – Bobby
    Oct 24 '13 at 10:57
  • \$\begingroup\$ @Bobby what is the best way to start going about changing this? \$\endgroup\$
    – Kieran
    Oct 24 '13 at 11:31
  • 1
    \$\begingroup\$ Start by zero, maybe even with a diagram showing what you need and what not. Design the classes Employee/Payslip as completely independent, which only interact via functions with the outside world (no calling of System.out too!). Also another rule of thumb: If you need to suffix your variables with numbers because the name is duplicate, you're doing something wrong. Also sorry, but this would need a complete rewrite so I won't do a review here. Maybe you should buy yourself a good book like "Effective Java", that would clear a lot up and would explain you the basics better then I could. \$\endgroup\$
    – Bobby
    Oct 24 '13 at 12:41
12
\$\begingroup\$

Before we talk about making this program object-oriented, we have more basic issues to cover.

Basic (Java) Programming

Scoping

Each variable has a scope where it is visible. In Java, variables have block scope, that is, their visibility begins with their declaration and ends with the closing brace } for the current block.

int x = 0; // declaration of "x"
if (x < 4) {  // we enter a block
  int y = 2; // declaration of "y"
  ...
} // the scope of "y" ends here
// x is still visible here

So when we need temp variables, we can declare them in the inner scope where they are needed. The fewer variables in an outer scope we have, the easier a program is to understand.

You should avoid variables that are public, static, or in outer scopes, because these are in a sense global variables. There are often better ways to access your data, e.g. through variables in tight scope, or through getter methods.

Variable naming

A variable name should make it obvious what this variable does. For example, you have two variables sc and sd. These are bot scanners around the System.in stream. You never explain what their difference is. Good variable names don't tell use what their content is (we can already see the type at the variable declaration), but rather what its purpose is. E.g. input might be better than sc. What is list? Yes, I can see that it's a List, thankyou very much. Oh, it's a collection of all employess? → employees.

A few lines later, you have tempstring1 through tempstring7. It is impossible to know from these names what they are supposed to do. It seems tempstring1 should be name and tempstring4 should be address, but it is difficult to understand the relevant code because it is obscured with these unintelligible variable names.

Use better names! They cost you nothing, and make code much easier to read. And above all, avoid single-letter names.

No Magic Numbers, and Bad Assumptions

A magic number is a number that occurs in the source code without explanation why this specific number is needed here. Numbers like 0 or 1 are often obvious, but anything else should be explained.

The problem is that these numbers often contain assumptions about your data that can be invalidated without you realizing it. When the data changes, and your program isn't updated accordingly, then you get bugs that are difficult to track down. For example, in your `main you have this loop:

for (int i = 0; i < 5; i++) ...

The number 5 is a magic number here. Where does it come from? Ah, of course it's from your arrays like names. Using names.length here would already be much better.

If you make certain assumptions you can also test them for validity with asserts. An assert is an expression that expresses a state which you as a programmer know to be true, but you want to make sure. Here, an assertion that all of your input arrays have the same length seems like a good idea:

assert names.length == streets.length;
assert names.length == ...;
...

Putting a Few Pieces Together

We can now refactor your loop there to use better variable scoping etc:

assert names.length == streets.length;
assert names.length == ...;
...

for (int i = 0; i < names.length; i++) {
    // declare these vars in this scope only – not needed on the outside
    String name      = names[i];
    String street    = streets[i];
    String town      = towns[i];
    String postcode  = postcodes[i];
    int    payrollNo = payrollNos[i];
    char   taxcode   = taxcodes[i];
    String type      = types[i];

    Employee employee = new Employee(name, street, town, postcode, payrollNo, taxcode, type);
    employees.add(employee);
}

Wow, is that a lot code for so little. Instead of spreading your data of one employee across several arrays and then later building it, you might as well construct them directly:

static ArrayList<Employee> employees = new ArrayList<Employee>();

// this is called a static initializer block
// it runs before `main` is executed
static {
  employees.add(new Employee("John Hepburn", "50 Granton Road",  "Edinburgh", "EH6 7UT", 10001, 'C', "Monthly"));
  employees.add(new Employee("David Jones",  "121 Lochend Park", "Edinburgh", "EH1 1BA", 10002, 'B', "Weekly"));
  ...
}

Hey, that is not only shorter, this is easier to read as well

Foreach-loops

When iterating over each element of a Java collection, using .get(...) calls feels natural at first, but there is something better. Assume code like this:

int temppayrollNo = ...;
boolean foundEmployee = false;
int a = 0;
while (a < list.size()) {
    tempEmployee = list.get(a);
    if (tempEmployee.payrollNo == temppayrollNo) {
        foundEmployee = true;
        break;
    }
    a++;
}

First of all, this is a normal for-loop which you just expanded to the more low-level while. Below I also clear up variable names:

int wantedPayrollNo = ...;
Employee foundEmployee = null;
for (int i = 0; i < employees.size(); i++) {
    Employee employee = employees.get(a);
    if (employee.payrollNo == wantedPayrollNo) {
        foundEmployee = employee;
        break;
    }
}

if (foundEmployee != null) ...

We can use a for-each loop for any collection that implements the Iterable interface, which any List does. Therefore:

int wantedPayrollNo = ...;
Employee foundEmployee = null;
for (Employee employee : employees) {
    if (employee.payrollNo == wantedPayrollNo) {
        foundEmployee = employee;
        break;
    }
}

...

This abstracts over actually accessing each element, and is more general. Use modern Java features like this to make your code easier.

Choosing a good data structure

Above, we iterated through all employees and stopped when we found the correct one. For that, we were comparing a certain index (here: the payroll number) for equality. There are data structures that can do this much more efficiently. In Java, these implement the Map interface. To index the employees by their payroll number, we do something like:

// do this once, on startup
Map<Integer, Employee> employeesByPayrollNo = new HashMap<Integer, Employee>();
for (Employee employee : employees) {
    employeesByPayrollNo.put(employee.payrollNo, employee);
}

Then, we can just look up an entry like Employee foundEmployee = employeesByPayrollNo.get(wantedPayrollNo).

Don't repeat yourself

Parts of your code have massive repetition, for example when print out an report about your employee. Many of the lines are independent from the payment frequency. You could also have written:

boolean isWeekly = tempEmployee.type == "Weekly";
double pay = 0;
double hourlyPay = 8.50;

if (isWeekly) {
    System.out.println("Please enter hours worked this week: ");
    pay = hourlyPay * ss.nextDouble();
}
else {
    System.out.println("Please Enter Salary (£): ");
    pay = ss.nextInt();
}

System.out.println();
System.out.println("PAYSLIP");
if (isWeekly) System.out.println("Week No:"  + (int) (Math.random() * 52));
else          System.out.println("Month No:" + (int) (Math.random() * 12));
System.out.println("Name: "+ tempEmployee.getName());
System.out.println("Payroll No: "+ tempEmployee.getPayrollNo());
System.out.println("Address: "+ tempEmployee.getStreet() + ", "+ tempEmployee.getTown() + ", "+ tempEmployee.getPostcode());
System.out.println("Tax Code: "+ tempEmployee.getTaxcode());
if (isWeekly) System.out.println("Weekly Pay: £"+ pay);
else          System.out.println("Salary : £" + pay + " Monthly Pay: £" + (pay / 12));
}

Of course, such functionality shouldn't be hardcoded into your program, but you should rather be using a templating system where you only do template.render(isWeekly, pay) or something.

Aspects not covered

  • Redundant code (all this substringing nonsense when parsing a name)
  • Single-Responsibility principle (Address should be its own class)
  • Enums instead of strings to ensure correctness at compile time (what happens if the type of an employee is "weekly" or Wekly? That shouldn't be possible)
  • When (not) to use inheritance
  • Early returns to avoid deeply nested ifs
  • Finding abstractions and refactoring into helper methods
  • Abstracting away from text-based interfaces: System.out.println(...) should be a very rare occurrence.
\$\endgroup\$
2
  • \$\begingroup\$ thank you for such a detailed response, a lot seems very very useful. Hopefully I can go away and use this. Really hoping I can get the OOP sorted as i seem to just run everything through the main rather than use my classes correctly, any wee tips on how to move my code into the right classes? \$\endgroup\$
    – Kieran
    Oct 24 '13 at 13:32
  • \$\begingroup\$ @Kieran OOP is no end in itself. It is an useful abstraction mechanism, along with “procedural programming” (abstracting stuff into helper functions). Start with cleaning up your code, then read a respected Java book, which will teach you the basics of object orientation. While OOP is in no way complicated, you have more urgent things to learn before that. \$\endgroup\$
    – amon
    Oct 24 '13 at 13:46
4
\$\begingroup\$

There is much to be gained by refactoring this code into an object-oriented style. However, before we proceed, I would like to point out that it's not good procedural-style code to begin with.

You've marked every class as … extends MainProgramme, which is nonsense. You wouldn't say that an employee is a main programme, so you shouldn't say that in your code either. Your employee code resides in the payslips package. Having package payslips; at the top of each file is all you need.

In MainProgramme, you are using class variables in many places where a local variable should be used. Remember, using a class variable is just about as bad as a global variable (well, slightly better than a global in the sense that it is in a namespace). In fact, if you want good code, get rid of all static variables and functions in your code unless you know exactly what the justification is.


Well, let's think about what kinds of objects there ought to be in this system. Obviously, there an Employee class. (Your existing Employee class is perfectly fine, except that its instance variables should all be private.) Employees belong on a payroll, so there should be a Payroll class. (You could also call it a Company instead.)

import java.util.*;

public class Payroll implements Iterable<Employee> {
    private List<Employee> employees;
    private int maxPayrollNo = Integer.MIN_VALUE;

    public Payroll(Employee[] employees) {
        // LinkedList supports .remove(), but ArrayList doesn't
        this.employees = new LinkedList<Employee>(Arrays.asList(employees));
        for (Employee e : employees) {
            if (e.getPayrollNo() > this.maxPayrollNo) {
                this.maxPayrollNo = e.getPayrollNo();
            }
        }
    }

    @Override
    public Iterator<Employee> iterator() {
        return this.employees.iterator();
    }

    public void add(Employee e) {
        this.employees.add(e);
    }

    public int nextPayrollNo() {
        return ++this.maxPayrollNo;
    }

    @Override
    public String toString() {
        StringBuilder s = new StringBuilder();
        for (Employee e : this) {
            s.append(e).append('\n');
        }
        return s.toString();
    }
}

Three things to note in the code above.

  1. This is the logical place to put the .nextPayrollNo() method, and it demonstrates that a bare List is insufficient to model the payroll.

  2. The way in which the collection of employees is stored is hidden from everything else. I've chosen to use a LinkedList now, but in the future it may change to a HashSet or something, and the interface could remain the same. You could also implement a .findByPayrollNo(int) and a .remove(Employee) method as well, but I've chosen not to bother.

  3. Your MainProgramme.printEmployeeList() was just a stray chunk of code without a home. I've moved it here, where it logically belongs, so you can call payroll.toString().


What may not be so obvious is that the main programme itself could benefit from object-oriented reorganization. Your .userChoice() function could be slightly better written as a giant switch block. A giant switch block, in turn, is an antipattern, or Bad Code Smell, in object-oriented programming, the remedy for which is polymorphism.

What does that mean, in this example? Your switch decides which action to perform. Therefore, you want to define different kinds of "Action" objects. Specifically, what these actions all have in common is that they operate on a Payroll, so let's call these actions PayrollActions.

public interface PayrollAction {
    /**
     * The name of the action.
     */
    String toString();

    /**
     * Code to perform the action.
     */
    void perform(Payroll p);
}

Let's skip ahead a bit. Where do I want to go with this? My goal is to simplify MainProgramme to this:

import java.util.Scanner;

public class MainProgramme
{
    private static Employee[] EMPLOYEES = {
        new Employee("John Hepburn", "50 Granton Road", "Edinburgh", "EH6 7UT", 10001, 'C', "Monthly"),
        new Employee("David Jones", "121 Lochend Park", "Edinburgh", "EH1 1BA", 10002, 'B', "Weekly"),
        new Employee("Louise White", "100 Govan Avenue", "Glasgow", "G15 5GG", 10003, 'C', "Monthly"),
        new Employee("Harry Martin", "51 Gorgie Road", "Edinburgh", "EH5 2PR", 10004, 'C', "Monthly"),
        new Employee("Christine Robertson", "1 Leith Street", "Edinburgh", "EH4 4ST", 10005, 'B', "Weekly"),
    };

    private static PayrollAction[] ACTIONS = {
        new Exit(),
        new FilteredPrint(EmployeeTypeFilter.WEEKLY),
        new FilteredPrint(EmployeeTypeFilter.MONTHLY),
        new EmployeeDelete(),
        new EmployeeAdd(),
        new PayslipPrint(),
    };

    public static void main(String[] args)
    {
        Payroll payroll = new Payroll(EMPLOYEES);

        // Main menu
        System.out.println("Welcome to your Payroll System");
        System.out.println();
        System.out.println("Please enter your choice below from the following options");
        System.out.println();

        // List the actions, with "Exit = 0" last
        for (int i = 1; i < ACTIONS.length; i++) {
            System.out.format("%s = %d\n", ACTIONS[i], i);
        }
        System.out.format("%s = %d\n", ACTIONS[0], 0);

        // User to enter number of choice
        Scanner sc = new Scanner(System.in);

        PayrollAction action;
        try {
            action = ACTIONS[sc.nextInt()];
        } catch (ArrayIndexOutOfBoundsException noSuchAction) {
            System.out.println("You have entered the wrong choice");
            return;
        }
        action.perform(payroll);
    }
}

Much better, isn't it? Please take a long break now and see if you can reorganize the code to make it work. I'll include some spoilers below.


We need a general way to select which employee(s) to operate on. We can accomplish that using "filter" objects.

public interface EmployeeFilter {
    boolean accepts(Employee e);
}

Specifically, we need to support your weekly and monthly employee filters. We can create those two filters as singletons. (Singletons are are justified use of static members.)

public class EmployeeTypeFilter implements EmployeeFilter {
    private final String employeeType;

    private EmployeeTypeFilter(String employeeType) {
        this.employeeType = employeeType;
    }

    public static EmployeeTypeFilter MONTHLY = new EmployeeTypeFilter("Monthly"),
                                     WEEKLY  = new EmployeeTypeFilter("Weekly");

    @Override
    public boolean accepts(Employee e) {
        return this.employeeType.equals(e.getType());
    }

    @Override
    public String toString() {
        return this.employeeType.toLowerCase() + " employees";
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.