I have a class named Employee and another (reg_employee) that instantiates the former class.

I want to know if this code can be improved.

class Employee:

    def __init__(self):
        self.Name = ''
        self.Id = 0
        self.Age = 0
        self.company = 0
        self.title = ''
        self.Level = 0
        self.xp = 0.0
        self.Tasks = []
        self.Projects = []

    def setname(self, name):
        self.Name = name

    def setage(self, age):
        self.Age = age

    def setcompany(self, company):
        self.company = company

    def setxp(self, lvlup):
        self.xp = lvlup
import Employee

class reg_employee:

    def __init__(self):
        self.employees = []

    def read_info(self, name, age, company):
        e = Employee.Employee()
        e.Name = name
        e.Age = age
        e.company = company
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$
    – Mast
    Dec 18 '20 at 12:03

The code looks fine. However, there are a few things you can do to improve it:

  • first, avoid using Capital letters for variable. In Python, they should be named in snake_case;
  • use snake_case for setters (e.g., set_name) and getters as well, or use properties instead. See https://www.python-course.eu/python3_properties.php for more information.
  • You can avoid instantiating the variable in the __init__.
  • If you are using Python 3.X, consider specifying the parameters types and return types.

Here is how I would refactor your code:

class Employee:

    def name(self):
        return self.__name

    def name(self, name: str):
        self.__name = name

    def age(self):
        return self.__age

    def age(self, age: int):
        self.__age = age

    def company(self):
        return self.__company

    def company(self, company: str):
        self.__company = company

    def xp(self):
        return self.__xp

    def xp(self, lvlup: float):
        self.__xp = lvlup

However, you should use setters if you need to perform some checks before assigning the value to the variable (e.g., you want to check that the passed age > 18 or that the title is at least "a bachelor degree", and so forth. If you do not need those checks, you can simply call the variable, for example:

class Employee:

    def __init__(self):
        self.name = ''
        self.id = 0
        self.age = 0
        self.company = 0
        self.title = ''
        self.level = 0
        self.xp = 0.0
        self.tasks = []
        self.projects = []

employee = Empolyee()
employee.name = 'Foo Bar'
employee.age = 30

Instead of using setters or populating the variables one by one, you can further improve the code by passing the parameters to the Employee class constructor:

class Employee:

    def __init__(self, name:str, age:int, company:int, id:int=None, title:str=None,
                 xp=float=None, task:list=None, projects:list=None):
        self.name = name
        self.id = id
        self.age = age
        self.company = company
        self.title = title
        self.level = level
        self.xp = xp
        self.tasks = tasks
        self.projects = projects

   # Properties here (getters and setters) ...

Then, read_info will become:

def read_info(self, name, age, company):
    e = Employee.Employee(name=name, age=age, company=company)


As highlighted by hjpotter, a more suitable approach in this case would use dataclasses. Therefore, the class can be rewritten as:

from dataclasses import dataclass

class Employee:

    name: str
    age: int
    company: int
    id: int = 0
    title: str = None
    level: int = 0
    xp: float = 0.0
    tasks: list = None
    projects: list = None

The __init__ is created automatically, and a new object can be instantiated as in the previous example:

def read_info(self, name, age, company):
    e = Employee.Employee(name=name, age=age, company=company)

  • 4
    \$\begingroup\$ I usually tend to use **kwargs when I have to deal with a big number of arguments in a class which seems to be the case here as well. \$\endgroup\$ Dec 18 '20 at 11:40
  • 5
    \$\begingroup\$ in this specific case i think dataclasses are more suitable \$\endgroup\$
    – hjpotter92
    Dec 18 '20 at 12:06

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.