Error in using instanceOf operator in Java

Question

Not sure what is going wrong on line 24. I'm trying to override the Object.equals() method, but since that method takes type Object, I want to filter out inputs that are not an instance of Employee. This is for an intro java course

I'm getting these errors:

4 errors found:
File: /Users/wesley/Documents/Employee.java  [line: 24]
Error: ')' expected
File: /Users/wesley/Documents/Employee.java  [line: 24]
Error: variable declaration not allowed here
File: /Users/wesley/Documents/Employee.java  [line: 24]
Error: ';' expected
File: /Users/wesley/Documents/Employee.java  [line: 30]
Error: 'else' without 'if'

Here's the code.

//employee number, name, and salary
public class Employee{
  private String name;
  private final int ID;
  private double salary;
  private static int lastID;
  public Employee(String name, double salary){
    this.name=name;
    ID=Employee.lastID+1;
    this.salary=salary;
    Employee.lastID=Employee.lastID+1;
  } 
  public String getName(){return name;}
  public int getID(){ return ID; }
  public double getSalary(){ return salary;}
  public void setName(String newName){name=newName;}
  public void raise(double newSalary){ salary=newSalary;}
  @Override
  public String toString(){
    return "Name: "+ getName() + "\nID: " + getID() + "\nSalary: " + getSalary();
  }
  @Override
  public boolean equals(Object o){
    if(o instanceOf Employee){//errors are here!
    Employee input=(Employee) o;
    if(this.getSalary()==input.getSalary() && this.getName().equals(input.getName())){
      return true;
    }
  }
    else{return false;
    }//another error here
      }
    }

Can't be bothered to fix the indentation. Just happened when I pasted stuff in

Answer 1

There is a number of things that you can improve.

First, the instanceof keyword is lowercase.

Second, if you override equals(Object), you MUST also override hashCode(), as mentioned in this answer. This answer also contains excellent examples of how to implement both equals and hashCode.

Third, your implementation allows name to be null, in which case this.getName() is null and your check

this.getName().equals(input.getName()) 

throws a NullPointerException. I suggest you follow the more robust practice in the answer or this tutorial, or make sure that name can never be null, by checking it in the constructor and setName.

Additional improvements and remarks

If there is a field ID, I assume that it is there to identify the employee. In that case, it must be included in the equals check.

If there is an 'ID`, why also check the name and the salary? Does changing the salary also mean that it becomes a different Employee? Same if I correct the name of the Employee?

The initialisation of ID is not thread-safe, and can lead to different Employees having the same ID. Consider doing:

import java.util.concurrent.atomic.AtomicInteger;
///
private static AtomicInteger lastID = new AtomicInteger();

And in the constructor just do

this.ID = lastID.getAndIncrement();

which makes the following line unnecessary:

Employee.lastID=Employee.lastID+1;

Another thing is that the method raise also allows the salary to be set lower than its previous value. I would either check for that, or rename raise to setSalary.

Given all that, it would lead to something like:

import java.util.Objects;
import java.util.concurrent.atomic.AtomicInteger;

public class Employee {
    private static AtomicInteger lastID = new AtomicInteger();
    private String name;
    private final int ID;
    private double salary;

    public Employee(String name, double salary) {
        this.name = name;
        ID = lastID.getAndIncrement();
        this.salary = salary;
    }

    public String getName() { return name; }

    public int getID() { return ID; }

    public double getSalary() { return salary; }

    public void setName(String newName) { name = newName; }

    public void setSalary(double newSalary) { salary = newSalary; }

    @Override
    public String toString() {
        return "Name: " + getName() + "\nID: " + getID() + "\nSalary: " + getSalary();
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (!(o instanceof Employee)) return false;
        Employee employee = (Employee) o;
        return ID == employee.ID;
    }

    @Override
    public int hashCode() {
        return Objects.hash(ID);
    }
}

Answer 2

It's instanceof, with a lowercase o, not "instanceOf" as you have it right now. After correcting this mistake, you can see that not all branches of the method have a return statement - what happens if the first if is entered but the second isn't?

Simplifying away boolean conditions and redundant else statements can solve this:

@Override
public boolean equals(Object o){
    if (o instanceof Employee) { // instanceof instead of instanceOf
        Employee input=(Employee) o;
        return this.getSalary() == input.getSalary() && 
               this.getName().equals(input.getName());
    }
    return false;
}