3
\$\begingroup\$

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<String> Save(OM_Role role);
}

Project - 3 - Definition of Save Role Method

public class DAL_Role : IRole
{
    public async Task<String> 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
{
    IRole _role;
    public roleAPIController(IRole role)
    {
        _role = role;
    }

    public roleAPIController()
    {
    }

    [Route("api/v1/SaveRole"), HttpPost]
    [System.Web.Mvc.ValidateAntiForgeryToken]
    public async Task<HttpResponseMessage> Save([FromBody] OM_Role role)
    {
        var result = await _role.Save(role);
        return Request.CreateResponse(HttpStatusCode.OK, new { ErrorMessage = result });
    }
}

Project - 5 - BLL(Class Library Project) - This calls the Web API

internal class RoleResponse
{
    public String ErrorMessage { get; set; }
    public OM_Role result { get; set; }
    public int RoleID { get; set; }
}

public class BLL_Roles : IRole
{
    public async Task<string> Save(OM_Role role)
    {
        try
        {
            var url = "api/v1/SaveRole";
            String URI = String.Format("{0}{1}", "APIDomainName", url);    
            var postData = new OM_Role();
            postData.IsActivated = role.IsActivated;
            postData.Role = role.Role;
            postData.RoleID = role.RoleID;    
            var httpClient = new HttpClient();    
            HttpContent content = new StringContent(JsonConvert.SerializeObject
  (postData));    
            content.Headers.ContentType = new MediaTypeHeaderValue("application/json");
            var responseMessage = await httpClient.PostAsync(URI, content);    
            /*     Retrieve the data from API    */
            string responseBody = await responseMessage.Content.ReadAsStringAsync();

            var result =
           ((RoleResponse)JsonConvert.DeserializeObject<RoleResponse>(responseBody));    
            return result.ErrorMessage;
        }
        catch (Exception ex)
        {
            return ex.Message;
        }
    }
}

Project - 6 - Presentation Layer that calls BLL using Ninject

[LoginAuthentication, AdminAuthorization]
public class roleController : Controller
{
    IRole _role;
    public roleController(IRole role)
    {
        _role = role;
    }

    [HttpPost, Route("SaveRole")]
    public async Task<JsonResult> Save(OM_Role objrole)
    {
        var data = await _role.Save(objrole);
        return Json(new { Success = String.IsNullOrEmpty(data) });
    }
}

Project - 7 - Code First Database Creation Entity FrameWork

public class DatabaseTables : DbContext
{
    public DatabaseTables()
        : base("Freelancer")
    {
    }

    public virtual IDbSet<OM_Role> tblRoles { get; set; }
    protected override void OnModelCreating(DbModelBuilder modelBuilder)
    {

        RoleTable(modelBuilder);
    }

    private void RoleTable(DbModelBuilder modelBuilder)
    {
        var roleModel = modelBuilder.Entity<OM_Role>();
        roleModel.Property(x => x.RoleID)
        .HasDatabaseGeneratedOption(DatabaseGeneratedOption.Identity);
        roleModel.Property(x => x.Role).IsRequired().IsVariableLength().HasMaxLength(50);
        roleModel.Property(x => x.IsActivated).IsRequired();
        roleModel.ToTable("tblRole");
    }
}
\$\endgroup\$
  • \$\begingroup\$ Let me get this straight. You have 7 separate projects or you have 7 separate files? \$\endgroup\$ – RubberDuck Sep 5 '15 at 15:16
  • \$\begingroup\$ @RubberDuck: There are 7 different Projects \$\endgroup\$ – eddedeed Sep 5 '15 at 15:18
  • \$\begingroup\$ Could you elaborate on what the project manager is telling you that is bad? \$\endgroup\$ – Ethan Bierlein Sep 5 '15 at 15:29
  • \$\begingroup\$ @EthanBierlein: Sir, they just told that to remove the unnecessary code and to use the best coding conventions. Unfortunately they are not able to suggest the good things they want to see. I think, may be because they are busy. \$\endgroup\$ – eddedeed Sep 5 '15 at 15:31
  • \$\begingroup\$ Okay. One more question. (Two actually). Is there more code in these projects? Did someone specify this design for you, or did you come up with this high level design yourself? (Not the code, I know you wrote the code, I mean the architecture.) \$\endgroup\$ – RubberDuck Sep 5 '15 at 15:39
5
\$\begingroup\$

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<RoleResponse>(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<string> 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<string> 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<JsonResult> 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.

\$\endgroup\$
  • \$\begingroup\$ Thank you first of all for the very quick suggestion. I am checking for IHttpActionResult Question: async is being used in BLL Layer. Although the async operation is already being done in BLL. Then do I still need to mark Presentation Layer Controller as async ? \$\endgroup\$ – eddedeed Sep 5 '15 at 15:53
  • \$\begingroup\$ @Pankaj Yes, the code needs to be async all the way. \$\endgroup\$ – dcastro Sep 5 '15 at 16:00
  • \$\begingroup\$ I saw the updated code: That says the code is unnecessary var result = ((RoleResponse)JsonConvert.DeserializeObject<RoleResponse>(responseBody)); In this code I am DeSerializing the Object. May I request to suggest to proper way of DeSerialization ? \$\endgroup\$ – eddedeed Sep 5 '15 at 16:03
  • \$\begingroup\$ @Pankaj Not the code, the cast. You could simply do this: var result = JsonConvert.DeserializeObject<RoleResponse>(responseBody) \$\endgroup\$ – dcastro Sep 5 '15 at 16:05
  • \$\begingroup\$ @Pankaj I added an example showing how I'd report errors, instead of using an empty/non-empty string. \$\endgroup\$ – dcastro Sep 5 '15 at 16:17

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.