3
\$\begingroup\$

This is the question:

You have an Employee class:

class Employee {
    String name;
    Integer id,
    Employee manager
}

Each employee has a manager and the manager of CEO is null. Find all direct and indirect employees of a manager.

For example:

  • Say Employee e1 reports to CEO
  • e2 and e3 reports to e1
  • e4 and e5 reports to e2
  • e6 reports to e3

Then by giving e1 as input, output should be e2, e3, e4, e5 and e6.

I know I am using the stupidest way to do it. But I wonder if there is any better way to do the if-statement?

function func() {
  function Employee(name, id, manager) {
    this.name = name;
    this.id = id;
    this.manager = manager;
  }

  const ceo = new Employee("ceo", 1, null);
  const e1 = new Employee("e1", 2, ceo);
  const e2 = new Employee("e2", 3, e1);
  const e3 = new Employee("e3", 4, e1);
  const e4 = new Employee("e4", 5, e2);
  const e5 = new Employee("e5", 6, e2);
  const e6 = new Employee("e6", 7, e3);

  var name = document.getElementById("txtName").value;

  var eArr = [];


  if (e1.manager.name === name.toLowerCase()) {
    eArr.push(e1.name);
  }
  if (e2.manager.name === name.toLowerCase() || (e2.manager.manager !== null && e2.manager.manager.name === name.toLowerCase())) {
    eArr.push(e2.name);
  }
  if (e3.manager.name === name.toLowerCase() || (e3.manager.manager !== null && e3.manager.manager.name === name.toLowerCase())) {
    eArr.push(e3.name);
  }
  if (e4.manager.name === name.toLowerCase() || (e4.manager.manager !== null && e4.manager.manager.name === name.toLowerCase()) || (e4.manager.manager.manager !== null && e4.manager.manager.manager.name === name.toLowerCase())) {
    eArr.push(e4.name);
  }
  if (e5.manager.name === name.toLowerCase() || (e5.manager.manager !== null && e5.manager.manager.name === name.toLowerCase()) || (e5.manager.manager.manager !== null && e5.manager.manager.manager.name === name.toLowerCase())) {
    eArr.push(e5.name);
  }
  if (e6.manager.name === name.toLowerCase() || (e6.manager.manager !== null && e6.manager.manager.name === name.toLowerCase()) || (e6.manager.manager.manager !== null && e6.manager.manager.manager.name === name.toLowerCase())) {
    eArr.push(e6.name);
  }

  if (eArr.length !== 0)
    document.getElementById("ans").innerHTML = "Ans: " + eArr;
  else
    document.getElementById("ans").innerHTML = "Ans: No reportees";
}
Manager Name = <input type="text" id="txtName">
<button onclick="func()">Result</button>
<p id="ans"></p>

\$\endgroup\$
  • \$\begingroup\$ This gives me a new idea: Tree.from \$\endgroup\$ – FreezePhoenix Aug 17 '18 at 0:01
1
\$\begingroup\$

Hard-coding all of these relationships is problematic: you have to rewrite all of your code if any relationships change or if new employees are added. What if you have a company with a few hundred or a few thousand employees?

To scale up, first create a data structure for your employees. This could be an array or an object keyed by ID. Generally, this information will be populated from JSON or XML returned from a database, so I've created an employeeData structure that mimics this.

Once you have a workable employees array and relationships have been created, write an Employee method employeeOf. This method works exactly as you're doing it, but dynamically using a loop rather than a series of hard-coded conditionals.

In terms of HTML, I recommend specific DOM element names and using an event listener as an alternative to an onclick property. Consider making your input/button combo a <form>.

Here's a potential first refactor, keeping your basic design but addressing some of the above:

(function() {

  function Employee(name, id, manager) {
    this.name = name;
    this.id = id;
    this.manager = manager;
  }
  
  Employee.prototype.employeeOf = function(target) {
    let manager = this.manager;

    while (manager) {
      if (manager.name === target) {
        return true;
      }

      manager = manager.manager;
    }

    return false;
  };

  const employeeData = [
    {name: "ceo", id: 1, manager: null},
    {name: "e1", id: 2, manager: "ceo"},
    {name: "e2", id: 3, manager: "e1"},
    {name: "e3", id: 4, manager: "e1"},
    {name: "e4", id: 5, manager: "e2"},
    {name: "e5", id: 6, manager: "e2"},
    {name: "e6", id: 7, manager: "e3"}
  ];

  const employees = employeeData.reduce((a, e) =>
    a.concat([new Employee(
      e.name, e.id, a.find(m => m.name === e.manager
    ))]), []
  );

  document.getElementById("find-employees-btn").addEventListener("click", e => {
    const target = document.getElementById("employee-input").value;
    const result = employees.filter(e => e.employeeOf(target)).map(e => e.name);

    document.getElementById("employee-result")
      .innerHTML = "Ans: " + (result.length ? result : "No reportees");
  });
})();
Manager Name = <input type="text" id="employee-input">
<button id="find-employees-btn">Result</button>
<p id="employee-result"></p>

This works, but there are many areas to improve.

For example, building the employees structure requires find, which is succinct but traverses up to the entire array on each call. Managers are assumed to have been added to the array prior to their employees which can cause issues.

More seriously, the bottom-up "employee has a manager" relationship makes it awkward and slow to retrieve the top-down, opposite relationship: employees of a manager. If redesigning your Employee class is an option, consider this.employees (an array of Employees) rather than (or in addition to) this.manager (an Employee) as a member field. This makes searching for employees much more efficient and simpler to code: simply iterate over the target manager's employees and recursively collect all of those employees' underlings. There won't be any wasted checks or conditionals in this design.

