Add string to array based on flag

Question

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<string> roles = new List<string>();

        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";
}

mjolka · Accepted Answer · 2015-02-13 09:29:47Z

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<UserRole>, we can write

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;
    }
}

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<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);
}

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

Actually i modified the code a bit so that i don't leak the company code to online community, thank for the dictionary techniquies :D — Sarawut Positwinyu
– Sarawut Positwinyu, Commented Feb 13, 2015 at 10:43

RubberDuck · Accepted Answer · 2015-02-13 11:59:38Z

This is similar advice to what you've already received, but I'd like to expand on why it would be better to return an IEnumerable.

First off, it's always better to code to abstractions rather than implementations. Array is an implementation of IEnumerable.

[SerializableAttribute]
[ComVisibleAttribute(true)]
public abstract class Array : ICloneable, 
    IList, ICollection, IEnumerable, IStructuralComparable, IStructuralEquatable

MSDN Array class

By returning an IEnemerable your code becomes less brittle. It becomes easier to change the code to return a List without breaking the client code.

And returning a list makes a lot of sense here. This is the very first line of code that you have.

List<string> roles = new List<string>();

And then you "cast" it to an Array when you return it.

return roles.ToArray();

Returning an IEnumerable lets you simply return the List.

Side note:

Using var to declare the roles variable also makes sense. It reduces repetition and will make it so there's one less change to make if you decide to change the implementation.

var roles = new List<string>();

Community · Accepted Answer · 2017-05-23 12:40:50Z

3

Expanding on the answer by @mjolka:

You could also use a [Flags] enum.

[Flags]
public enum Role {
    Admin,
    Read,
    Write,
}

something.Role = Role.Read | Role.Write;
// ...
if(something.Role.HasFlag(Role.Read)) {
    // ...
}

edited May 23, 2017 at 12:40

CommunityBot

1

answered Feb 16, 2015 at 14:41

ANeves

2,98218 silver badges29 bronze badges

Add a comment |

Stack Exchange Network

Add string to array based on flag

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Add string to array based on flag

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions