views:

450

answers:

7

How would you write this block of code?

A user can have 3 levels of permission:

Suspend Lock Ban

We have to decide if we want to delete the users account automatically.

The business rules are as follows:

Delete User if: suspend + lock + ban

Warning Message if: suspend + lock

So how would you write this code block? Keeping code readability and maintainability in mind.

+1  A: 

I would create 3 predicates, like

Boolean canLoginIntoSuperUser()
Boolean canLoginIntoManagerSession()

And then then put the code for a check (e.g. return isOwner() && isManager() && isManager for the superuser).

This way you got the actual check just in one place which makes it maintainable and the referring checks are very readable.

Offcourse you could create a enum with the LoginSections, but there it would it not be possible to have different states for Manager Section and Super User section (i.e. every Super User Section user has implicit access to the Manager Section, maybe you want to change that later).

flolo
with your edit, I would keep it the same, just the names would change for your example (i.e. shouldBeDeleted(), shouldBeWarned())
flolo
A: 

Usually it would be a case where you would just use if, elseif, elseif, else statements.

But I would just not code permissions like that inside a software. It is a lot easier to think in features in a software and roles should reflect features. So:

A Role data structure. (the features) A RoleGroup data structure. (to map stuff like user, manager, etc. to features)

Then it becomes very easy, you won't have to code switches everywhere, just a simple if.

Adding to that, a lot of people will also use bit wise operations on bit fields.

Loki
+1  A: 

I'd write this:

[Flags]
public enum UserLevel {
    None = 0,
    Suspend = 1,
    Lock = 2,
    Ban = 3
};

public class UserStatus {
    UserLevel _level;

    public bool IsDeletable {
        get {
            return Suspended && Locked && Banned;
        }
    }
    public bool Suspended {
        get { return (_level & UserLevel.Suspend) != 0; }
        set { _level = (_level  & ~UserLevel.Suspend) | value ? UserLevel.Suspend : UserLevel.None; }
    }
    // etc for other props

}

plinth
You are missing a closing ')' in the second snippet :)
leppie
heh - I just rewrote to match the rewrite in the question...
plinth
You broke it! Your enum values are in correct! (FlagsAttribute does not enforce anything)
leppie
A: 

something like

[Flags()]
enum Things
{
 Suspend,Lock,Ban
}

public Foo(Things userFlags)
{
 switch(userFlags)
 {
  case Things.Suspend & Things.Lock & Things.Ban:
   //delete
   break;
  case Things.Suspend & Things.Ban:
   //somethign else
   break;
  // yada yada yada

 }
}

This is something you can grow with over time and is more readable

keithwarren7
That will lead to an unmaintainable mess. What happens if you add a new 'flag'?
leppie
You also need to assign proper values for the enum.
leppie
as I told James, I just wrote it out real quick, forgot the values.As far as an unmaintainable mess, I just dont think so. He wants specific actions for specific combinations; new flags could introduce many new combos any way you code it.
keithwarren7
I thought the [Flags] took care of assigning those automatically?
Treb
Ok, if you dont believe me, try it yourself! Taking James's 'corrections' into consideration, a 'case' is not going to test for bits, it will matched to value. Now add some arb flag, this can be on or off, but if it is on, your code will break. The case will not be matched. Understand?
leppie
This is bad, the logic is in the client rather than encapsulated along with the data. Use a class instead of an enum.
Wedge
Wedge
+4  A: 

I would create the following enum:

[Flags]enum UserLevel 
{ None=0, Suspend=1, Lock=2, Ban=4, 
  Delete=Suspend | Lock | Ban,  // equililent to 7
  Warn=Suspend | Lock }         // equililent to 3

Then you can use bitwise enum comparisons

if ((user.Level & UserLevel.Warn) == UserLevel.Warn) 
     user.Warn();
else if ((user.Level & UserLevel.Delete) == UserLevel.Delete) 
     user.Delete();
Charles Bretana
Yay, someone finally gets it right! +1 :)
leppie
Um - not really - if the underlying implementation of your rules change, this gets much worse. Better to have predicate methods that are allow your code to be detached from the implementation.
plinth
This is decent, but I agree with plinth, you want to encapsulate/abstract all of this logic, including the enums, inside a class or some such. If you have to twiddle bit flags in client code, you're doing it wrong.
Wedge
+1  A: 

keithwarren7 had the best answer, although he missed two key points: First, the enums need to be set to bit position values. Next the values for the case statement must be added or OR'd. If they are AND'd they'll all result in zero:

case Things.Suspend | Things.Lock | Things.Ban:

Putting that with some other good ideas we have here, we get:

[Flags]
enum Things
{
 None = 0,
 Suspend = 1,
 Lock= 2,
 Ban = 4,
 Delete = Suspend | Lock | Ban,  
 Warn = Suspend | Lock   
}

public bool IsDeletable(this User user)
{ 
    return ((user.Level & UserLevel.Delete) == UserLevel.Delete) ;
}
// :

if (user.IsDeletable)
    user.Delete();

(Of course, that kind-of assumes you are using C#3, when you never actually mention which language you were using....)

James Curran
yeah, sorry - I was quickhanding, no compiler checks to call me stupid :)
keithwarren7
+9  A: 

A lot of people are fond of bitfields. I am not. I believe the solution to be more readable and maintainable when you use simple boolean properties.

class User 
{
    public bool IsSuspended { get; set; }
    public bool IsLocked { get; set; }
    public bool IsBanned { get; set; }
    public bool ShouldDelete { get { return IsSuspended && IsLocked && IsBanned; } }
    public bool ShouldWarn { get { return IsSuspended && IsLocked; } }
}
Matt Brunell
+1. This is the "KISS" code.
Paul Nathan
All of the enum based solutions are messy. You are mixing attributes with actions and on top of that using cryptic bitwise manipulation to model business rules is not too good.
JC
+1 This is an excellent solution. I would rename the class to AccountPermissions or something more specific than the generic "User". Also, it might make sense for the flags to have explicit default values.
Wedge
The thing I like about using Enums is that you can easily add a new permission without having to update the database and sprocs (insert/update) and the associated DAL. (both implementations will have to update the BLL).
Blankman