2
\$\begingroup\$

I have created a simple Employee program and I am looking for a code review.

I want to make sure my use of concepts such as double-underscore methods and @property annotations are correct. I am also looking for feedback on my methodology.

Please feel free to be critical of each aspect of the program:

import time;

#Handles creating and maintaining employees.
class Employee:
  num_of_emps = 0
  raise_amount = 1

  def __init__(self, first, last, id, pay):
    self.first = first
    self.last = last
    self.id = id
    self.pay = pay
    self.manager = None
    self.start_time = time.strftime("%D @ %I:%M:%S %p")
    Employee.num_of_emps += 1
    Employee.raise_amount += .01

  #Method defines a string used
  #to recreate Employee object.
  def __repr__(self):
    return "Employee('{}', '{}', '{}')".format(self.first, self.last, self.pay)

  #Method created for readability
  #for end user.
  def __str__(self):
    return self.info

  #Alternative constructor, built to create
  #Employee objects from strings.
  @classmethod
  def from_string(cls, emp_str):
    first, last, id, pay = emp_str.split('-')
    return cls(first, last, id, pay)

  #Method built to check if a
  #date falls on the weekend.
  @staticmethod
  def is_workday(day):
    if day.weekday() == 5 or day.weekday() == 6:
      return False
    return True

  #Method developed s/t supervisor's employees
  #will always also be managers employees.
  def set_manager(self, mgr):
    self.manager = mgr

  #Method prints employee information.
  @property
  def info(self):
    if isinstance(self, Manager):
      return print("Manager's ID:", self.id, "\nFull name:", self.fullname, "\nSalary:", self.pay,
                   "\nEmployee's under supervision:", self.print_emp, "\nEmployee stated on:", self.start_time)
    if isinstance(self, Supervisor):
      return print("Supervisor's ID:", self.id, "\nFull name:", self.fullname, "\nSalary:", self.pay,
                   "\nEmployee's under supervision:", self.print_emp, "\nEmployee stated on:", self.start_time)
    elif isinstance(self, Developer):
      return print("Developer's ID:", self.id, "\nFull name:", self.fullname, "\nSalary:", self.pay,
                   "\nPrograming Language:", self.prog_laung, "\nEmployee stated on:", self.start_time)
    else:
      return print("Employee ID:", self.id, "\nFull name:", self.fullname, "\nSalary:", self.pay,
                   "\nEmployee stated on:", self.start_time)

  #Given an Employee object one can change
  #its pay amount.
  @property
  def new_raise(self):
    self.pay = int(self.pay * self.raise_amount)

  #The following three methods created to manage
  #Employee objects full name.
  @property
  def fullname(self):
    return '{} {}'.format(self.first, self.last)

  @fullname.setter
  def fullname(self, name):
    first, last = name.split(' ')
    self.first = first
    self.last = last

  @fullname.deleter
  def fullname(self):
    self.fisrt = None
    self.last = None
    print('Name has been deleted.')


#Manager class, inherits from Employee class.
class Manager(Employee):
  raise_amount = 1.04

  def __init__(self, first, last, id, pay, emp_under_sup = None):
    super().__init__(first, last, id, pay)
    if emp_under_sup is None:
      self.emp_under_sup = []
    else:
      self.emp_under_sup = emp_under_sup

  #Alternative constructor, built to create
  #Employee objects from strings.
  @classmethod
  def from_string(cls, emp_str):
    first, last, id, pay, emp_under_sup = emp_str.split('-')
    return cls(first, last, id, pay, emp_under_sup=None)

  #Add an Employee object to the managers
  #employee supervision list.
  def add_emp(self, emp):
    if emp not in self.emp_under_sup:
      self.emp_under_sup.append(emp)
      emp.set_manager(self)

  #Remove an Employee object from the managers
  #employee supervision list.
  def remove_emp1(self, emp):
    if emp in self.emp_under_sup:
      self.emp_under_sup.remove(emp)

  @property
  def print_emp(self):
    return ', '.join([emp.fullname for emp in self.emp_under_sup])


#Supervisor class, inherits from Employee class.
class Supervisor(Employee):
  raise_amount = 1.03

  def __init__(self, first, last, id, pay, emp_under_sup = None):
    super().__init__(first, last, id, pay)
    if emp_under_sup is None:
      self.emp_under_sup = []
    else:
      self.emp_under_sup = emp_under_sup

  #Alternative constructor, built to create
  #Employee objects from strings.
  @classmethod
  def from_string(cls, emp_str):
    first, last, id, pay, emp_under_sup = emp_str.split('-')
    return cls(first, last, id, pay, emp_under_sup=None)

  #Add an Employee object to the supervisors
  #employee supervision list.
  def add_emp(self, emp):
    if emp not in self.emp_under_sup:
      self.emp_under_sup.append(emp)
      if self.manager:
        self.manager.add_emp(emp)

  #Remove an Employee object from the supervisors
  #employee supervision list.
  def remove_emp(self, emp):
    if emp in self.emp_under_sup:
      self.emp_under_sup.remove(emp)

  @property
  def print_emp(self):
    return ', '.join([emp.fullname for emp in self.emp_under_sup])


