0
\$\begingroup\$

I have 3 tables. Users, Roles and RolesToUsers

Here is User model:

public partial class User
{
    [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2214:DoNotCallOverridableMethodsInConstructors")]
    public User()
    {
        this.UserToRoles = new HashSet<UserToRole>();
    }

    [Key]
    public int Id { get; set; }
    public string Name { get; set; }

    [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2227:CollectionPropertiesShouldBeReadOnly")]
    public virtual ICollection<UserToRole> UserToRoles { get; set; }
}

Here is Role Model:

public partial class Role
{
    [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2214:DoNotCallOverridableMethodsInConstructors")]
    public Role()
    {
        this.UserToRoles = new HashSet<UserToRole>();
    }

    [Key]
    public int Id { get; set; }
    public string RoleName { get; set; }

    [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2227:CollectionPropertiesShouldBeReadOnly")]
    public virtual ICollection<UserToRole> UserToRoles { get; set; }
}

And here is UserToRole model:

public partial class UserToRole
{
    [Key]
    public int Id { get; set; }
    [ForeignKey("Role")]
    public Nullable<int> RoleId { get; set; }
    [ForeignKey("User")]
    public Nullable<int> UserId { get; set; }

    public virtual Role Role { get; set; }
    public virtual User User { get; set; }
}

I need to show all Users and related to it Roles.

I write this repo method

public List<UserViewModel> GetUsers()
{
    var users = db.Users
        .Include(u => u.UserToRoles.Select(ur => ur.Role))
        .Select(u => new UserViewModel
        {
            UserName = u.Name,
            Roles = u.UserToRoles.Select(ur => ur.Role.Name)
        })
        .ToList();

    return users;
}

After it I need to show data from database in this format Ronald - Admin,User, where Ronald,etc is Name of Users, after - is Roles related to user.

Repo method is working great

And in View I show it using this code.

function GetUsers() {
let url = "/Home/GetUsers";
$.ajax({
    type: 'GET',
    url: url,
    dataType: 'json',
    success: function (data) {
        var separator = "";
        for (var i = 0; i < data.length; ++i) {
            var userRow = data[i].UserName + " - " + data[i].Roles.join(", ");

            $('#taskresult3').append(separator + userRow);
            separator = ",";
        }
    }
});

Any suggestions or improvements I can do to this code?

\$\endgroup\$
1
\$\begingroup\$

partial is meant to be used when the class spans multiple files, not when the class is to be extended. If you want to use virtual there is no need to mark the class as partial unless it really does exist in multiple files (a bad design for the most part anyway, with few good uses). If you want to force the class to be inherited, use abstract instead of partial.

You can remove the default constructor (and the warning suppression) by using default values:

public class User
{
    [Key]
    public int Id { get; set; }
    public string Name { get; set; }

    [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2227:CollectionPropertiesShouldBeReadOnly")]
    public virtual ICollection<UserToRole> UserToRoles { get; set; } = new HashSet<UserToRole>();
}

It is now functionally the same but has less code to maintain.

Nullable properties can also be defined shorthand:

public class UserToRole
{
    [Key]
    public int Id { get; set; }
    [ForeignKey("Role")]
    public int? RoleId { get; set; }
    [ForeignKey("User")]
    public int? UserId { get; set; }

    public virtual Role Role { get; set; }
    public virtual User User { get; set; }
}

The int? is effectively the same as Nullable<int>, it is a little easier on the eyes without having to mentally parse generics.

The last comment is about your repo method. It could be shortened up a little bit:

public List<UserViewModel> GetUsers()
{
    return db.Users
        .Include(u => u.UserToRoles.Select(ur => ur.Role))
        .Select(u => new UserViewModel
        {
            UserName = u.Name,
            Roles = u.UserToRoles.Select(ur => ur.Role.Name)
        })
        .ToList();
}

Or, if you really want to shorten it up:

public List<UserViewModel> GetUsers() => db.Users
                                           .Include(u => u.UserToRoles.Select(ur => ur.Role))
                                           .Select(u => new UserViewModel
                                           {
                                               UserName = u.Name,
                                               Roles = u.UserToRoles.Select(ur => ur.Role.Name)
                                           }).ToList();

The only reason I would use the intermediate users variable in the method is to do debugging inside the method. You could add a breakpoint on the return users; line and debug the value. If you are confident in the operation of the query and the results, shortening it up helps with code maintenance.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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