Does my class violate the Single Responsibility Principle in SOLID?

Question

I want to ask:

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
There are deleteAccount() in Role and Account. That is ok ?
Admin and Moderate violates DRY principle ? Because the deleteAccount() was repeated. If yes, how to fix it ?

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

Bart van Ingen Schenau · Accepted Answer · 2019-07-31 08:16:01Z

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
– Nguyen Nguyen, Commented Jul 31, 2019 at 11:35
@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
– Bart van Ingen Schenau, Commented Jul 31, 2019 at 12:40
@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ć
– Filip Milovanović, Commented Jul 31, 2019 at 15:54

Martin Maat · Accepted Answer · 2019-07-31 06:18:21Z

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.

Stack Exchange Network

Does my class violate the Single Responsibility Principle in SOLID?

2 Answers 2

Hot Network Questions

Does my class violate the Single Responsibility Principle in SOLID?

2 Answers 2

Related

Hot Network Questions