current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

Is there anyway to improve this kind of code ?

    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.Write);
        }
        if (this.IsEntityUser)
        {
            roles.Add(USER_ROLES.IsEntityUser);
        }

        return roles.ToArray();
    }
share|improve this question
2  
Can you add more context to your question ? Like what is this, what is Dto ? –  Heslacher 15 hours ago
    
Can you show how the enum is defined? –  Jeroen Vannevel 8 hours ago

2 Answers 2

up vote 6 down vote accepted

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.

share|improve this answer
    
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 14 hours ago
up vote 2 down vote

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>();
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.