Managing employee information

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 list = new ArrayList();

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 && 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 +

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 employees = new ArrayList();

// 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++) {

StackExchange Code Review Q#33167, answer score: 12