10
\$\begingroup\$

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));
    }
}
\$\endgroup\$
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ AccessControl is a model and comes from the SDK/API, so no changing that one. CallerMemberName came after a lot of the properties; it's on my list to update the older ones. Also, all of this logic is already in the server-side. I need it client-side solely for UI/design prettyness. I'm going to look more into a single method, but I think I'll run into issues where some properties get reset incorrectly. \$\endgroup\$ – FUR10N May 12 '16 at 15:03
  • \$\begingroup\$ In my one new method, if (CanManagePermissions) CanDownload=true; if (!CanDownload) CanManagePermissions=false; I need both rules but now I cannot uncheck CanDownload anymore. The ordering of the checks is very dependent on who triggered the call. I feel like I'm looking at this wrong. \$\endgroup\$ – FUR10N May 12 '16 at 15:04
  • \$\begingroup\$ Fair point. In that case you have to sacrifice something. You either keep your current approach and sacrifice readability. Or you reduce the complexity of your dependencies and use single method. For example you could eliminate the last use case completely, by disabling CanDownload checkbox when CanManagePermissions is set to true. \$\endgroup\$ – Nikita B May 13 '16 at 7:08
  • \$\begingroup\$ You can also change your approach completely (if that is an option). You could create a bunch of valid permission presets ("admin", "guest", "user", etc.) and instead of using a bunch of checkboxes, let user pick an existing preset from the list. Another option is to let user pick whatever the hell they want, but show a warning if they try to apply invalid combination of settings. \$\endgroup\$ – Nikita B May 13 '16 at 7:14
  • \$\begingroup\$ This is 'sorta' the route I'm going to. The other answers change much more than I'm willing to do right now. My one method is going to take in the property name and just switch on the name to carry out side-effects. Not much shorter, but it's easy to follow IMO. Posting as a separate answer for the curious. \$\endgroup\$ – FUR10N May 13 '16 at 11:46
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

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;
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Flags don't work too well with UI layer. Binding a single enum value to a bunch of checkboxes while also validating all the possible combinations is a lot of converting with no actual profit. That being said, using flags in business layer is a good idea. \$\endgroup\$ – Nikita B May 12 '16 at 14:46
  • 1
    \$\begingroup\$ Oh, overlooked that this was in WPF, but bit flags are so universal. In this particular case, the validation would be about checking the sums of the values of the selected checkboxes and making sure that they match the sums of allowed combinations. \$\endgroup\$ – RWRkeSBZ May 12 '16 at 14:51
  • \$\begingroup\$ Yeah, checking the sums is clever, I like it! \$\endgroup\$ – Nikita B May 12 '16 at 15:01
  • \$\begingroup\$ You should fix your code though: in your example user.Permissions == Permissions.CanView will return false. You should use HasFlag method when checking for flags. \$\endgroup\$ – Nikita B May 12 '16 at 15:02
  • \$\begingroup\$ Ooh didn't think about using flags for this! I'll investigate that more; not quite sure how it will work when you add in account prefs and enabled/disabled-ness. \$\endgroup\$ – FUR10N May 12 '16 at 15:12
1
\$\begingroup\$

Probably not the Modern (tm) 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.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

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));
    }
}
| improve this answer | |
\$\endgroup\$

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.