Problem
The purpose of the class is to set permissions for a user. These include delete, download, upload, view, etc. These properties often depend on other properties, and setting them has side effects.
For example, if I set CanUpload=false, then I need to also set CanManagePermissions=false since you cannot be an admin without upload permission. Likewise, if I set CanManagePermissions=true, then I need to set delete, download, and upload to true. Plus, you can’t have CanDownload permission without also having CanView permission, so I set that too. But you can have CanView without CanDownload.
You can see this starts to get a bit convoluted. Especially when you through in account preferences that relax/tighten the relationships and force disable some of the permissions (which is important since I’m binding this to UI and want to enable/disable checkboxes).
The code works as-is, but it is very difficult to maintain and understand all of the relationships and side-effects. If someone is new to the code it can be a trip trying to figure out how properties depend on and change other properties (even more so when you get to doubly nested dependencies like CanManagePermissions->CanDownload->CanView).
Can I make the relationships any clearer or easier to work with? I’m assuming it’s more than just an issue of documentation, but maybe not.
One thought was to have one big method that all of the setters call at the end. That one method would enforce all of the relationships. Not sure if this can satisfy every condition or if it’s a good idea though.
public class AccessControlViewModel : INotifyPropertyChanged
{
private AccountPreferences Preferences => CurrentUser.Account.Preferences;
private bool AllowDownloadNotificationsWithoutAdmin => Preferences.AllowDownloadNotificationsWithoutAdmin;
private bool ShowDownloadLinkInUploadNotification => Preferences.ShowDownloadLinkInUploadNotification;
public AccessControl AccessControl { get; private set; }
public bool HasChanged { get; private set; }
public bool CanUpload
{
get
{
return AccessControl.CanUpload.GetValueOrDefault();
}
set
{
if (AccessControl.CanUpload != value)
{
AccessControl.CanUpload = value;
RaisePropertyChanged(nameof(CanUpload));
if (!value)
{
CanManagePermissions = false;
if (EnableViewOnly)
{
CanView = true;
}
else
{
CanDownload = true;
}
}
HasChanged = true;
RaisePropertyChanged(nameof(PermissionSummary));
}
}
}
public bool CanDownload
{
get
{
return AccessControl.CanDownload.GetValueOrDefault();
}
set
{
if (AccessControl.CanDownload != value)
{
AccessControl.CanDownload = value;
RaisePropertyChanged(nameof(CanDownload));
if (!value)
{
CanManagePermissions = false;
if (!EnableViewOnly)
{
CanView = false;
}
if (!EnableNotifyOnUpload)
{
NotifyOnUpload = false;
}
}
else
{
CanView = true;
}
HasChanged = true;
RaisePropertyChanged(nameof(EnableNotifyOnUpload));
RaisePropertyChanged(nameof(PermissionSummary));
}
}
}
public bool CanDelete
{
get
{
return AccessControl.CanDelete.GetValueOrDefault();
}
set
{
if (AccessControl.CanDelete != value)
{
AccessControl.CanDelete = value;
RaisePropertyChanged(nameof(CanDelete));
if (!value)
{
CanManagePermissions = false;
}
else
{
CanDownload = true;
CanUpload = true;
}
HasChanged = true;
RaisePropertyChanged(nameof(PermissionSummary));
}
}
}
public bool EnableViewOnly => Preferences.EnableViewOnly;
public bool CanView
{
get
{
return AccessControl.CanView.GetValueOrDefault();
}
set
{
if (AccessControl.CanView != value)
{
AccessControl.CanView = value;
if (!value)
{
CanDownload = false;
CanUpload = true;
}
if (!EnableNotifyOnUpload)
{
NotifyOnUpload = false;
}
HasChanged = true;
RaisePropertyChanged(nameof(CanView));
RaisePropertyChanged(nameof(EnableNotifyOnUpload));
RaisePropertyChanged(nameof(PermissionSummary));
}
}
}
public bool CanManagePermissions
{
get
{
return AccessControl.CanManagePermissions.GetValueOrDefault();
}
set
{
if (AccessControl.CanManagePermissions != value)
{
AccessControl.CanManagePermissions = value;
RaisePropertyChanged(nameof(CanManagePermissions));
if (value)
{
CanDelete = true;
CanUpload = true;
CanDownload = true;
}
else if (!EnableNotifyOnDownload)
{
NotifyOnDownload = false;
}
HasChanged = true;
RaisePropertyChanged(nameof(EnableNotifyOnDownload));
RaisePropertyChanged(nameof(PermissionSummary));
}
}
}
public bool EnableNotifyOnUpload => CanDownload || (CanView && !ShowDownloadLinkInUploadNotification);
public bool NotifyOnUpload
{
get
{
return AccessControl.NotifyOnUpload.GetValueOrDefault();
}
set
{
if (NotifyOnUpload != value && CanDownload)
{
AccessControl.NotifyOnUpload = value;
HasChanged = true;
}
else
{
AccessControl.NotifyOnUpload = false;
}
RaisePropertyChanged(nameof(NotifyOnUpload));
}
}
public bool EnableNotifyOnDownload => CanManagePermissions || AllowDownloadNotificationsWithoutAdmin;
public bool NotifyOnDownload
{
get
{
return AccessControl.NotifyOnDownload.GetValueOrDefault();
}
set
{
if (NotifyOnDownload != value)
{
AccessControl.NotifyOnDownload = value;
if (value && !AllowDownloadNotificationsWithoutAdmin)
{
CanManagePermissions = true;
}
HasChanged = true;
RaisePropertyChanged(nameof(NotifyOnDownload));
}
}
}
public string PermissionSummary
{
get
{
//...Returns a string based on the above properties...
}
}
public event PropertyChangedEventHandler PropertyChanged;
private void RaisePropertyChanged([CallerMemberName]string propertyName = "")
{
PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
}
}
Solution
I would definitely recommend moving all the logic from setters to single ValidatePermissions method, which should check every permission and, if the current state is invalid, correct them. This way it will be way easier to understand and maintain all the dependencies.
You should also consider moving validation logic to your business layer, if having correct permissions setting is critical for your application. More often than not those settings are stored somewhere (DB, .ini file, cloud, etc.), and you don’t want your application to break, just because those setting for whatever reason were modified outside your application.
Also, you don’t have to use nameof() operator if you are using CallerMemberName attribute. You can just call RaisePropertyChanged() (without arguments).
AccessControl property is fishy. If it is actually a wpf control, than this is a major violation of MVVM. Your viewmodels should never have a direct access to view, thats what databinding is for. If it is not a UI control, then IMHO you should rename it, removing the Control word from both class name and property name. AccessManager sounds good to me.
It appears that business logic (” I set CanUpload=false, then I need to also set CanManagePermissions=false since you cannot be an admin without upload permission”) is being mixed up with the user interface presentation.
Your AccessControlViewModel should just be a strongly typed bag of data representing what the user has chosen on a form. After pressing a button, you should then validate the entries in the form.
After validation has passed, you really need a collection of classes that represents the “Problem Domain” – managing permissions, like say a User class:
public class User
{
public bool CanUpload { get; private set; }
public bool CanManagePermissions { get; private set; }
public void CanAllowUploads
{
get { return CanManagePermissions; }
}
public void AllowUploads()
{
if (!CanAllowUploads)
throw new InvalidOperationException("Cannot allow uploads without being able to manage permissions");
CanUpload = true;
}
public void DisallowUploads()
{
CanUpload = false;
}
public void AllowPermissionManagement()
{
CanManagePermissions = true;
}
public void DisallowPermissionManagement()
{
CanUpload = false;
CanManagePermissions = false;
}
}
Here the relationship between uploading and managing permissions is enforced.
User user = new User();
user.AllowUploads(); // Throws InvalidOperationException
user.AllowPermissionManagement();
user.AllowUploads(); // No exception gets thrown
user.CanUpload // Is True
user.CanManagePermissions // Is True
user.DisallowPermissionManagement();
user.CanUpload // Is False
user.CanManagePermissions // Is False
You really need a separate of UI logic from Business Logic.
Instead of using bool properties, use bit flags with enumeration types. Just make sure that the values assigned to the different [permission] flags are in increasing powers of 2 (e.g. 0, 1, 2, 4, 8, 16, etc.)
enum Permissions { None = 0, CanView = 1, CanDownload = 2, CanManagePermissions = 4 }
class User { public Permissions Permissions { get; set; } }
...
user.Permissions = Permissions.CanView | Permissions.CanDownload;
Probably not the Modern ™ approach, but I would set this these up with two layers: base permissions and effective or resolved permissions. You could implement this by having the get{} for a property check the dependencies of the current property with an AND (&&) to each property that it depends on.
This is what I’m going with. I changed all of the properties to be simple get/set with a backing field, and then listen to PropertyChanged to do the side-effects. I think it’s more readable (enough so that I’m happy with it).
private void AccessControlViewModel_PropertyChanged(object sender, PropertyChangedEventArgs e)
{
var setHasChanged = false;
switch (e.PropertyName)
{
case nameof(CanView):
{
if (!CanView)
{
CanDownload = false;
CanUpload = true;
}
setHasChanged = true;
RaisePropertyChanged(nameof(EnableNotifyOnUpload));
break;
}
case nameof(CanDownload):
{
if (CanDownload)
{
CanView = true;
}
else
{
NotifyOnUpload = false;
CanManagePermissions = false;
if (!EnableViewOnly || ShowDownloadLinkInUploadNotification)
{
CanUpload = true;
}
else if (!CanUpload)
{
CanDelete = false;
}
}
setHasChanged = true;
RaisePropertyChanged(nameof(EnableNotifyOnUpload));
break;
}
case nameof(CanUpload):
{
if (!CanUpload)
{
if (!EnableViewOnly)
{
CanDownload = true;
}
if (!CanDownload)
{
CanDelete = false;
}
CanView = true;
CanManagePermissions = false;
}
setHasChanged = true;
break;
}
case nameof(CanDelete):
{
if (CanDelete)
{
CanView = true;
CanDownload = true;
}
else
{
CanManagePermissions = false;
}
setHasChanged = true;
break;
}
case nameof(CanManagePermissions):
{
if (CanManagePermissions)
{
CanView = true;
CanDownload = true;
CanUpload = true;
CanDelete = true;
}
setHasChanged = true;
RaisePropertyChanged(nameof(EnableNotifyOnDownload));
break;
}
case nameof(NotifyOnDownload):
case nameof(NotifyOnUpload):
{
setHasChanged = true;
break;
}
}
if (setHasChanged)
{
HasChanged = true;
if (!EnableNotifyOnDownload)
{
NotifyOnDownload = false;
}
if (!EnableNotifyOnUpload)
{
NotifyOnUpload = false;
}
RaisePropertyChanged(nameof(PermissionSummary));
}
}