patterncsharpModerate
Add string to array based on flag
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 writepublic 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.