LinQ question 1

[whosrdaddy]

Hi,

if want to refactor this piece of code using Linq:

 public RolesDTO Roles(int? filterRoleId, int? filterUserId)
        {
            var rolesDto = new RolesDTO
                               {
                                   WithRole = new List<UserRoleDTO>(), 
                                   WithNoRole = new List<UserRoleDTO>()
                               };
            var users = repository.GetAll(false);
            var roleId = filterRoleId ?? 0;
            var userId = filterUserId ?? 0;
            if (users != null)
            {
                foreach (var user in users)
                {
                    var rolefound = user.Username.ToLowerInvariant().Equals("admin");
                    if (user.Roles != null && !rolefound)
                    {
                        foreach (var role in user.Roles)
                        {
                            rolefound = (roleId != 0 && role.RoleId == roleId) || (userId != 0 && role.UserId == userId);
                            if (rolefound)
                            {                                
                                rolesDto.WithRole.Add(new UserRoleDTO
                                                          {
                                                              UserId = role.UserId,
                                                              UserFullname = user.Fullname,
                                                              AccessLevel = role.AccessLevel,
                                                              RoleId = role.RoleId
                                                          });
                                break;
                            }
                        }
                    }
                    if (!rolefound)
                        rolesDto.WithNoRole.Add(new UserRoleDTO
                        {
                            UserId = user.Id,
                            UserFullname = user.Fullname,
                        });
                }
            }
            // sort lists
            return rolesDto;
        }

these are the requirements:

- if filterRoleId is not null:
the IList<RolesDTO> WithRole must contain all RoleIds that are equal to filterRoleId and IList<RolesDTO> WithNoRole must contain all RoleIds that are not equal to filterRoleId

- same principle for filterUserId and role.UserId

the user that has the username "admin" may not appear in both lists

Thanks,
Daddy

-----------------------------------------------------
What You See Is What You Get
Never underestimate tha powah of tha google!

 

[jmeckley]

why refactor to linq? if it's working, don't mess with it.

I would recommend some other refactorings though.
1. use the null object pattern. this will removed the need to constantly check for nulls.
2. encapsulate collections so they are not exposed to client calls. this allows the contain object to control how a collection is manipulated.
3. encapsulate logic into appropriate objects. for example, determining if the user an administrator should be part of the user object it is a user who can be an administrator after all.
4. keep your variables explicit. 1st you use the Boolean variable rolefound to determine if the user is an adminstrator, then you use it again to determine if the role exists. according to the logic above if the user is an administrator it should not be part of either list.

taking into account the refactorings above I came up with this

class User
{
	private IList<Role> roles
	public IEnumerable<Role> Roles {get{return roles;}}

	public User()
	{
		Roles = new List<Role>();
	}

	public void AssociatedWith(string role)
	{
		var role = new Role(this, role);
		if(roles.Contains(role)) return;
		roles.Add(role);
	}
   
	public void DisAssociateFrom(Role role)
	{
		if(!roles.Contains(role)) return;
		roles.Remove(role);
	}
	
	public bool IsAnAdministrator()
	{
		return Username == "admin";
	}
}

public RolesDTO Roles(int roleId, int userId)
{
	var roles = repository
		.GetAll(false)
		.DefaultIfEmpty()
		.Where(user => !user.IsAnAdministrator())
		.SelectMany(user => roles); 
	
	var withRoles = roles
		.Where(role =>  role.RoleId == roleId || role.User.UserId == userId)
		.Select(role => new UserRoleDTO
						{
							UserId = role.User.UserId,
							UserFullname = user.Fullname,
							AccessLevel = role.AccessLevel,
							RoleId = role.RoleId
						})
		.ToArray();
		
	var withoutRoles = roles
		.Where(role => role.RoleId != roleId)
		.Select(role => new UserRoleDTO
						{
							UserId = role.User.UserId,
							UserFullname = user.Fullname
						})
		.ToArray();
	
	[COLOR=green]//not being a fan of public lists I use an array[/color]
	return new RolesDTO { WithRole = withRoles, WithNoRole = withoutRoles };
}

Jason Meckley
Programmer
Specialty Bakers, Inc.

faq855-7190
faq732-7259

 

[whosrdaddy]

thanks Jason,

quote: not being a fan of public lists I use an array

care to elaborate on this subject?

/Daddy




-----------------------------------------------------
What You See Is What You Get
Never underestimate tha powah of tha google!

 

[jmeckley]

I like to keep the actual implementation of the collection encapsulated within an object and expose the lowest common interface IEnumerable<T>. This is known as the Iterator pattern.

Benfits:
1. collection manipulation is managed by the containing object. In this case User. You cannot add a role to the user without using a member of the User object. the logic associated with adding a role to a list is also in a single location.
2. Public access as IEnumerable<T> makes the collection readonly. I can also take advantage of differed iternation. research the yield keyword and IEnumerable for more information.


If needed i could then change the implemenation later on. In the example of the User. I could change the IList<> to an ISet<>, HashTable, Dictionary<,>. the consumers of the User class are indifferent to what the actual implementation is. They simpley know calling User.Roles will return an enumeration of roles associated with the user.

DTOs are slightly different. usually DTO's are either involved with a GUI, In which case I want want all my data ready to go. Any deferred iteration that was going to occur should have happened by now. Once I'm at the GUI I just want to render and be done.
Another use for DTOs in sending data over the wire (message service bus, webserivce) I cannot guarantee the receiver is a .net process which understand generics and I cannot serialize an interface (not without dynamic proxy anyway). An array is a simple data type implementation with a fixed sized.

HTH

Jason Meckley
Programmer
Specialty Bakers, Inc.

faq855-7190
faq732-7259

 

[whosrdaddy]

in fact the code that I showed you is from a service layer.
the actual layout I have is like this:

database<->repository<->service<->presentation<->view

the service layer makes DTO's for my presentation layer.

/Daddy

-----------------------------------------------------
What You See Is What You Get
Never underestimate tha powah of tha google!

 

[jmeckley]

that is a common architectural layout. It is good to see other developers moving towards a layered approach were thought is given to separation of concerns.

Jason Meckley
Programmer
Specialty Bakers, Inc.

faq855-7190
faq732-7259

 

[whosrdaddy]

Indeed, it simplifies testing alot!

I'm having problems testing my (mvc) controllers though -> Need some time to get aquainted with rhino.mocks

Cheers,
Daddy

-----------------------------------------------------
What You See Is What You Get
Never underestimate tha powah of tha google!

 

[jmeckley]

Daddy, here is post on the topic of encapsulation. Jimmy does a much better job at explaining it, than I do.

Jason Meckley
Programmer
Specialty Bakers, Inc.

faq855-7190
faq732-7259