3
\$\begingroup\$

I have to take over an old project that was coded in VS 2010 C# 4.0 in 2012. Interface programming seems to be all over the place and over-applied to me, but I am unsure if it is that way.

I need to know what to keep and what to refactor.

Looking at the code, do you think it was applied correctly? Interface is used as a contract, ain't it?

Solution Explorer
=========================== 
Inventory.BLL
|_ CustomerBLL.cs
|_ ICustomerBLL.cs
|_ BLLProvider.cs

Inventory.BO
|_ Customer.cs

Inventory.DAL
|_ CustomerDAL.cs
|_ ICustomerDAL.cs
|_ DALProvider.cs

Inventory.UI
|_ frmCustomer.cs
===========================

//CustomerBLL.cs 
namespace Inventory.BLL
{
    class CustomerBLL : ICustomerBLL
    {
        #region Services
        ICustomerDAL CustomerDAL
        {
            get { return DALProvider.CustomerDAL; }
        }
        #endregion

        #region ICustomerBLL Members
        public Customer GetCustomerDetail(string customerCode)
        {
            return CustomerDAL.GetCustomerDetail(customerCode);
        }

        public bool Save(Customer customer)
        {
            return CustomerDAL.Save(customer);
        }

        public DataTable GetCustomers_List(Customer customer)
        {
            return CustomerDAL.GetCustomers_List(customer);
        }
        #endregion
    }
}
//End of Class


//Interface ICustomerBLL.cs 
namespace Inventory.BLL
{
    public interface ICustomerBLL
    {
        Customer GetCustomerDetail(string customerCode);
        bool Save(Customer customer);
        DataTable GetCustomers_List(Customer customer);
    }
}
//End of Interface



//BLLProvider.cs 
namespace Inventory.BLL
{
    public class BLLProvider
    {
        private static ICustomerBLL customerBLL = new CustomerBLL();

        public static ICustomerBLL CustomerBLL
        {
            get { return customerBLL; }
        }
    }
}
//End of Class


//Customer.cs
namespace Inventory.BO
{
    [Serializable]
    public class Customer : CloneBase<Customer>
    {
        private string customerCode;
        private string address1;

        public string CustomerCode
        {
            get { return customerCode; }
            set { customerCode = value; }
        }
        public string Address1
        {
            get { return address1; }
            set { address1 = value; }
        }
    }
}
//End of Class


//CustomerDAL.cs 
namespace Inventory.DAL
{
    class CustomerDAL : ICustomerDAL
    {
        #region Private Members
        private Customer BindCustomer(DataSet ds, DataRelation dr)
        {
            Customer customer = new Customer();

            foreach (DataRow row in ds.Tables[0].Rows)
            {
                customer.CustomerCode = Convert.ToString(row[0]);
                customer.Address1 = Convert.ToString(row[4]);

                List<MailingAddresses> mailingAddresses = new List<MailingAddresses>();

                foreach (DataRow addressRow in row.GetChildRows(dr))
                {
                    MailingAddresses mailingAddresses = new MailingAddresses();
                    mailingAddresses.CustomerCode = Convert.ToString(addressRow[1]);         
                    mailingAddresses.Address1 = Convert.ToString(addressRow[4]);
                    mailingAddresses.Add(mailingAddresses);
                }
                customer.MailingAddresses = mailingAddresses;
            }

            return customer;
        }
        #endregion

        #region ICustomerDAL Members
        public Customer GetCustomerDetail(string customerCode)
        {
            DatabaseDAL databaseDAL = new DatabaseDAL();
            ....
        }

        public bool Save(Customer customer)
        {
            bool result;
            DatabaseDAL databaseDAL = new DatabaseDAL(); 
            List<MyDatabaseCommand> myDbCmd = new List<MyDatabaseCommand>();
            OdbcParameter[] param;

            param = new OdbcParameter[2];
            param[0] = new OdbcParameter("?CustomerCode", customer.CustomerCode);
            param[1] = new OdbcParameter("?Address1", customer.Address1);

            myDbCmd.Add(new MyDatabaseCommand("INSERT INTO customer (customer_code, address_1) VALUES (?, ?);", param));

            int i = 1;

            foreach (MailingAddress mailingAddress in customer.MailingAddress)
            {
                param = new OdbcParameter[2];
                param[0] = new OdbcParameter("?CustomerCode", mailingAddress.CustomerCode);
                param[1] = new OdbcParameter("?Address1", mailingAddress.Address1);

                myDbCmd.Add(new MyDatabaseCommand("INSERT INTO mailing_address (customer_code, address_1) VALUES (?, ?);", param));

                i += 1;
            }

            result = databaseDAL.ExecTransaction(myDbCmd);
            return result;
        }
        #endregion
    }
}
//End of Class


//Interface ICustomerDAL.cs 
namespace Inventory.DAL
{
    public interface ICustomerDAL
    {
        Customer GetCustomerDetail(string customerCode);
        bool Save(Customer customer);
    }
}
//End of Interface


//DALProvider.cs 
namespace Inventory.DAL
{
    public class DALProvider
    {
        //There are 20 others in this class files, I just listed those applicable in this sample
        private static IDatabaseDAL databaseDAL = new DatabaseDAL();
        private static ICustomerDAL customerDAL = new CustomerDAL();

        public static IDatabaseDAL DatabaseDAL
        {
            get { return databaseDAL; }
        }

        public static ICustomerDAL CustomerDAL
        {
            get { return customerDAL; }
        }
    }
}
//End of Class


