Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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. What is the recommended or standard way of doing this?

  2. If none of these methods are okay, kindly suggest one with code?

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.

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)
share|improve this question
    
you should separate these into 4 separate questions I think. and then see what answers you get from each of them, then you could better decide which one you like the best. –  Malachi Apr 12 at 3:33
1  
@Malachi that was also my first/original thought, but it can be reviewed as a whole. –  Mat's Mug Apr 12 at 3:37
    
@Mat'sMug, I read a couple and decided to answer...lol –  Malachi Apr 12 at 3:46
    
I like that you're using parameterized queries. –  Mat's Mug Apr 12 at 4:16
1  
Consider using an ORM, like Entity Framework. –  svick Apr 12 at 17:26
add comment

2 Answers 2

up vote 14 down vote accepted

None of the Above.

You've shown 4 different ways of mixing up presentation and data concerns; the 4 approaches only differ in minute implementation details and all suffer the same flaws - what you have here is a God class that knows everything about everything there is to know.

Let's think differently.


This answer uses the names I would have used instead of yours.


We have at least 3 distinct, separate things:

  • We have a Form that knows about its textboxes.

  • We have a BankAccountInfo that holds an AccountNumber, an AccountName, an AccountType the BankName and an EmployeeName.

  • We have a way to fetch data from the database, here ADO.NET.

Each of these 3 concepts is its own class - you have the Form and the BankAccount, but you've turned the bank account into some "coordinator" that knows about the Form and deals with fetching the data from the database.

Leaving the form aside, BankAccountInfo should look like this:

public class BankAccountInfo
{
    public string AccountNumber { get; set; }
    public string AccountName { get; set; }
    public string AccountType { get; set; }
    public string BankName { get; set; }
    public string EmployeeName { get; set; }
}

Where's the code? Elsewhere. It's not the role of this object to know about textboxes, nor about some SQL backend. It has only one, simple goal: expose data. Not fetch data - that's the role of a service class:

public class BankAccountDataService
{
    private readonly string _connectionString;
    public BankAccountDataService(string connectionString)
    {
        _connectionString = connectionString;
    }

    public BankAccountInfo GetByAccountNumber(string accountNumber)
    {
        using (var connection = new SqlConnection(_connectionString))
        {
            var sql = "SELECT ...";
            connection.Open();
            using (var command = new SqlCommand(sql, connection))
            {
                command.Parameters.AddWithValue(...);
                using (var reader = command.ExecuteReader())
                {
                    if (reader.Read())
                    {
                        return new BankAccountInfo
                                   {
                                       EmployeeName = reader[0].ToString(),
                                       AccountNumber = reader[1].ToString(),
                                       AccountName = reader[2].ToString(),
                                       BankName = reader[3].ToString(),
                                       AccountType = reader[4].ToString()
                                   }; 
                    }
                }
            }
        }

        return null;
    }
}

Now you have a class whose role is specifically to interact with the database; its interface gives you a GetByAccountNumber method that lets you fetch everything there is to know about a BankAccountInfo by passing in an account number. It knows nothing of a form, and it doesn't care how the returned object is being used, it's none of its concern.

This is where you have a design decision to make: you need to somehow "connect" these pieces together. Let's see:

  • the form really only needs to know about BankAccountInfo.
  • the BankAccountInfo doesn't need to know about BankAccountDataService.
  • the BankAccountDataService needs to know about BankAccountInfo (and any other type it might return).

We're missing something else.

public class BankAccountPresenter
{
    private BankAccountView _form;
    private BankAccountDataService _service;

    public BankAccountPresenter(BankAccountView form, BankAccountDataService service)
    {
        _form = form;
        _form.OnShowAccountInfo += View_OnShowAccountInfo;

        _service = service;
    }

    public void Show()
    {
        _form.ShowDialog();
    }

    private View_OnShowAccountInfo(object sender, EventArgs e)
    {
        var info = _service.GetByAccountNumber(_form.AccountNumber);
        if (info == null)
        {
            MessageBox.Show("Could not find an account for specified number.");
            return;
        }

        _form.AccountName = info.AccountName;
        _form.AccountType = info.AccountType;
        _form.BankName = info.BankName;
    }
}

The above assumes that there's another class that's calling the presenter's Show method (that class will own the form and the service instances), and that the BankAccountView class looks something like this:

public partial class BankAccountView : Form
{
    public event EventHandler OnShowAccountInfo;

    public string AccountNumber
    {
        get { return AccountNumberTextBox.Text; }
        set { AccountNumberTextBox.Text = value; }
    }

