Improve MVC code that updates user role

[Andrew33]

Hi,
Any suggestions on how to improve this code which updates user role (MVC).
Thanks

[Authorize(Roles = "Administrator")]
[HttpPost]
public ActionResult EditUser(UserAdminViewModel model)
{
var result = m_AccountModel.UpdateUser(model);
if (result)
return RedirectToAction("UserList");
}

public bool UpdateUser(UserAdminViewModel model)
{
using (var dbConnection = MyDbConnectionPool.GetConnection())
{
var user = dbConnection.Query<DbAspNetUserRoles>().FirstOrDefault(p => p.UserId == model.Id);
if (user == null)
{
var newRoleRec = new DbAspNetUserRoles()
{
RoleId = model.Role,
UserId = model.Id
};
dbConnection.Insert(newRoleRec);
}
else
{
var sql = $"UPDATE AspNetUserRoles SET RoleId = '{model.Role}' WHERE UserId = '{model.Id}'";
dbConnection.Query<DbAspNetUserRoles>(sql).ToList();
}
}
}

 

[cjard]

Using [CODE] tags so I can actually read it would be a great start

 

[Skydiver]

I hate misnamed methods. If it's call "update" then it should only do an "update". But your code does an "insert-if-not-present, otherwise update". Code should have the minimal amount of surprises for the reader.

Also, why are you using Query<>() to do an update?

 

[Andrew33]

Formatted code

[Authorize(Roles = "Administrator")]
[HttpPost]
public ActionResult EditUser(UserAdminViewModel model)
{
  var result = m_AccountModel.UpdateUser(model);
  if (result)
    return RedirectToAction("UserList");
}

public bool UpdateUser(UserAdminViewModel model)
{
  using (var dbConnection = MyDbConnectionPool.GetConnection())
  {
    var user = dbConnection.Query<DbAspNetUserRoles>().FirstOrDefault(p => p.UserId == model.Id);
    if (user == null)
    {
      var newRoleRec = new DbAspNetUserRoles()
      {
        RoleId = model.Role,
        UserId = model.Id
      };
      dbConnection.Insert(newRoleRec);
    }
    else
    {
      var sql = $"UPDATE AspNetUserRoles SET RoleId = '{model.Role}' WHERE UserId = '{model.Id}'";
      dbConnection.Query<DbAspNetUserRoles>(sql).ToList();
    }
  }
}

 

[Skydiver]

With that formatted code, it kind of jumps out on the screen that EditUser() seems to be missing a return when the if condition is not satisfied; as well as UpdateUser() has no return to satisfy the declaration that says it returns a bool.

 

[cjard]

And what happens when model.Role has been kindly supplied by the user as Administrator'--