2

I want to ask:

  1. Whether the Role class violates Single Responsibility Principle in SOLID ? I think deleteAccount() is not belong to Role class but Role class is way to extend code in the future

  2. There are deleteAccount() in Role and Account. That is ok ?

  3. Admin and Moderate violates DRY principle ? Because the deleteAccount() was repeated. If yes, how to fix it ?

  4. How to implement the deleteAccount() which can delete itself account ? Should I use strategy design pattern to do that ?

     class Account:
        def __init__(self, email, password, security_ans):
            self.status = "Active"
            self.role = Member()
    
    
        def deleteAccount(self, dest_email):
            dest_account = accounts.getByEmail(dest_email) #get account obj from accounts 
            self.role.deleteAccount(self)
    

This is my Role class:

class Role:
def blockAccount(self, account):
    raise NotImplementedError("No Permission")

class Admin(Role):
    def deleteAccount(self, account_obj):
        account_obj.setStatus('Closed')

class Moderator(Role):
    def deleteAccount(self, account_obj):
        account_obj.setStatus('Closed')

class Member(Role):
    def deleteAccount(self, account_obj):
        raise NotImplementedError("No Permission") 
4

Yes, your Role class is violating the Single Responsibility Principle, because it has knowledge how to delete an account. That knowledge should be reserved to the Account class itself.

The knowledge the belongs in the Role class is which accounts a user with a particular role is allowed to delete. And a single user can have different roles.

For example, Role could handle the permissions like this:

class Role:
  def canDeleteAccount(self, requester, target):
    return False

class Admin(Role):
  def canDeleteAccount(self, requester, target):
    # Admins can delete anyone
    return True 

class Moderator(Role):
  def canDeleteAccount(self, requester, target):
    # Moderators can only delete Members, not Admins or other Moderators
    return target.is_only_member()

class Member(Role):
  def canDeleteAccount(self, requester, target):
    # Members can only delete themselves
    return requester == target

The account deletion logic in Account itself could look something like this

class Account:
  def deleteAccount(self, dest_email, hard_delete=False):
    dest_account = accounts.getByEmail(dest_email) #get account obj from accounts
    can_delete = any(role.canDeleteAccount(self, dest_account) for role in self.roles)
    if can_delete:
      if hard_delete and (Admin in self.roles or dest_account.reputation() < hard_delete_threshold):
        accounts.delete(dest_account)
      else:
        dest_account.setStatus("Closed") 

Note that I extended the deletion logic to allow a hard deletion (removal from the database) for low-reputation accounts or if the deletion is done by a user with the Admin role. The distinction between a hard and soft deletion is not visible for the Role classes.

Also note that Moderator A can delete itself, if that user also has the Member role.

  • If I want to extend 1 more subclass ToughGuy that only can temporary delete Account by setting status to "Temp_Closed", I have to use a lot of if/else in Account class. It violates O principle in SOLID. So, that is still fine ? – Nguyen Nguyen Jul 31 at 11:35
  • 1
    @NguyenNguyen: I don't subscribe to the strict interpretation of the Open/Closed principle that code must not ever be changed. If requirements change, like adding the feature of temporary deletion, then in my book it is fine to change existing code, as long as you can prove that you didn't break existing functionality that should still work. – Bart van Ingen Schenau Jul 31 at 12:40
  • 2
    @NguyenNguyen: You cannot close against any type of change; the Open/Closed principle must be applied strategically, where it matters - meaning against certain kinds of change, at certain parts of the codebase. If it turns out you made the wrong choices, then you have to redesign at some point. Which is why you want to postpone those choices until the "last responsible moment", when you know enough about the domain and the kinds of changes that are likely to occur, so that you can base the tradeoffs in your design around that knowledge. These principles must not be applied dogmatically. – Filip Milovanović Jul 31 at 15:54
3

DeleteAccount should not be in any of your examples because a role is not an actor, it is a property. A role has a bunch of rights linked to it. It is just a convenience, it allows you to assign a set of rights to a person. With these rights that person is then allowed or not to initiate a delete operation. The delete operation itself should be a method on an AccountManager class that can create, modify and delete accounts.

So your main problem is that your are thinking of roles as persons. They are not, you still need a person class, which could be Member because member is not a role anyway. Just keep member, do not make it descend from role and instead add a list of roles as a property to member.

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.