#Developer class, inherits from Employee class.
class Developer(Employee):
  raise_amount = 1.02

  def __init__(self, first, last, id, pay, prog_laung):
    super().__init__(first, last, id, pay)
    self.prog_laung = prog_laung

  #Alternative constructor, built to create
  #Employee objects from strings.
  @classmethod
  def from_string(cls, emp_str):
    first, last, id, pay, prog_laung = emp_str.split('-')
    return cls(first, last, id, pay, prog_laung)
\$\endgroup\$
  • 2
    \$\begingroup\$ How would this be used? What problem does it solve? \$\endgroup\$ – Mast Jul 10 '18 at 19:22
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question instead. \$\endgroup\$ – Mast Jul 11 '18 at 22:24
3
\$\begingroup\$

As a warning, I'm rather nit-picky, but here's everything I see:

  • Run your code through flake8 to ensure PEP8 compliance
  • Some of your names are a bit under-specified. I'd prefer first_name and last_name to first and last (first and last what?). Also, as a general rule, names are hard and it's actually not safe to assume a person has a first and last name. I'd rename pay to salary. Instead of emp_under_sup how about supervisees or direct_reports? Similarly, instead of add_emp how about add_supervisee? prog_laung should probably be programming_language.
    • Your ordering of first, last, id is a little strange. Usually the id comes first.
    • Generally we use """Doc comments.""" to document method and classes instead of # inline comments
    • Good use of @classmethod in your from_string methods
    • It seems from raise_amount that you use floats for money. Never use floats for money!
    • You use a list comprehension in many instances where a generator comprehension would be just fine (and doesn't build the intermediate list up in memory). Ex instead of return ', '.join([emp.fullname for emp in self.emp_under_sup]) try return ', '.join(emp.fullname for emp in self.emp_under_sup)
    • In Python we tend to try things and catch failures instead of asking for permission beforehand. So for example instead of:
if emp in self.emp_under_sup:
    self.emp_under_sup.remove(emp)

Try:

try:
    self.emp_under_sup(remove)
except ValueError:
    pass

You may also want to revisit why you are suppressing this. Shouldn't it be an error if you try to remove a supervisee that isn't supervised by that employee?

  • In return cls(first, last, id, pay, emp_under_sup=None) there is no need to provide emp_under_sup=None as it defaults to None.
  • Prefer ternaries when possible, ex: self._emp_under_sup = [] if emp_under_sup is None else emp_under_sup
  • print('Name has been deleted.') is a bad idea for code that you want others to use. What if someone wants to use your code in a place where they don't want that printed?
  • Why do you have a deleter on fullname? What does "deleting" someone's name even mean? You probably don't need it.
  • name.split(' ') may return more than 2 parts (which will cause your tuple unpacking to ValueError). Instead try name.split(' ', 1) to limit to 1 split by space.
  • If you're using Python >=3.6 use f-strings instead of format strings. Ex. return f'{self.first} {self.last}' instead of return "{} {}".format(self.first, self.last) (we also prefer double quotes for format strings on Python <= 3.5)
  • Your use of @property and the setter for fullname is good (although I'd call it full_name. But all of your other properties are misused. Properties that aren't settings almost never should mutate things (because it's unexpected). See new_raise. Instead try something like employee.give_raise(), which more explicitly indicates that employee will be mutated. info and print_emp also shouldn't be properties (the fact that print_emp has a verb in its name is a good indicator). Look into maybe overriding __str__ for this purpose.
  • Consider maybe having manager be a property instead of having a set_manager(). Then the setter for it can ensure that the employee is added to the manager's emp_under_sup list.
  • You have duplicate behavior for managers and supervisors. Are they the same? If not, consider multiple inheritance or having Manager inherit the supervisee list and methods from Supervisor
  • is_workday() doesn't make sense as a classmethod on Employee. This should probably be on its own.
  • import time; should be import time
  • Setting self.start_time = time.strftime("%D @ %I:%M:%S %p") in the constructor is rather inflexible and doesn't allow for a different start time (say if you load the employee from a file that started a few years ago). Instead try taking start_time as a parameter to the constructor. Then perhaps add a @classmethod called create, which accepts first, last, id, pay but passes time.strftime("%D @ %I:%M:%S %p") as the start_time into the constructor.
  • Your use of class variables seems glitchy and feels like it doesn't what you wanted. Avoid these where possible.
  • Your serialization straight up doesn't work. It doesn't indicate the object type that it has printed out, so you can't tell from a string whether it was an Employee or a Manager that was serialized. Instead of trying to do this yourself, why not use a builtin solution like pickle. From this, it's pretty clear you haven't tested this code. Because this just won't work. Test your code!
  • Your repr can't be "used to recreate Employee object." It gives 3 arguments but the Employee constructor takes 4 (and the last 2 are presumably not strings). Try: return f'Employee({repr(self.first)}, {repr(self.last)}, {repr(self.id)}, {repr(self.pay)})'
  • Why store the start_time as a string. Use datetime.datetime.now() to give you datetime object which allows you to do math and interrogate (ex. you can compute how many years the employee has been with the company with datetime.datetime.now() - start_time)

With all of this, I'd try reformatting your code like so:

import datetime


class Employee:
    @classmethod
    def create(cls, id, first_name, last_name, salary):
        start_time = datetime.datetime.now()
        return cls(id, first_name, last_name, salary, start_time)

    def __init__(self, id, first_name, last_name, salary, start_time):
        self.id = id
        self.first_name = first_name
        self.last_name = last_name
        self.salary = salary

        self._manager = None

    @property
    def full_name(self):
        return f'{self.first_name} {self.last_name}'

    def give_raise(self, amount):
        self.salary += amount

    @property
    def manager(self):
        return self._manager

    @manager.setter
    def manager(self, manager):
        # The manager sets self._manager for us
        manager.add_direct_report(self)

    @manager.deleter
    def manager(self):
        # The manager None's self._manager for us
        self._manager.remove_direct_report(self)


class Manager(Employee):
    @classmethod
    def create(cls, id, first_name, last_name, salary, direct_reports=None):
        start_time = datetime.datetime.now()
        return cls(id, first_name, last_name, salary, start_time,
                   direct_reports)

    def __init__(self, id, first_name, last_name, salary, start_time,
                 direct_reports=None):
        super().__init__(id, first_name, last_name, salary, start_time)
        self.direct_reports = [] if direct_reports is None else direct_reports

    def add_direct_report(self, employee):
        self.direct_reports.append(employee)
        employee._manager = self

    def remove_direct_report(self):
        self.direct_reports.remove(employee)
        employee._manager = None


class Developer(Employee):
    @classmethod
    def create(cls, id, first_name, last_name, salary, programming_language):
        start_time = datetime.datetime.now()
        return cls(id, first_name, last_name, salary, start_time,
                   programming_language)

    def __init__(self, id, first_name, last_name, salary, start_time,
                 programming_langauge):
        super().__init__(id, first_name, last_name, salary, start_time)
        self.programming_lanague = programming_language

It's up to you to properly document this and take another stab at some of the things I excluded (like your borked raise_amount logic and global employee counter).

\$\endgroup\$
  • 1
    \$\begingroup\$ I am working to understand everything you suggested. Thanks, btw for taking the time to help me out. \$\endgroup\$ – Rob Jul 11 '18 at 20:09
  • \$\begingroup\$ I was confused about the 'property' tag. For instance, I never meant for raise_amount to be changeable but fixed; variable, for each type of employee but unchangeable. Unfortunately, where I work we have a coding standard that sets all indentations to two spaces so I can't write code that is completely PEP8 compliant. I will post my edited version of the program based on your suggestions. I am not sure I hit on all the points you made so if you wouldn't mind taking another look that would be great! \$\endgroup\$ – Rob Jul 11 '18 at 20:53
  • \$\begingroup\$ With flake8 you can disable specific rules (so flake8 --ignore=E111 will suppress errors about your 2-space tabs). Whether you wanted raise_amount to be changeable new_raise definitely should not have been a @property (and I'd give it a better name like give_raise). The reason why I removed the raise_amount class variable is it didn't make much sense (both logically and from a coding style perspective). Does adding more developers really increase all of their raises? Even if it does, this seems much better suited for a custom collection... \$\endgroup\$ – Bailey Parker Jul 12 '18 at 2:47
  • \$\begingroup\$ Say you made an EmployeeList that just wraps a regular list. It keeps track of how many employees are in the list and provides a method give_all_raises() which calls give_raise() on each employee with the proper amount (based on how many employees are in the list). \$\endgroup\$ – Bailey Parker Jul 12 '18 at 2:48

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.