N-tiered web app to save a role

My project manager is saying that there are many bad things in my code, but they are not helping me to correct it. Can you please specify the bad things they mean?

I have 7 projects in my solution that work together to save a role to the database. Here are the details:

Project - 1 - Object Model(Class Library Project) - Only declaring the Class Properties

public class OM_Role
{
    [DisplayName("Role ID"), Key]
    public Int16 RoleID { get; set; }

    [DisplayName("Role Name"), Required(ErrorMessage = "Please enter role")]
    public String Role { get; set; }

    [DisplayName("Is Activated")]
    public Boolean IsActivated { get; set; }
}

Project - 2 - IDAL(Class Library Project) - Declaring the Interface methods

public interface IRole
{
    Task Save(OM_Role role);
}

Project - 3 - Definition of Save Role Method

public class DAL_Role : IRole
{
    public async Task Save(OM_Role role)
    {
        using (var transaction = new TransactionScope(
            TransactionScopeOption.Required,
            new TransactionOptions { IsolationLevel = IsolationLevel.ReadUncommitted },
            TransactionScopeAsyncFlowOption.Enabled))
        {

            using (var roleContext = new DatabaseTables())
            {
                var data = await roleContext.tblRoles.FirstOrDefaultAsync(
                i => i.RoleID == role.RoleID);
                if (data != null)
                {
                    data.Role = role.Role;
                    data.IsActivated = role.IsActivated;
                }
                else
                {
                    role.IsActivated = false;
                    roleContext.tblRoles.Add(role);
                }
                await roleContext.SaveChangesAsync();
                transaction.Complete();
                return "";
            }
        }
    }
}

Project - 4 - WebAPI for calling the DAL Layer through IRole using Ninject

```
public class roleAPIController : ApiController
{
IRo

Naming conventions.

In C#, types are named using PascalCase. Rename:

• OM_Role to OmRole

• DAL_Role to DalRole

• rolesAPIController to RolesApiController

• and so on

The same goes for properties - so tblRoles should be named TblRoles

Coding Style

You could mark your private field _role readonly. People often also explicitly mark them as private though it's not entirely needed.

The IRole type doesn't really represent a role, does it? The name seems misleading. I think IRoleRepository would be a better fit.

Why does roleAPIController have a parameterless constructor? Using that constructor would result in NullRefenceExceptions being thrown, since _role would be unassigned.

This cast is unnecessary:

var result = ((RoleResponse)JsonConvert.DeserializeObject(responseBody));

You should wrap the usage of IHttpClient in a using statement to ensure proper resource cleanup:

using(var client = new HttpClient())
{
    ...
}

Exceptions

Regarding BLL_Roles.Save: Catching all exceptions is usually regarded as a bad idea. I'm not saying this is the case, but consider catching the specific exceptions you expect to happen, e.g. HttpRequestException, JsonException, etc.

Why does the IRole.Save method return a Task if the string is always empty? Why not just Task? Similarly, your roleController checks if the string is null: String.IsNullOrEmpty(data), this seems redundant.

Also, returning a empty/non-empty string to signal that something went wrong does not seem right. Surely, if the API call fails, that would be an exceptional scenario, would it not? Consider letting the exception bubble up to the controller, where the error can be handled. Then, change the IRole.Save return type from Task to simply Task.

This is how I'd report errors:

Web Api:

public interface IRole
{
    Task Save(OmRole role);
}

public class DalRole : IRole
{
    public async Task Save(OmRole role)
    {
        //...
    }
}

public class RoleApiController : ApiController
{
    [Route("api/v1/roles"), HttpPost]
    [System.Web.Mvc.ValidateAntiForgeryToken]
    public async IHttpActionResult Save([FromBody] OM_Role role)
    {
        try
        {
            var result = await _role.Save(role);
            return OK();
        }
        catch (Exception ex)
        {
            // log exception
            return InternalServerError(ex);
        }
    }
}

MVC App:

public class BllRole : IRole
{
    public async Task Save(OmRole role)
    {
        var url = "api/v1/roles";
        String URI = String.Format("{0}{1}", "APIDomainName", url);    
        var postData = new OmRole();
        postData.IsActivated = role.IsActivated;
        postData.Role = role.Role;
        postData.RoleID = role.RoleID;    
        using(var httpClient = new HttpClient())
        {
            var content = new StringContent(JsonConvert.SerializeObject(postData));
            content.Headers.ContentType = new MediaTypeHeaderValue("application/json");

            var responseMessage = await httpClient.PostAsync(URI, content);    

            //throws an exception if a 500 Internal Server Error occurred
            responseMessage.EnsureSuccessStatusCode();
        }
    }
}

[LoginAuthentication, AdminAuthorization]
public class RoleController : Controller
{
    [HttpPost, Route("SaveRole")]
    public async Task Save(OmRole objrole)
    {
        try
        {
            await _role.Save(objrole);
            return Json(new { Success = true });    
        }
        catch(Exception ex)
        {
            return Json(new { Success = false });    
        }
    }
}

HTTP and Web Api

The route api/v1/SaveRole does not seem RESTful. RESTful APIs are resource-oriented - a more natural way would be to simply issue a POST requets to api/v1/routes.

If you're using Web API 2, your method rolesAPIController.Save could be re-written to leverage the IHttpActionResult family of classes

public async IHttpActionResult Save([FromBody] OM_Role role)
{
    var result = await _role.Save(role);
    return Ok(new {ErrorMessage = result});
}

Also notice how you're returning a 200 OK response, even if the request failed. This is definitly not right. You should serve a 500 Internal Server Error instead.

StackExchange Code Review Q#103879, answer score: 5