2
\$\begingroup\$

To keep in practice with good techniques of java programming, I've decided to write a database. All it does it stores employees, allows users that are logged in to get/set employees, and has a login mechanism that prevents any methods from being used if you are not logged in. I'm looking for feedback in the following areas:

  • Structure Is the overall structure of the program good? Are there any improvements that can be made to make the structure of the program more efficient/compact?
  • Login Mechanism Is the way I implemented this system okay? Is there a better way I can implement this login system?
  • Exceptions I created two custom exceptions for this program. Is the way I coded these exceptions acceptable? Should I have had a CustomException class that they could inherit from, so I can just have the message in printErrorMessage as a parameter/hard coded? I used Eclipse while writing these exceptions, so the serialVersionUID is auto-generated.
  • Efficiency/Compactness Is there any way this program can be made more efficient? Such as the getHighestSalary method in the Database class.

Any and all suggestions are invited and appreciated. Style and other neatness tips are encouraged as well.

Database.java

package database;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class Database {

    private ArrayList<Employee> employees;
    private final String username;
    private final String password;
    private boolean loggedIn;

    public Database() {

        this.employees = new ArrayList<Employee>();
        this.username = generateUsername();
        this.password = generatePassword();
        this.loggedIn = false;

        populateEmployees();
    }

    public void addEmployee(Employee employee) throws LoginException {
        if(loggedIn) {
            this.employees.add(employee);
            sortEmployees();
            return;
        }
        throw new LoginException("Not Logged In!");
    }

    public Employee getHighestSalary() throws LoginException {
        if(loggedIn) {
            Employee highest = this.employees.get(0);
            for(Employee employee : this.employees) {
                if(employee.getSalary() > highest.getSalary()) {
                    highest = employee;
                }
            }
            return highest;
        }
        throw new LoginException("Not Logged In!");
    }

    @SuppressWarnings("unchecked")
    public void sortEmployees() throws LoginException {
        if(loggedIn) {
            Collections.sort((List)this.employees); 
            return;
        }
        throw new LoginException("Not Logged In!");
    }

    public Employee getEmployee(String name) throws EmployeeNotFoundException, LoginException {
        if(loggedIn) {
            for(Employee employee : this.employees) {
                if(employee.getName().equals(name)) {
                    return employee;
                }
            }
            throw new EmployeeNotFoundException("Employee Not Found!");
        }
        throw new LoginException("Not Logged In!");
    }

    //Filler for tester class
    private void populateEmployees() {
        for(int i = 0; i < 10; i++) {
            this.employees.add(new Employee("Employee" + i));
        }
    }

    public void login(String username, String password) {
        if(this.username.equals(username) && this.password.equals(password)) {
            this.loggedIn = true;
        }
    }

    public ArrayList<Employee> getEmployees() throws LoginException {
        if(loggedIn) {
            return this.employees;
        }
        throw new LoginException("Not Logged In!");
    }

    //Used for testing
    private String generateUsername() {
        return "username123";
    }

    //Used for testing
    private String generatePassword() {
        return "password123";
    }

}

Employee.java

package database;

public class Employee {

    private final String name;
    private int age;
    private int salary;


    public Employee(String name) {
        this.name = name;
    }

    public Employee(String name, int age) {
        this.name = name;
        this.age = age;
    }

    public Employee(String name, int age, int salary) {
        this.name = name;
        this.age = age;
        this.salary = salary;
    }

    public String getName() { return this.name; }
    public int getAge() { return this.age; }
    public int getSalary() { return this.salary; }

    public String toString() {
        return "Name: " + this.name;
    }

}

LoginException.java

package database;

public class LoginException extends Exception {

    private static final long serialVersionUID = 1L;

    public LoginException(String message) {
        super(message);
    }

    public void printErrorMessage() {
        System.out.println("LoginException: Not logged in!");
    }

}

EmployeeNotFoundException.java

package database;

public class EmployeeNotFoundException extends Exception {

    private static final long serialVersionUID = 1L;

    public EmployeeNotFoundException(String message) {
        super(message);
    }

    public void printErrorMessage() {
        System.out.println("EmployeNotFoundException: The employee you are searching for could not be found!");
    }

}

Tester.java

package database;

public class Tester {

    @SuppressWarnings("unused")
    public static void main(String[] args) {
        Database database = new Database();
        database.login("username1234", "password123");

        //Should throw `LoginException`
        try {
            for(Employee employee : database.getEmployees()) {
                System.out.println(employee);
            }
        } catch (LoginException e) {
            e.printErrorMessage();
        }

        //Should throw `EmployeeNotFoundException`
        try {
            Employee test = database.getEmployee("Ben");
        } catch (EmployeeNotFoundException e) {
            e.printErrorMessage();
        } catch (LoginException e) {
            e.printErrorMessage();
        }

    }

}
\$\endgroup\$
4
\$\begingroup\$

Structure

Database does not adhere to the concept of Single Responsibility. Ignoring the test code, there are some considerations to make. I would only keep the autenthication methods and perhaps add DBMS methods as transaction functionality in this class. All ORM-related methods should be put in seperate Repository classes.

public class EmployeeRepository : Repository<Employee> {
    public void save(Employee employee) { /* .. */ }
    public void delete(int employeeId) { /* .. */ }
    public Employee Get(int employeeId) { /* .. */ }
    // and so on ..
}

Employee is an entity, but does not have a primary key. I would add an int employeeId;. Some fields are final, others aren't, but only getters are available. You are inconsistent. Always override equals and hashcode for entities, in order to distinguish them from other instances.


Login Mechanism

You check a plain password against a stored plain password. This is as bad a practice I can think of. Fortunately for you, many companies wouldn't even mind this flow ;-)

You are much better of hashing the password, using a salt, perhaps some pepper using key stretching. And please don't try to implement this yourself, use existing APIs.


Exceptions

Your *Exception classes provide utility methods that write to the console. I can understand why you add them for a trivial example as this, but they wouldn't make sense otherwise. There already is the message. I don't understand why you don't print the message, but a hardcoded string instead.

public void printErrorMessage() {
        System.out.println("LoginException: Not logged in!");
}
   public void printErrorMessage() {
        System.out.println(e.getMessage());
   }

I used Eclipse while writing these exceptions, so the serialVersionUID is auto-generated.

If your IDE generates code for you, do you want to know why this was done?


Efficiency/Compactness

Your loops can be refactored. Let's take one example.

 Employee highest = this.employees.get(0);
      for(Employee employee : this.employees) {
          if(employee.getSalary() > highest.getSalary()) {
              highest = employee;
          }
 }
Employee highest = Collections.max(employees, Comparator.comparing(c -> c.getSalary()));

How efficient is it to sort entities on the repository? This is something calling code should bother with, not the repository.

  public void sortEmployees() { /* .. */ }
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I would not even put the authentication methods in the repository itself. It is possible to separate the responsibility of restricting access control completely by defining the repository as an interface and storing the access control information in a thread local. Access control can then be implemented as an invisible proxy for the repository. This approach does not bind the repository to a specific kind of authentication method. \$\endgroup\$ – TorbenPutkonen Jun 17 '19 at 13:15
  • \$\begingroup\$ @TorbenPutkonen My point exactly was to keep authentication and DBMS related stuff out of the Repository. I would see this Database class as a proxy class for all non-ORM related database actions, such as authentication. Its implementation may very well use the strategy you described. \$\endgroup\$ – dfhwze Jun 17 '19 at 14:16

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.