0

I'm a beginner to .net and could you guide me to right direction. My problem is based on the following code. Here I have 4 variations of same method and all 4 variations are working fine.

  1. I just want to know what is the recommended or standard way of doing this?

  2. Are all these forms of method ok?

Code explanation:
From a windows form I'm calling a viewAccount() method which is in bankAccount class. Its purpose is to get relevant bank account details of an employee from the database and then those details should be shown in the text boxes of calling form.

Also please note that I have reduced no of line to make it more readable. Appreciate your any help towards the right direction.

Thank you.

Example 01 - Method will return a bankAccount object with fields populated with data from the database

class bankAccount
{
    //Member fields...
    string acNo;
    string acName;
    string bank;
    string acType;
    frmShowAccount form=new frmShowAccount();

    public bankAccount viewAccount( string acNo )
    {
        this.acNo = acNo;

        using (SqlConnection newCon = new SqlConnection(db.GetConnectionString))
        {
            SqlCommand newCmd = new SqlCommand("SELECT Employee.Name, BankAccount.ac_name, BankAccount.bank_name, BankAccount.ac_type FROM BankAccount INNER JOIN Employee ON BankAccount.emp_id = Employee.Emp_ID WHERE (BankAccount.ac_no = @bankAccount)", newCon);

            newCmd.Parameters.Add("@bankAccount", SqlDbType.Char).Value = acNo;
            newCon.Open();
            SqlDataReader rdr = newCmd.ExecuteReader();
            rdr.Read();

            form.txtEmpName.text = rdr.GetString(0); //EmpName is not a member of bankAccount class
            this.acName = rdr.GetString(1);
            this.bank = rdr.GetString(2);
            this.acType = rdr.GetString(3);

            return this;
        }
    }
}

// CALLING THE ABOVE METHOD...

bankAccount newBA = new bankAccount();
newBA = newBA.viewAccount(txtACNo.text);  // A reference is set to the instance returned
txtACName.text = newBA.acName;  // Get the value of instance field to text box

