current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite
1

I have been trying out some stuff using inheritance with classes in Python 3.4. In the code below, I use the super() call to return the __str__ from the parent class. Is it correct coding practice to save the information returned by the parent class method in a variable, concatenate the __str__ from the child class to that of the parent class, and return like I did twice below?

Also, what would be best coding practice to convert such output to a string? Would it be better to

  1. Do this also in the parent class.
  2. Also in the child class.
  3. When printing - in this case - Director or Manager?
class Employee:
   def __init__(self, name, salary, position):
      self._name = name
      self._salary = salary
      self._position = position

   def __str__(self):
      return str(self._name) + ' ' + str(self._salary) + ' ' + str(self._position)

class Director(Employee):
   def __init__(self, name, salary, position, mngteam):
      super().__init__(name, salary, position)
      self.managementteam = mngteam

   def __str__(self):
      st = super().__str__() # IS THIS GOOD CODING PRACTICE?
      st += ' ' + str(self.managementteam)
      return st

class Manager(Employee):
   def __init__(self, name, salary, position, supervises):
      super().__init__(name, salary, position)
      self.supervises = supervises

   def __str__(self):
      st = super().__str__() # IS THIS GOOD CODING PRACTICE?
      st += ' ' + str(self.supervises)
      return st

d = Director('DirMax', 100000, 'Director', 5)
print(d)
m = Manager('ManSam', 50000, 'Manager', 10)
print(m)
share|improve this question

1 Answer 1

up vote 10 down vote accepted

In general, calling the parent class implementation, keeping the result and then processing it is perfectly acceptable OOP practice. However, there is a problem with your string handling; concatenation (+) is not the most efficient way of doing things (e.g. a + b + c becomes (a + b) + c, creating intermediate strings that aren't needed). Instead, when implementing __str__ and __repr__, you can use str.format, for example:

class Employee:

   ...

   def __str__(self):
      return '{0._name!s} {0._salary!s} {0._position!s}'.format(self)


class Director(Employee):

   ...

   def __str__(self):
      return '{0} {1.managementteam}'.format(super().__str__(), self)

You aren't entirely compliant with the style guide; for example, managementteam/mngteam should really be management_team. Additionally, you have an odd mix of private-by-convention (leading underscore, e.g. _salary) and public (no leading underscore, e.g. supervises) instance attributes - why? Should these be protected? Should the instance be immutable? Some docstrings would be nice, too.

I would be inclined to add some validation of inputs - for example, if salary should always be a positive integer, you could add checking for that. One way to do that is in __init__, but a more thorough approach is with a property:

class Employee:

    def __init__(self, ..., salary, ...):
        ...
        self.salary = salary  # note assignment to salary not _salary
        ...

    ...

    @property
    def salary(self):
        return self._salary

    @salary.setter
    def salary(self, new_salary):
        if new_salary < 0:
            raise ValueError('salary must be positive')
        self._salary = new_salary
share|improve this answer
    
Great comment jonrsharpe! Thanks! I will read more about properties etc. For now, why would it be a bad habit to name class attributes with a preceding underscore? Doesn't this mean that this attribute should be treated as private? – user78265 Jul 19 at 18:34
1  
@tufyjhkvh yes, the leading underscore means private-by-convention. But it's not clear from this code why they should be private (again, docstrings would be handy!) Should they be read-only, for example (this can also be achieved with a property)? – jonrsharpe Jul 19 at 18:35
    
Hi @jonrsharpe. I've read some documentation regarding your comment. Your comment was very helpful, thanks! Still one remaining question, what is the property decorator for? I haven't been able to find much useful documentation about this.. Thanks! :) – user78265 Jul 20 at 9:31
    
It's a way to customise access to specific attributes; see e.g. stackoverflow.com/a/17330273/3001761 – jonrsharpe Jul 20 at 9:36
    
Thanks @jonrsharpe! – user78265 Jul 20 at 9:58

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.