HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Add string to array based on flag

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
arrayflagbasedstringadd

Problem

Is there any way to improve this kind of code? I have converted this from production code for privacy reasons, so there might be some mistakes.

public class AddNewUserDto
{

    public bool IsAdmin { get; set; }
    public bool IsRead { get; set; }
    public bool IsWrite { get; set; }

    public string[] GetRolesByDto()
    {
        List roles = new List();

        if (this.IsAdmin)
        {
            roles.Add(USER_ROLES.ADMIN);
        }
        if (this.IsRead)
        {
            roles.Add(USER_ROLES.IsRead);
        }
        if (this.IsWrite)
        {
            roles.Add(USER_ROLES.IsWrite);
        }
        if (this.IsEntityUser)
        {
            roles.Add(USER_ROLES.IsEntityUser);
        }

        return roles.ToArray();
    }
}

public static class USER_ROLES
{
    public const string IsAdmin= "Admin";
    public const string IsRead= "Read";
    public const string IsWrite= "Write";
    public const string IsEntityUser = "EntityUser";
}

Solution

strings are probably the wrong choice to represent roles. You lose the power of the type system -- the compiler won't pick up on typos, e.g. "Amdin". Consider using an enum instead.

Be consistent with your naming. Why is it USER_ROLES.IsRead and USER_ROLES.Write (not IsWrite)? Stick to the standard naming conventions, e.g. write UserRole.IsRead instead of USER_ROLES.IsRead.

Does the calling code really need an array returned? It would probably be fine with an IEnumerable.

If we change the return type to IEnumerable, we can write

public IEnumerable GetRoles()
{
    if (this.IsAdmin)
    {
        yield return UserRole.Admin;
    }

    if (this.IsRead)
    {
        yield return UserRole.Read;
    }

    if (this.IsWrite)
    {
        yield return UserRole.Write;
    }

    if (this.IsEntityUser)
    {
        yield return UserRole.EntityUser;
    }
}


But I'm guessing you're asking more about the repetition. Here's one way to get around it -- create an association between the booleans and the values. Here I've used a dictionary; it's possibly a bit of overkill but it does give us the nice initialiser syntax:

public IEnumerable GetRoles()
{
    var roles = new Dictionary
    {
        { UserRole.Admin, this.IsAdmin },
        { UserRole.Read, this.IsRead },
        { UserRole.Write, this.IsWrite },
        { UserRole.EntityUser, this.IsEntityUser }
    };
    return roles.Where(role => role.Value)
                .Select(role => role.Key);
}


Other options would be an array of KeyValuePairs, an array of Tuples, etc.

Code Snippets

public IEnumerable<UserRole> GetRoles()
{
    if (this.IsAdmin)
    {
        yield return UserRole.Admin;
    }

    if (this.IsRead)
    {
        yield return UserRole.Read;
    }

    if (this.IsWrite)
    {
        yield return UserRole.Write;
    }

    if (this.IsEntityUser)
    {
        yield return UserRole.EntityUser;
    }
}
public IEnumerable<UserRole> GetRoles()
{
    var roles = new Dictionary<UserRole, bool>
    {
        { UserRole.Admin, this.IsAdmin },
        { UserRole.Read, this.IsRead },
        { UserRole.Write, this.IsWrite },
        { UserRole.EntityUser, this.IsEntityUser }
    };
    return roles.Where(role => role.Value)
                .Select(role => role.Key);
}

Context

StackExchange Code Review Q#80409, answer score: 12

Revisions (0)

No revisions yet.