Example 02 - Method will return a data reader and it will be used by the form to get data

    class bankAccount
    {
        string acNo;
        string acName;
        string bank;
        string acType;

        public SqlDataReader viewAccount( string acNo )
        {
            this.acNo = acNo;

            using (SqlConnection newCon = new SqlConnection(db.GetConnectionString))
            {
                SqlCommand newCmd = new SqlCommand("Same SELECT …”,newCon);

                newCmd.Parameters.Add()…
                newCon.Open();
                SqlDataReader rdr = newCmd.ExecuteReader();
                rdr.Read();

                return rdr;
            }
        }
    }

//CALLING THE ABOVE METHOD...
bankAccount newBA = new bankAccount();
SqlDataReader rdr = newBA.viewAccount(txtACNo.text) //A reference to hold the returning reader from the method call
txtACName.text = rdr.getString(1); //Get the value through the reader to text box

Example 03: this method want return values but explicitly assign values to the text boxes in the form

  class bankAccount
  {
        string acNo;
        string acName;
        string bank;
        string acType;
        frmShowAccount form=new frmShowAccount();

        public void viewAccount( string acNo )
        {
            this.acNo = acNo;

            using (SqlConnection newCon = new SqlConnection(db.GetConnectionString))
            {
                SqlCommand newCmd = new SqlCommand("Same SELECT …", newCon);

                newCmd.Parameters.Add()…
                newCon.Open();
                SqlDataReader rdr = newCmd.ExecuteReader();
                rdr.Read();

                // Setting values to the text boxes in the current instance of form
                form.txtName.text=rdr[0];
                form.txtACName.text=rdr[1];
                form.txtBankName.text=rdr[2];
                form.txtACType.text=rdr[3];         
            }
        }
    }

//CALLING THE ABOVE METHOD
bankAccount newBA = new bankAccount();
newBA.form.this; // reference 'form' which is in the 'bankAccount' class is set to current instance of the form object.

Example 04: this method want return any value. It will only initialize instance fields of the class with the data

    class bankAccount
    {
        string acNo;
        string acName;
        string bank;
        string acType;
        frmShowAccount form=new frmShowAccount();

        public void viewAccount( string acNo )
        {
            this.acNo = acNo;

            using (SqlConnection newCon = new SqlConnection(db.GetConnectionString))
            {
                SqlCommand newCmd = new SqlCommand("Same SELECT …)", newCon);

                newCmd.Parameters.Add()…
                newCon.Open();
                SqlDataReader rdr = newCmd.ExecuteReader();
                rdr.Read();

                form.txtName.text=rdr[0];
                this.acName=rdr[1];
                this.bank=rdr[2];
                this.acType=rdr[3];
        }
    }


// CALLING THE ABOVE METHOD
bankAccount newBA = new bankAccount();
txtACName.text = newBA.acName; // Text boxes get the data from account object's instance fields (probably through a get property)
7
  • Question is too big, not interested to read :( Consider making it concise. Apr 10, 2014 at 12:46
  • Suggest you look up separation of concerns. They are all unacceptable in my opinion. You'd be better off pointing this on codereview. Apr 10, 2014 at 12:52
  • 3
    This question appears to be off-topic because it is about code review Apr 10, 2014 at 12:52
  • Please refer How do I ask a good question? Apr 10, 2014 at 13:26
  • I would also put the SqlCommand and SqlDataReader around usings.
    – Mo Patel
    Apr 10, 2014 at 13:28

3 Answers 3

0

Option 1 is your best bet, but:-

a) The method accessing the database really should return a new BankAccount model object, rather than setting the properties of this:-

class BankAccountModel
{
  public string AccountNumber { get; set; }
  public string AccountName { get; set; }
  public string Bank { get; set; }
  public string AccountType { get; set; }

  public static BankAccountModel GetAccount(string accountNumber)
  {
    var account = new BankAccountModel()
    {
      AccountNumber = accountNumber,
    };

    using (SqlConnection newCon = new SqlConnection(db.GetConnectionString))
    {
        ...

        account.AccountName = rdr.GetString(1);
        account.Bank = rdr.GetString(2);
        account.AccountType = rdr.GetString(3);
    }

    return account;
  }
}

b) If you're making a WPF/Winforms/Webforms application, you'll want to investigate Data Binding rather than manually setting the values of your controls to the values of your model.

var account = BankAccountModel.GetAccount(accountNumber);
myControl.DataSource = account;

(Or if you're making an ASP.NET MVC application, you just pass your model to the view):-

var model = BankAccountModel.GetAccount(accountNumber);
return View(model);

c) It's usually helpful if your model doesn't have to concern itself with the way that it's being persisted. Eventually you'll want to pull the "GetAccount" method out of your Model class and into some dedicated data access code (e.g. a BankAccountRepository). This gives you better flexibility for situations where you need to control the lifecycle of your managed resources (e.g. the database connection), or if you need to obtain BankAccountModels from multiple sources:-

class BankAccountModel
{
  public string AccountNumber { get; set; }
  public string AccountName { get; set; }
  public string Bank { get; set; }
  public string AccountType { get; set; }
}

class BankAccountRepository
{
  public BankAccountModel GetAccount(string AccountNumber)
  {
    ...
  }
}
2
  • Thank you very much for your idea. In my project I have several entity classes like Employee, Client etc. So for each class should I use separate class for db logic or a general one for all? Chathur.
    – CAD
    Apr 12, 2014 at 3:24
  • I use separate repositories per entity type, but on the grand scale of things it's not terribly important. Apr 22, 2014 at 16:03
0

I would say none of the above
You are mixing the ui, data layer, and business layer

for sure bankAccount should not access the ui directly

2 is the closest
but you should pass values to the ctor
the ctor should not access the database directly

have separate method to create the class
this method access the db

public bankAccount GetbankAccount(int acNo)   
{
    // access db 
    // create bankAccount 
}
0

Don't mix your data model with the UI, it will become a nightmare to maintain and evolve. Keep different and independent concerns - keeping and manipulating data in memory, fetching data from and writing data to the database and presenting data to the user - separated. Therefore option three is bad.

Option two is not good either because you really want to encapsulate your low lever database interaction code and not have to deal with data readers all over the place.

Options one and four are both okay, see the next paragraph, but both still have issues and I would not recommend using them. This is probably not really helpful but there are just so many things to say and consider - database context management, data modeling, view modeling, data binding, ... - that I can not properly address everything in an answer. If you are really interested in the best solution - there is no single best solution - I suggest to take invest several weeks and just read a lot of related articles, blogs, tutorials, manuals and so on to get an idea what tools, ideas, problems and solutions are out there and then you can better judge and decide what you need for your case. But it will really take weeks to just to get a rough overview of what is out their.

It usually is a good idea to also separate keeping data in memory and fetching data from the database. There is a design pattern called active record that puts the database access part on the data classes. This gives you code like

var user = User.GetById(42);

user.Name = "John Doe";

user.Save();

instead of

var database = new Database();

var user = database.GetById<User>(42);

user.Name = "John Doe";

database.Save(user);

and is usually more natural and easier to read. The main drawback is that it gets harder to separate concerns. You may want to have a look design patterns for data persistence. After all you usually just use an O/R mapper like Entity Framework or NHibernate if you prefer the active record pattern. The only good reason for writing your own database access layer - besides fun and education - is that you need every bit of performance.

Not the answer you're looking for? Browse other questions tagged or ask your own question.