what is the best way to write a recurring code with different enums

Question

Basically what this code does is to determine whether the user is allowed access to the page or not from list of pages as enum, each one has the role name as its name and the page name inside it.

The problem is that I don't think this is the best practice to write this code. why? basically if a new role is added I'd have to write an if condition for it which doesn't feel right. so if there is anyway to make this cleaner I would like to know it.

private static bool AllowPageAccess(int roleID, string url)
{
    string cleanedUrl = ExtractPageNameFromUrl(url);

    if (roleID == (int)Roles.Admin)
        foreach (string pageName in Enum.GetNames(typeof(AdminPages)))
            if (pageName == cleanedUrl)
                return true;

    if (roleID == (int)Roles.CompanyOwner)
        foreach (string pageName in Enum.GetNames(typeof(CompanyPages)))
            if (pageName == cleanedUrl)
                return true;

    if (roleID == (int)Roles.Customer)
        foreach (string pageName in Enum.GetNames(typeof(CustomerPages)))
            if (pageName == cleanedUrl)
                return true;

    if (roleID == (int)Roles.Guest)
        foreach (string pageName in Enum.GetNames(typeof(EveryonePages)))
            if (pageName == cleanedUrl)
                return true;

    return false;
}

Answer 1

if a new role is added I'd have to write an if condition for it which doesn't feel right

If a new role is added, let's call it NewRole, then you'll also have to create a NewRolePages enum. That's always going to mean you have to write something new. Whether it's an if condition, a new entry in a mapping table, a new handler, ... the used pattern can change but the fact that you need to add something doesn't.

However, I think there's a more central problem here. For a given page, it's reasonable that multiple roles can access it, and thus you're going to end up with the same page being mentioned multiple times: once in each FooPages for every Foo role that should have access.

This violates DRY, as you're now stuck with having to synchronize multiple enums should a pagename ever change. You're effectively abusing your enums as a list of strings. Presumably, you're doing so because you want Intellisense, but that's really not the way to go about it.

A much more reusable approach would be to refactor your roles as a Flags enum, and create a mapping between the pagename and the combined enum.

[Flags]
public enum Roles
{
    Admin = 1,
    CompanyOwner = 2, 
    Customer = 4, 
    Guest = 8, 

    Everyone = 15,  // is the same as combining the above 4 roles, since 15 = 8+4+2+1
}

This means you can combine the values:

Role rolesForSettingsPage = Role.Admin | Role.CompanyOwner;

However, you want to map these (combined) enums to a string value. For that, you can set up a dictionary:

private Dictionary<string, Role> _pageRoles = new Dictionary<string, Role>()
{
    { "Welcome", Role.Everyone },
    { "Settings", Role.Admin | Role.CompanyOwner },
    { "CustomerInfo", Role.Admin | Role.Customer },
    { "GuestPage", Role.Guest }
}

And then you can use the dictionary to check if the given role has access to the given page

public bool RoleCanAccessPage(Role userRole, string url)
{
    string pageName = ExtractPageNameFromUrl(url);

    return _pageRoles.ContainsKey(pageName)  //Does this page have a mapping?
           &&
           (_pageRoles[pageName] & userRole) != 0;  //Does the user have at least one of the mapped roles
}

This also gives you the ability for a user to have multiple roles, but that is optional and can be ignored if you don't need it.

Additionally, notice that in this example you do not have copy/pasted data such as the page name.

Note
My answer sticks as close to your intended string-to-enum mapping as reasonably possible. There are other ways of doing this, but they require bigger architectural changes and would take more time and effort to showcase/explain/implement. My answer is simply one of many possible answers.

Note 2
If you really wanted to, you could still create a Page enum which contains all pages (not just separated per role!), and then your dictionary can be changed to be a Dictionary<Page, Roles>().

---

Minor comments and improvements

These comment aren't applicable to my suggested improvement but they are still good tips nonetheless

• For good practice, enums should have a singular name (e.g. Page). The only exception here are flags enums, where the plural is preferred (e.g. [Flags] Roles`).
• As a rule of thumb, avoid unnecessary casting of string/int values when dealing with these enums. Don't use int roleID. Enums exist for a reason, so use them: Role userRole. This means you don't need to cast, and the code that calls your method will be much more readable.
• Use a switch instead of separate if statements.
• You can use LINQ's .Any() instead of foreach. For your code, the foreaches could be changed to Enum.GetNames(typeof(AdminPages)).Any(name => name == cleanedUrl)
• AllowPageAccess is not a good method name. You're returning a boolean, so your method should be asking a question. AllowPageAccess would be the name of a method with does (sets) something. IsPageAccessAllowed would be the name of a method which returns a boolean indicating if access to the page is allowed.

Answer 2

If you have the option to change the architecture away from Enums, you could add a class to handle your logic. Here's a (very basic) example of what this could look like.

public class MyPage
{
    public string Url { get; }
    public int AccessLevel { get; }

    public MyPage(string url, int accesslevel)
    {
        Url = url;
        AccessLevel = accesslevel;
    }

    public bool UserCanAccess(int roleId)
    {
        return roleId >= AccessLevel;
    }
}

Now assume you have a list of pages setup like this:

List<MyPage> pages = new List<MyPage>()
{
    new MyPage("test1", 1), // Allow guests and up
    new MyPage("test2", 2), // Allow Customers and up
    new MyPage("test3", 3), // Allow CompanyOwners and up
    new MyPage("test4", 4), // Allow Admins
    new MyPage("test5", 2)
};

Your AllowPageAccess method would look like this:

return pages.FirstOrDefault(p => p.Url == url).UserCanAccess(roleId);

Answer 3

You have an enum with a series of type values and a number of classes, each of which corresponds to exactly one enum value. But the association between concrete class and enum value is not explicit, therefore you have to repeat an entire block of code when you want to actually use this associtation.

What you should do is either maintain a map that maps enum values to handler classes, or (even better) reference the appropriate handler class directly in the definition of your enum values. Either way allows you to write the foreach ... typeof code only once.