I'm a newbie to coding trying to understand OOP concepts. Recently I came across a code and struggled to understand some lines

class User():
    """Represent a simple user profile."""

    def __init__(self, first_name, last_name, username, email, location):
        """Initialize the user."""
        self.first_name = first_name.title()
        self.last_name = last_name.title()
        self.username = username
        self.email = email
        self.location = location.title()
        self.login_attempts = 0

    def describe_user(self):
        """Display a summary of the user's information."""
        print("\n"   self.first_name   " "   self.last_name)
        print("  Username: "   self.username)
        print("  Email: "   self.email)
        print("  Location: "   self.location)

    def greet_user(self):
        """Display a personalized greeting to the user."""
        print("\nWelcome back, "   self.username   "!")

    def increment_login_attempts(self):
        """Increment the value of login_attempts."""
        self.login_attempts  = 1

    def reset_login_attempts(self):
        """Reset login_attempts to 0."""
        self.login_attempts = 0


class Admin(User):
        """A user with administrative privileges."""
    
        def __init__(self, first_name, last_name, username, email, location):
            """Initialize the admin."""
            super().__init__(first_name, last_name, username, email, location)
    
            # Initialize an empty set of privileges.
            self.privileges = Privileges()
    
class Privileges():
        """A class to store an admin's privileges."""
    
        def __init__(self, privileges=[]):
            self.privileges = privileges
    
        def show_privileges(self):
            print("\nPrivileges:")
            if self.privileges:
                for privilege in self.privileges:
                    print("- "   privilege)
            else:
                print("- This user has no privileges.")
    
    
    eric = Admin('eric', 'matthes', 'e_matthes', '[email protected]', 'alaska')
    eric.describe_user()
    
    eric.privileges.show_privileges()
    
    print("\nAdding privileges...")
    eric_privileges = [
        'can reset passwords',
        'can moderate discussions',
        'can suspend accounts',
        ]
    eric.privileges.privileges = eric_privileges
    eric.privileges.show_privileges()

The following line seems very confusing. If you can suggest a better way to code this I welcome your feedback

  eric.privileges.privileges = eric_privileges

I really appreciate your thoughts on this. Thanks a lot

CodePudding user response：

Regarding the line eric.privileges.privileges = eric_privileges...

It seems indeed confusing to have a class Privileges with a single class variable named privileges. This may be a hint that the class is not really required. Then one possibility would be:

1. Remove the class Privileges

If the privileges are Admin-specific - i.e. assuming the privilege functionality is not used again in other parts of the code - why not use a list for storing all privileges? The show_privileges() method could either be moved to the Admin class or be an independent function. The second option - and the moderate use of classes in general - often leads to code that is easier to maintain and to test.

class Admin(User):    
    def __init__(self, first_name, last_name, username, email, location):
        super().__init__(first_name, last_name, username, email, location)
        self.privileges = []

def show_privileges(privileges):
    print("\nPrivileges:")
    if privileges:
        for privilege in privileges:
            print("- "   privilege)
        else:
            print("- This user has no privileges.")


eric = Admin('eric', 'matthes', 'e_matthes', '[email protected]', 'alaska')
eric.describe_user()
print_privileges(eric.privileges)

print("\nAdding privileges...")
eric_privileges = [
    'can reset passwords',
    'can moderate discussions',
    'can suspend accounts',
    ]
eric.privileges = eric_privileges
show_privileges(eric.privileges)

Perhaps it does make sense to store the privileges in a separate class (e.g. it is supposed to store further variables and provide more functionality). In this case, the current name may not be the best option to represent the class. So another possibility is:

2. Rename the class Privileges

If the class Privileges is required, perhaps renaming it would help to clarify things. E.g., calling it PrivilegeHandler (or what else is appropriate in the context) would lead to:

class Admin(User):    
    def __init__(self, first_name, last_name, username, email, location):
        super().__init__(first_name, last_name, username, email, location)
        self.privilege_handler = PrivilegeHandler()

class PrivilegeHandler:

    def __init__(self, privileges=[]):
        self.privileges = privileges
        # ... 

    def add_privileges(self, new_privileges):
        self.privileges.extend(new_privileges)

    def show_privileges(self):
        print("\nPrivileges:")
        # ...

# ...
eric.privilege_handler.add_privileges(eric_privileges)
eric.privilege_handler.show_privileges()