    public string AccountName
    {
        get { return AccountNameTextBox.Text; }
        set { AccountNameTextBox.Text = value; }
    }

    public string AccountType
    {
        get { return AccountTypeTextBox.Text; }
        set { AccountTypeTextBox.Text = value; }
    }

    public string BankName
    { 
        get { return BankNameTextBox.Text; } 
        set { BankNameTextBox.Text = value; }
    }

    public BankAccountView()
    {
        InitializeComponents();
    }

    private void GetAccountInfoButton_Click(object sender, EventArgs e)
    {
        if (OnShowAccountInfo != null)
        {
            OnShowAccountInfo(this, EventArgs.Empty);
        }
    }
}

What's going on here? The view does nothing other than displaying the data and notifying the presenter whenever it needs something to happen - here, the user clicked a button: the presenter does all the work, fetching the required model from the database using an instance of a service class. Everyone plays its own little part, and everything is properly encapsulated - you don't expose the form's TextBox members, rather, you expose a string that represents that textbox' value.


Your Code

I already commented on that, but I'll reiterate it: I like that you're using parameters. Lots of beginners will just concatenate them into the command string, and that tends to make my eyes bleed that's rather ugly.

Here's a brain-dump of what jumped at me while I was reviewing your code:

  • Java-style naming. camelCase in C# is for locals (parameters, etc.) - class, method and property names use PascalCase.
  • Hungarian Notation and Disemvoweling. Don't chop off vowels in your identifiers. Use names that you can pronounce. Since this is WinForms there's some kind of a historical consensus about the accepted semi-Hungarian naming style for controls though.
  • Undisposed disposables. You're using using blocks and that's great, however you're not consistently disposing all objects that implement IDisposable, that's less great.
  • Misleading names. The ViewAccount(string) method doesn't actually display anything, it fetches information. Do what you say, say what you do.
share|improve this answer
    
Your answer is very helpful. Before accepting it I have one thing to know. In my system I have several entity classes like employee, client etc, having to interact with db. So that should I write separate DataService and presenter classes for each? –  Chathuranga Apr 12 at 15:43
1  
I'd probably make a presenter per form, and have the service class expose everything that a form needs to use; if things start getting tangled, then I'd refactor / extract classes as needed, to keep responsibilities clear. Notice there's no Employee class here, BankAccountInfo isn't tied to a single data table - it represents a 'model' that the view is using. The service class can tell which information from that 'model' belongs in which db table. Check out the 'Model-View-Presenter' pattern (sorry no link, phone post!) ;) –  Mat's Mug Apr 12 at 15:53
2  
Very good answer, only thing I'd add is as the system goes you'll want more of these services (or repositories as they're commonly called), and you'll have other services in between when you need to abstract away complex combinations of repositories. –  Mitchell Lee Apr 12 at 23:15
    
Now presenting a model from database to UI is clear. But if we have some thing to do with the bankAccountInfo before presenting it to UI, ( a business logic ) where to put that code? In bankAccountInfo class? –  Chathuranga Apr 13 at 4:19
1  
@Chathur that can depend on many things, but you'll probably want to look into Program.cs. Notice how I don't new up things but instead receive them in from the constructor? This is called constructor injection; look for "Dependency Injection" if you like this approach. Basically you'll new up all these objects near the application's entry point. As for busines logic, it can go in the presenter class, and be refactored into its own class if needed. Hope it helps! –  Mat's Mug Apr 13 at 14:59
show 2 more comments

with all of the methods you should add another using statement for your Readers as well.

right now you aren't closing your reader, I am not sure that the reader is closed inside the using block of the connection, but I would imagine it should be, logically you can't have the reader after the connection is closed so I would think it would close the reader automatically. The only thing with that is you don't see it in the code so you can't be sure that is what it is doing.

If I were you I would just wrap it in a using statement.


you should also use a using statement with

SqlCommand newCmd = new SqlCommand("Same SELECT …)", newCon);

like this

using (SqlConnection newCon = new SqlConnection(db.GetConnectionString)) {
    using (SqlCommand newCmd = new SqlCommand("Same SELECT …)", newCon) {
        newCmd.Parameters.Add()…
        newCon.Open();
        Using (SqlDataReader rdr = newCmd.ExecuteReader()) {
            rdr.Read();

            form.txtName.text=rdr[0];
            this.acName=rdr[1];
            this.bank=rdr[2];
            this.acType=rdr[3];
        }
    }
}
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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