\$\endgroup\$
  • 1
    \$\begingroup\$ This is the best answer, as I write this. (1) It is by far more comprehensible than the other. (2) Employee has functionality! Very Single Responsibility, very OO. Kudos. (3) An Employees class would be nice. There would be no ...assumed to have been added to the array.... Also, would be the right place for the employees function and code that is in getElementById calls. Then a Employees.create(employeeStruct) would be the capstone for a beautifully encapsulated and reusable Employee/Employees functionality. \$\endgroup\$ – radarbob Aug 17 '18 at 0:04
  • \$\begingroup\$ @radarbob In my answer, there is no 'assumed to be added to an array'. It is in a private method, and if you are 'assuming' anything then you are messing with the code on purpose in the first place. I could say a similar thing: 'assuming a property is set'. In my example, I made it as robust as possible so that if you manage to get access to an employee instance, then you are not using it as the OP asked. \$\endgroup\$ – FreezePhoenix Aug 17 '18 at 1:06
  • \$\begingroup\$ @ggorlen, is reduce() act like recursive function? \$\endgroup\$ – Justin Ling Aug 17 '18 at 1:13
  • \$\begingroup\$ @JustinLing No, it does not. \$\endgroup\$ – FreezePhoenix Aug 17 '18 at 1:14
  • \$\begingroup\$ Keep in mind I'm posting code as a "potential first refactor" rather than "this is the final result". I'm critiquing the design decisions/tradeoffs I made in the first refactor in the interest of being instructive. @radarbob has some nice ideas for extension beyond this first attempt. \$\endgroup\$ – ggorlen Aug 17 '18 at 1:14
1
\$\begingroup\$

Your approach could be made more robust by not hardcoding both the number of employees and their level in the heirarchy. You can do this by storing the employees in an array, where you can use loops or array methods to traverse, and using recursion to recursively climb the manager references.

const isEmployeeOf = (employee, managerName) => {
  // If the current employee is directly under the manager.
  if (employee.manager && employee.manager.name === managerName) return true

  // If we still need to look further, go one step up.
  if (employee.manager) return isEmployeeOf(employee.manager, managerName)

  // No more managers, must be false.
  return false;
}

const employees = [/* an array of all employees */]
employees.filter(employee => isEmployeeOf(employee, managerName))

However, this is inefficient as it needs to check all employees and climb the managerial hierarchy, both of which may not even belong to the manager you're looking for.

Instead, consider building a tree that you can traverse in a top-down manner (CEO downwards) to find your manager and gather its underlings. There are different kinds of tree structures as well as traversal methods, each with their own pros and cons. A good way to start with tree traversal is Depth-First Traversal which can easily be done using recursion.

\$\endgroup\$
  • 1
    \$\begingroup\$ this does not answer the question because it does not yield a list of all direct and indirect employees. \$\endgroup\$ – radarbob Aug 16 '18 at 23:39
  • \$\begingroup\$ @Joseph, thank you for your review and your suggestion. \$\endgroup\$ – Justin Ling Aug 17 '18 at 0:57
1
\$\begingroup\$

Review

You shouldn't put a function definition inside of another function like that. It means it has to recreate the function every time. It would be better to not get in a habit of that. Same goes for the contants ceo, etc. In addition, your code only works for one test case, as you manually check for each member.

Rewrite:

const [addEmployee, getEmplyeeBelows] = (function() {
   class Employee {
      constructor(name, id, manager) {
        this.name = name;
        this.id = id;
        this.manager = manager;
        this.manager = registry[manager];
    }
   }
   const registry = {};
   function addEmployee(name, number, parent) {
      if( parent === null ) {
         registry[name] = new Employee(name, number, parent);
      } else {
         parent.emplyees[number] = new Employee(name, number, parent);
         registry[name] = parent.emplyees[number];
      }
   }
   function getEmplyeeBelows(name) {
      let result = [];
      result.push(...Object.values(registry).map((emp) => {
        return emp.manager == name ? [emp, ...getEmplyeeBelows(emp)] : [];
      })
      return result;
   }
   return [addEmployee, getEmplyeeBelows]
})()
addEmployee('ceo', 1, null);
addEmployee('e1', 2, 'ceo');
addEmployee("e2", 3, 'e1');
addEmployee("e3", 4, 'e1');
addEmployee("e4", 5, 'e2');
addEmployee("e5", 6, 'e2');
addEmployee("e6", 7, 'e3');

const manager = () => {

  var name = document.getElementById("txtName").value;

  eArr = getEmplyeeBelows(name);

  if (eArr.length !== 0)
    document.getElementById("ans").innerHTML = "Ans: " + eArr;
  else
    document.getElementById("ans").innerHTML = "Ans: No reportees";
}
\$\endgroup\$
  • \$\begingroup\$ That a bit of implementation is any particular language construct is moot to the "employee business domain". These things should not be leaked into the abstraction being created. As soon as any code has meaning for "employees" (in our case) it should be given that meaning; regardless if it is a variable, loop, function, index, parameter, class, module, etc. \$\endgroup\$ – radarbob Aug 17 '18 at 0:26
  • \$\begingroup\$ @FreezePhoenix, thank you for your review, but I think adding this.employees = []; to Employee class is prohibited. \$\endgroup\$ – Justin Ling Aug 17 '18 at 1:01
  • \$\begingroup\$ @JustinLing Is that better? \$\endgroup\$ – FreezePhoenix Aug 17 '18 at 1:14

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.