trjhtr

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)

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).