2
\$\begingroup\$

I was going through design questions frequently being discussed in interviews and have found a problem which states:

Consider a company which wants to process salary hikes of its employees during recession period. As a precautionary measure, instead of hiking all employee salaries it decided to hike only for the employees who met at least two of the criteria. The problem statement is to design the classes and functions to help the company processing the hike letters.

  1. Published at least two research papers.
  2. Got star of the year award.
  3. Completed at least 5 years of experience.

Please check if its design implementation is okay (such as if it's loosely-coupled, well-encapsulated, and other concepts of OOPS are being followed).

package com.salaryhike;

import java.util.HashMap;

public class Company {
    private String name;

    public String getName() {
            return name;
    }

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

    HashMap<Employee, HashMap<AcheivementType, Object>> achiements = new HashMap<>();

    private void decideHike(SalaryUpdatable employee) {
            if (employee instanceof Employee && achiements.containsKey(employee)) {
                    float hikeValue = (15 / 100f);
                    HashMap<AcheivementType, Object> employeeAchievements = achiements.get(employee);
                    Integer numOfResercepapers = (Integer) employeeAchievements.get(AcheivementType.REARCH_PAPER);
                    Integer numberOfyearsOfemployement = (Integer) employeeAchievements
                                    .get(AcheivementType.NUM_OF_YEARS_EMPLOYED);
                    Boolean isStarOfTheYear = (Boolean) employeeAchievements.get(AcheivementType.IS_STAR_OF_THE_YEAR);
                    boolean eligibilitySet1 = false;
                    boolean eligibilitySet2 = false;
                    boolean eligibilitySet3 = false;
                    if (isStarOfTheYear != null && numberOfyearsOfemployement != null) {
                            eligibilitySet1 = isStarOfTheYear && (numberOfyearsOfemployement >= 5);
                    }
                    if (isStarOfTheYear != null && numOfResercepapers != null) {
                            eligibilitySet2 = isStarOfTheYear && (numOfResercepapers >= 2);
                    }
                    if (numberOfyearsOfemployement != null && numOfResercepapers != null) {
                            eligibilitySet3 = ((numberOfyearsOfemployement >= 5) && (numOfResercepapers >= 2));
                    }
                    if (eligibilitySet1 || eligibilitySet2 || eligibilitySet3) {
                            employee.updateSalary(this, employee.getSalary(this) + (hikeValue * employee.getSalary(this)));
                    }
            }
    }

    public void registerAchievement(Employee employee, AcheivementType type, Object newValue) {
            (achiements.get(employee)).put(type, newValue);
    }

    public void hireEmployee(Employee employee) {
            if (achiements.containsKey(employee)) {
                    throw new IllegalEmployement("EMPLOYEE ALREADY EMPLOYED");
            }
            achiements.put(employee, new HashMap<AcheivementType, Object>());
    }

    public void removeEmployee(Employee employee) {
            achiements.remove(employee);
    }

    public class IllegalEmployement extends RuntimeException {
            public IllegalEmployement(String msg) {
                    super(msg);
            }
    }

    public static void init() {
            Company company = new Company("ABC_PVT_LTD");
            Employee employee1 = new Employee(company, "A", 20000);
            Employee employee2 = new Employee(company, "B", 40000);
            Employee employee3 = new Employee(company, "C", 50000);
            Employee employee4 = new Employee(company, "D", 350000);
            Employee employee5 = new Employee(company, "A", 20000);
            Employee employee6 = new Employee(company, "B", 40000);
            Employee employee7 = new Employee(company, "C", 50000);
            Employee employee8 = new Employee(company, "D", 350000);
            company.hireEmployee(employee1);
            company.hireEmployee(employee2);
            company.hireEmployee(employee3);
            company.hireEmployee(employee4);
            company.hireEmployee(employee5);
            company.hireEmployee(employee6);
            company.hireEmployee(employee7);
            company.hireEmployee(employee8);
            employee1.registerAchievements(AcheivementType.REARCH_PAPER, 3);
            employee2.registerAchievements(AcheivementType.REARCH_PAPER, 1);
            employee3.registerAchievements(AcheivementType.REARCH_PAPER, 2);
            employee4.registerAchievements(AcheivementType.JAVA_CERIFICATION_NUM, 3);
            employee1.registerAchievements(AcheivementType.NUM_OF_YEARS_EMPLOYED, 5);
            employee2.registerAchievements(AcheivementType.IS_STAR_OF_THE_YEAR, true);
            employee3.registerAchievements(AcheivementType.IS_STAR_OF_THE_YEAR, true);
            employee4.registerAchievements(AcheivementType.JAVA_CERIFICATION_NUM, 3);
            employee5.registerAchievements(AcheivementType.REARCH_PAPER, 3);
            employee6.registerAchievements(AcheivementType.REARCH_PAPER, 1);
            employee7.registerAchievements(AcheivementType.REARCH_PAPER, 2);
            employee8.registerAchievements(AcheivementType.JAVA_CERIFICATION_NUM, 3);
            company.decideHike(employee1);
            company.decideHike(employee2);
            company.decideHike(employee3);
            company.decideHike(employee4);
            company.decideHike(employee5);
            company.decideHike(employee6);
            company.decideHike(employee7);
            company.decideHike(employee8);
            System.out.println(employee1.getSalary(company));
            System.out.println(employee2.getSalary(company));
            System.out.println(employee3.getSalary(company));
            System.out.println(employee4.getSalary(company));
            System.out.println(employee5.getSalary(company));
            System.out.println(employee6.getSalary(company));
            System.out.println(employee7.getSalary(company));
            System.out.println(employee8.getSalary(company));
    }

    public static void main(String args[]) {
            Company.init();
    }
}

enum AcheivementType {
    REARCH_PAPER, NUM_OF_YEARS_EMPLOYED, IS_STAR_OF_THE_YEAR, JAVA_CERIFICATION_NUM
}

interface SalaryUpdatable {
    public void updateSalary(Company company, double updatedSalary);

    public double getSalary(Company company);

    public class SalaryException extends RuntimeException {
            public SalaryException(String msg) {
                    super(msg);
            }
    }
}

class Employee implements SalaryUpdatable {
    private Company company;
    private double salary;
    private String name;

    public Employee(Company company, String name, double offeredSalary) {
            this.company = company;
            this.name = name;
            this.salary = offeredSalary;
    }

    public String getName() {
            return name;
    }

    public double getSalary(Company company) {
            if (company == this.company) {
                    return salary;
            } else {
                    throw new SalaryException("NOT ATHOURIZED TO KNOW SALARY");
            }
    }

    public void updateSalary(Company company, double updatedSalary) {
            if (company == this.company) {
                    this.salary = updatedSalary;
            } else {
                    throw new SalaryException("NOT ATHOURIZED TO KNOW SALARY");
            }
    }

    public void registerAchievements(AcheivementType type, Object value) {
            company.registerAchievement(this, type, value);
    }
} 
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Giving this code a cursory glance, the first thing that jumps out at me is the SalaryUpdatable interface. It seems to me like forcing an interface for the sake of showing implementing an interface. The biggest problem with it is that it fails to provide a meaningful abstraction. The only place it is used is as a parameter of Company#decideHike, and you immediately convert it into an Employee once it goes into the method. Get rid of it to simplify the code.

The second thing that jumps out at me is circular dependency between Company and Employee. A Company instance holds a collection of Employees, who then hold a reference to their containing Company. This is over-complicating your data model. I'm not sure if you are looking to potentially have Employees in multiple companies or multiple Companys with exclusive Employees, but this is not a problem manifest in the spec so you should not over-engineer (an example of YAGNI).

At the same time, you also have Companys holding the achievements of their Employees. Not only is this confusing to follow through an Employee back through to the Company, the Company now has (at least) two responsibilities: keep track of its Employees, and map Employees to their achievements. Since you are calling the method on the Employee, perhaps it makes more sense to keep the achievement in the Employee. Because these are objects in our system, and not actual people, we can trust them not to alter their achievements.

The next thing that bothers me is that achievements are stored in a map from AchievementType to Object. This means that we lose information that we have to later recover for each achievement type. This makes our system harder to maintain and harder to change. We need to examine the data model to see if there is a better way.

Since we should now have the Employee tracking its own achievements, we can actually break down the achievements into individual properties on the Employee. So

public void registerAchievements(AcheivementType type, Object value {
    company.registerAchievement(this, type, value);
}

becomes:

private int yearsEmployed = 0;
private int researchPapers = 0;
private int javaCerts = 0;
private boolean isStarOfYear = false;

...

public int getYearsEmployed() { return yearsEmployed; }
public void setYearsEmployed(int years) { yearsEmployed = years; }

public int getResearchPapers() { return researchPapers; }
public void setResearchPapers(int papers) { researchPapers = papers; }

public int getJavaCertifications() { return javaCerts; }
public void setJavaCertifications(int certs) { javaCerts = certs; }

public boolean getIsStarOfTheYear() { return isStarOfYear; }
public void setIsStarOfTheYear(boolean isStar) { isStarOfYear = isStar; }

We are now reducing Employee down to a value object. We are looking at it for the data it holds. It no longer talks about AchievementType, removing a dependency, but meaning we still have to figure out how to get those back so we can use them in Company.

Here, though, we have a design decision to make. Before, AchievementType was the key half of a map to a value, but now we've separated the value out into Employee. We could go back in Company and recreate the association, but we have an opportunity to look at the overall algorithm now. We aren't really interested in the values, only the achievements that are determined by those values. Specifically, we aren't interested in the exact number of years of experience, only that it's 5 or more.

We can start by transforming the AchievementType enum into an Achievement enum for the achievements we are interested in:

public enum Achievement
{
    PUBLISHED_TWO_RESEARCH_PAPERS,
    GOT_STAR_OF_THE_YEAR,
    HAS_FIVE_YEARS_EXPERIENCE;
}

Note that it turns out we are not interested in the number of Java certifications, so we don't need to add that as an Achievement right now. Now that we have our Achievements, we need a way to map to them from an Employee. We could create a method on Employee, but this creates a new dependency for that class. It would mean that, no matter how we want to use an Employee, we would always have Achievements tagging along, even if we don't want or need them. Achievements, though, are always dependent on an Employee having them, so we can instead create a static Achievement "factory" method on Achievement that takes an Employee and returns Achievements.

To return multiple Achievements, we need to wrap them into a collection container of some sort. Since we know we're going to have random access, and each Achievement should only appear once, we can return a Set<Achievement>. The "factory" method is going to be left a bit ugly, leaving a refactoring as an exercise for the reader:

public enum Achievement
{
    // ...
    public static Set<Achievement> forEmployee(Employee employee) {
        Set<Achievement> achievements = new HashSet<>();

        if(employee.getYearsEmployed() >= 5) {
            achievements.add(HAS_FIVE_YEARS_EXPERIENCE);
        }

        if(employee.getResearchPapers() >= 2) {
            achievements.add(PUBLISHED_TWO_RESEARCH_PAPERS);
        }

        if(employee.getIsStarOfTheYear()) {
            achievements.add(GOT_STAR_OF_THE_YEAR);
        }

        return achievements;
    }
}

This now greatly reduces the amount of logic in the Company#decideHike method, as we now would only need to check if the returned Set<Achievement> has two or more elements. The whole thing when all done looks like:

Employee.java

public class Employee
{
    private String name;
    private double salary;
    private int yearsEmployed = 0;
    private int researchPapers = 0;
    private int javaCerts = 0;
    private boolean isStarOfYear = false;

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

    public String getName() {
        return name;
    }

    public double getSalary() {
        return salary;
    }

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

    public int getYearsEmployed() {
        return yearsEmployed;
    }

    public void setYearsEmployed(int years) {
        yearsEmployed = years;
    }

    public int getResearchPapers() {
        return researchPapers;
    }

    public void setResearchPapers(int papers) {
        researchPapers = papers;
    }

    public int getJavaCertifications() {
        return javaCerts;
    }

    public void setJavaCertifications(int certs) {
        javaCerts = certs;
    }

    public boolean getIsStarOfTheYear() {
        return isStarOfYear;
    }

    public void setIsStarOfTheYear(boolean isStar) {
        isStarOfYear = isStar;
    }
}

Company.java

import java.util.Set;
import java.util.List;
import java.util.ArrayList;

public class Company
{
    private String name;
    private List<Employee> employees;

    public Company(String name) {
        this.name = name;
        employees = new ArrayList<>();
    }

    public String getName() {
        return name;
    }

    public void hire(Employee employee) {
        employees.add(employee);
    }

    public void giveRaises() {
        for(Employee employee : employees) {
            determineHike(employee);
        }
    }

    private void determineHike(Employee employee) {
        Set<Achievement> achievements = Achievement.forEmployee(employee);
        if(achievements.size() < 2) {
            return;
        }

        double hikeValue = 15.0 / 100;
        double currentSalary = employee.getSalary();
        employee.setSalary(currentSalary + currentSalary * hikeValue);
    }
}

Achievement.java

import java.util.Set;
import java.util.HashSet;

public enum Achievement
{
    PUBLISHED_TWO_RESEARCH_PAPERS,
    GOT_STAR_OF_THE_YEAR,
    HAS_FIVE_YEARS_EXPERIENCE;

    public static Set<Achievement> forEmployee(Employee employee) {
        Set<Achievement> achievements = new HashSet<>();

        if(employee.getYearsEmployed() >= 5) {
            achievements.add(HAS_FIVE_YEARS_EXPERIENCE);
        }

        if(employee.getResearchPapers() >= 2) {
            achievements.add(PUBLISHED_TWO_RESEARCH_PAPERS);
        }

        if(employee.getIsStarOfTheYear()) {
            achievements.add(GOT_STAR_OF_THE_YEAR);
        }

        return achievements;
    }
}

There's still more that can be done. For your main method, you should extract it out into a separate runner class. You should also use arrays and loops instead of creating each object individually. For the Employee class, there are some bad method names (i.e. getIsStarOfTheYear); we can follow Tell Don't Ask more closely; some of the setters can be replaced with constructor injection; and to be a true value object it should implement hashCode, equals, and toString. In Company, there is a List of Employees, but this might be better as a Set since each Employee should only be in there once. In general, monetary values should not be doubles because of precision errors.

\$\endgroup\$
3
\$\begingroup\$

Separate test code from production code

That means move init and main to a class SalaryHikeTest.

Use interfaces in declarations wherever possible

See relevant item summary from Effective Java here.

Item 52: Refer to objects by their interfaces

If appropriate interface types exist, then parameters, return values, variables, and fields should all be declared using interface types.

e.g. do not use HashMap where Map would suffice.

instead of :

    HashMap<Employee, HashMap<AcheivementType, Object>> achiements = new HashMap<>();

use:

    Map<Employee, Map<AcheivementType, Object>> achiements = new HashMap<>();

It's

Get rid of unused and unnecessary code

IllegalEmployement, SalaryException is unused.

Extracting the interface SalaryUpdatable is not justified. YAGNI

These snippets are unnecessary too:

} else {
        throw new SalaryException("NOT ATHOURIZED TO KNOW SALARY");
}

They aren't in the spec. Even if they were use exceptions for exceptional situations. See Item 57 and following in Effective Java (summary).

Zero One Infinity

A 1, 2, or 3 after a variable is sign of 0, 1, n violation

boolean eligibilitySet1 = false;
boolean eligibilitySet2 = false;
boolean eligibilitySet3 = false;

What's worse in this case is you hand coded each 2 element combinations of the criteria. C(n, r) is 3 for n = 3 and r = 2. This is not viable though: If the condition was "satisfying any 5 of 10 criteria", you would have to hand code 252 cases. And if the condition was "satisfying any 10 of some 20 criteria", you would have to hand code 184,756 cases. It's called combinatorial explosion for a reason.

Also see numberOfyearsOfemployement >= 5, numOfResercepapers >= 2 etc are copy/pasted. Copy/pasted code is bugs waiting to happen. Instead count the criteria satisfied and compare it to the threshold in the end:

Compare with this:

int numCriteriaSatisfied = 0;

if (numberOfyearsOfemployement >= 5) numCriteriaSatisfied ++;
if (numOfResercepapers >= 2) numCriteriaSatisfied ++;
if (isStarOfTheYear) numCriteriaSatisfied ++;

if (numCriteriaSatisfied >= MINIMUM_NUMBER_OF_CRITERIA) doSomething(...);

OO design

There are some OO design problems, but if you fix these first they will be easier to spot and fix.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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