//WinForm frmCustomer, .Net 4 Framework 
namespace Inventory.UI
{
    public partial class frmCustomer : Form
    {
        #region Services
        ICustomerBLL CustomerBLL
        {
            get { return BLLProvider.CustomerBLL; }
        }
        #endregion

        private bool SaveCustomerInfo()
        {
            Customer customer = GenerateCustomerObject(); 
            CustomerBLL.Save(customer);
        }

        private Customer GenerateCustomerObject()
        {
            Customer customer = new Customer();

            customer.CustomerCode = txtCustomerCode.Text;
            customer.Address1 = txtAddress1.Text;

            return customer;
        }
    }
}
//End of Form

Since I need to maintain, add functionality and refactor the codes, please give me any of your input.

\$\endgroup\$
  • \$\begingroup\$ @furor it seems you have misunderstood what we define as a code-review. We do not help you understanding code given to you, so you can refactor it or add new features. There is a proposal: "Code Understanding" that fills exactly this gap. Sure your code is reviewable as a CR question, but I don't think that this question will get answers along the lines of what you expect \$\endgroup\$ – Vogel612 Oct 29 '14 at 8:50
  • \$\begingroup\$ @Vogel612 Thank you for helping me with your replies. Sorry I didn't state that English is not my primary language and I could have misunderstood what is defined as code review. In this case, do you think this question can be on-topic? By the way, I understood the coding. The "Area51: Code Understanding" is kinda cute. Seriously, I didn't know it exist. I mean how many natives would know AreaXXX exist ;) \$\endgroup\$ – furor Oct 29 '14 at 9:30
3
\$\begingroup\$

CustomerDAL

You can remove the BindCustomer() method as it isn't called at all. But if you want to keep it, because you didn't show the whole code, you can, by extracting the childrow processing to a separate method, reduce it to

private Customer BindCustomer(DataSet ds, DataRelation dr)
{
    Customer customer = new Customer();
    int rowCount =ds.Tables[0].Rows.Count;
    if (rowCount == 0) {return customer;}

    DataRow row = ds.Tables[0].Rows[rowCount -1];

    customer.CustomerCode = Convert.ToString(row[0]);
    customer.Address1 = Convert.ToString(row[4]);

    customer.MailingAddresses = GetMailingAddresses(row.GetChildRows(dr));

    return customer;
}

private List<MailingAddresses> GetMailingAddresses(DataRow[] childRows)
{
    List<MailingAddresses>addresses = new List<MailingAddresses>();
    foreach (DataRow addressRow in childRows)
    {
        MailingAddresses mailingAddresses = new MailingAddresses();

        mailingAddresses.CustomerCode = Convert.ToString(addressRow[1]);         
        mailingAddresses.Address1 = Convert.ToString(addressRow[4]);

        addresses.Add(mailingAddresses);
    }
    return addresses;
}

Customer

By using auto properties this can be reduced to

public class Customer : CloneBase<Customer>
{

    public string CustomerCode { get; set;}
    public string Address1 { get; set;}

}
\$\endgroup\$
  • \$\begingroup\$ Yes in fact, one of the first stuff I did was to change all to auto properties and to apply the DRY principles. \$\endgroup\$ – furor Oct 29 '14 at 16:56
2
\$\begingroup\$

Interfaces are basically used for two (inter-related) things.

  1. To let several objects respond to the same commands. This lets you have a function where one of the parameters can be an object, but it doesn't have to be of a particular type.

  2. To allow an object in one assembly, to use an object in another assembly, without having the first assembly rely upon the second. Instead, both rely upon a third assembly, which defines the interface. In this case there MIGHT only be one object that implements the interface. One reason for doing this would be to avoid circular dependencies.

Whether you're solution is over-interfaced or not, depends upon whether the interfaces are used for one of the above reasons -- if not, then you should probably be using the objects directly, instead of using an interface.

On a totally unrelated note: you are accessing fields in your datarow by positions. This is a bad idea, as it is fragile -- it's easy to change the position of things and have it appear to continue to work, and only fail on some rare condition.

You have properties with declared backing fields, where the backing fields aren't used to do non-standard manipulation. They should be turned into auto properties for smaller code and easier reading.

\$\endgroup\$
  • \$\begingroup\$ I agreed on the issue of accessing datarow by positions to be a bad idea. I thought I read an article somewhere in 2008 that accessing it by field name is slower than using field index, no? \$\endgroup\$ – furor Oct 29 '14 at 16:58
  • \$\begingroup\$ @furor: probably a bit slower, the row<string>("address1") extension method is probably even slower than that. Despite any such performance impact, absent this being an identified bottle-neck, that is how I would recommend addressing the columns: typed and with the column name. \$\endgroup\$ – jmoreno Oct 29 '14 at 18:24
  • \$\begingroup\$ I went through all the codes. He has an interface for every class. There is no inheritance. All parameters passed are typed with the accompanying interface. Can you give me an example of point 2? \$\endgroup\$ – furor Oct 30 '14 at 2:20
  • \$\begingroup\$ @furor: inheritance is really a side issue. The issues that matter are (a) number of objects implementing the interface ; if 2 or more then you most likely need the interface, and (b) how many assemblies use the interface, again, if 2 or more then you need an interface (or possibly a new design). ORM libraries might use interfaces to do their work as a concrete example. \$\endgroup\$ – jmoreno Oct 31 '14 at 3:38

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.