1
\$\begingroup\$

I am starting a project to put into practice the lessons learned in "Clean Code" by Robert "Uncle Bob" Martin.

The project is a website that displays the names of items in a DB table. It uses ASP.NET MVC Framework, and I have tried my best to separate domains in the Model. I have used Test Driven Design, but there is a lot of code already, so I have left it out here.

It is extremely basic, but it uses a lot of concepts, and I would like to make sure I have understood everything right before adding complexity.

I am interested in learning how I could have better followed Agile concepts, even if overkill for this little application (or course, any other advice is good to take). In particular, I feel like to controller is doing a lot of logic for a single function (and it is difficult to unit test).

DataAccessLayer.dll

Customer.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace DataAccessLayer.DataTransferObject
{
    public class Customer
    {
        public string Name { get; set; }
        public Customer(string inputName)
        {
            Name = inputName;
        }
    }
}

ICustomerRepository.cs

using System;
using System.Collections.Generic;
using System.Data;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace DataAccessLayer.Repositories.Interfaces
{
    public interface ICustomerRepository
    {
        ICollection<DataTransferObject.Customer> GetAllCustomerss();
    }
}

SQLPlusCustomerRepository.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using DataAccessLayer.DataTransferObject;
using System.Data;

namespace DataAccessLayer.Repositories
{
    public class SQLPlusCustomerRepository : Interfaces.ICustomerRepository
    {
        private IDbConnection connection;
        public SQLPlusCustomerRepository(IDbConnection inputConnection)
        {
            connection = inputConnection;
        }
        public ICollection<Customer> GetAllCustomers()
        {
            ICollection<Customer> resultList = new List<Customer>();

            const string getAllCustomerQuery = "SELECT NAME FROM CUSTOMERS";

            using (connection)
            {
                connection.Open();
                IDbCommand dbCommand = connection.CreateCommand();
                dbCommand.CommandType = CommandType.Text;
                dbCommand.CommandText = getAllCustomerQuery;

                IDataReader dbDataReader = dbCommand.ExecuteReader();
                while (dbDataReader.Read())
                {
                    resultList.Add(new Customer(dbDataReader.GetValue(0).ToString()));
                }
                return resultList;
            }
        }
    }
}

CustomerDisplay (WebSite)

Customer.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;

namespace CustomerDisplay.Models
{
    public class Customer
    {
        public string Name { get; set; }
        public Customer(DataAccessLayer.DataTransferObject.Customer dtoCustomer)
        {
            Name = dtoCustomer.Name;
        }
    }
}

HomeController.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Web.Mvc;
using XXX.SQLplus;

namespace CustomerDisplay.Controllers
{
    public class HomeController : Controller
    {
        public ActionResult Index()
        {
            SQLplusConnection connection = new SQLplusConnection(
                Properties.Settings.Default.connectionString
                );
            DataAccessLayer.Repositories.SQLPlusCustomerRepository repository =
                new DataAccessLayer.Repositories.SQLPlusCustomerRepository(connection);

            ICollection<Models.Customer> viewItemsList = new List<Models.Customer>();

            foreach (DataAccessLayer.DataTransferObject.Customer dtoCustomer in repository.GetAllCustomers()){
                viewItemsList.Add(new Models.Customers(dtoCustomers));
            }
            return View(viewItemsList);
        }
    }
}

Index.cshtml

@model  ICollection<CustomerDisplay.Models.Customer>

@foreach(CustomerDisplay.Models.Customer Customer in Model)
{
    <span>@Customer.Name</span>
}
\$\endgroup\$
1
\$\begingroup\$

Give proper names to parameters; don't pointlessly prefix them, e.g. "inputName" or "dtoCustomer".


Four capital letters in a row -- as in "SQLPlus" -- goes against Microsoft's coding guidelines. It is expressed at length in the old version:

Do capitalize only the first character of acronyms with three or more characters, except the first word of a camel-cased identifier.


Your GetAllCustomers() method is 20 lines for something that can be done much easier and neater when using an ORM like Entity Framework or Dapper. IMHO you should avoid writing ADO.NET code unless you have a really good reason to do it.

I also have the impression that you don't always properly clean up data readers etc.


Is there a reason why your code is littered with class name prefixes like "Interfaces." or "DataAccessLayer.DataTransferObject."? Those make your code harder to read, IMHO.


I don't see why you create a SQLplusConnection in your controller, only to pass to to the SQLPlusCustomerRepository? Why not let the repository deal with all of that instead of mixing it all over the place?


The foreach converting DTOs to Models could be done far more elegantly in LINQ.


There are 100+ lines of code here, and all you've done is retrieved one field from one table and displayed that. I'm all in favor of separation, but to me this feels like massive overkill and it is likely to become very unwieldy very soon. It looks like code that would have been barely acceptable ten+ years ago, these days it feels outdated and bloated.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your input. The ADO.NET provider's for the DB used here does not support EF. And I had no idea simpler things like Dapper existed. -> GetAllCustomers() is now 4 lines. As far as prefixes go, I have created different folders in my project, to have interfaces in one place, DTOs in an other. VS created different namespaces, it seemed fine, I went with it. \$\endgroup\$ – Maxime Feb 22 '18 at 8:45
  • \$\begingroup\$ The SQLPlusConnection creation in the controller is a dependency thing. Despite its name (which will be changed), SQLPlusCustomerRepository is only using a IDBConnection. It is my understanding that it is best to keep it that way, and inject a dependency from upper layers, am I wrong? \$\endgroup\$ – Maxime Feb 22 '18 at 8:46
  • \$\begingroup\$ @Maxime The problem with splitting up a project in very strict layers is that you risk ending up doing a lot of plumbing without gaining much. These days I tend to avoid such patterns and instead focus on learning things like CQRS. \$\endgroup\$ – BCdotWEB Feb 22 '18 at 10:00
  • \$\begingroup\$ @Maxime: Dependency injection should only be used if the calling code benefits from having control over the dependency. In this case, you're creating more work for no discernable benefit. If you were truly working dependency-agnostic, you'd have a CustomerRepository, not a SQLPlusCustomerRepository. \$\endgroup\$ – Flater Feb 22 '18 at 10:27
  • \$\begingroup\$ @Flater The poorly chosen name notwithstanding, don't I have an agnostic CustomerRepository? \$\endgroup\$ – Maxime Feb 22 '18 at 10:53

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.