Problems with Open/Close sql connection

Question

I hope you forgive my beginner's head. I am having some problems with a code designed to open a connect to a SQL Server database.

As you can see frmManageAppointment form is loaded from a different Winform. So far so good. Upon loading, the Connect() part of the code kicks in and connects to my SQL Server database. So far so good.

I have a second script that load upon pressing the Save button on the form (basically saving a variety of textboxes and such). My guess is that the second instance of conn in the MakeAppointment code somehow closed the connection, based on the error I am getting.

I know I am not handling the script correctly so I would appreciate some help.

namespace Scheduler1
{
    public partial class frmManageAppointment : Form
    {
        public frmManageAppointment()
        {
            InitializeComponent();
        }

        private void frmManageAppointment_Load(object sender, EventArgs e)
        {
            Connect();

            txtDBConnection.Text = "Connected";
        }

        private void ExitButton_Click(object sender, EventArgs e)
        {
            this.Close();
        }

        private void SaveButton_Click(object sender, EventArgs e)
        {
            if (string.IsNullOrWhiteSpace(txtSurname.Text))
            {
                MessageBox.Show("Απαιτείται το Επίθετο του Εξεταζόμενου!");
                return;
            }

            if (MakeAppointment())
            {
                MessageBox.Show("Επιτυχής Προσθήκη!");
            }
            else
            {
                MessageBox.Show("Ανεπιτυχής Προσθήκη");
            }
        }

        public void Connect()
        {
            string conString = "Data Source=DIMITRIS-PC\\DIMITRISSQL;Initial Catalog=AppointmentDB;Integrated Security=True";
            SqlConnection conn = new SqlConnection(conString);

            conn.Open();

            if (conn.State == System.Data.ConnectionState.Closed)
            {
                MessageBox.Show("Closed");
            }
            else if (conn.State == System.Data.ConnectionState.Open)
            {
                MessageBox.Show("Open");
            }
        }

        public Boolean MakeAppointment()
        {
            string conString = "Data Source=DIMITRIS-PC\\DIMITRISSQL;Initial Catalog=AppointmentDB;Integrated Security=True";
            SqlConnection conn = new SqlConnection(conString);
            string sql = "INSERT INTO Appointmentdbo(Date, Time, Name, Surname, DOB, PatientID, Comments) VALUES ('" + dtpDate.Value.Date.ToString("MM/dd/yyyy") + "','" + comboTime.Text + "','" + txtName.Text + "','" + txtSurname.Text + "','" + dtpDOB.Value.Date.ToString("MM/dd/yyyy") + "','" + txtID.Text + "','" + txtComments.Text + "')";

            SqlCommand cmd = new SqlCommand(sql, conn);

            return cmd.ExecuteNonQuery() > 0;
        }
    }
} 

Answer 1

As the comments pointed out, you're creating a connection (the conn object in Connect(), opening it, and then conn goes out of scope. It's the conn object here that IS the open connection.

Here's the shortest way to get this to work, but as the comments also state, this isn't really that great, either. I laid this out to explain exactly what you were doing wrong/why what you had wasn't working. Below this I have an example with using statements.

public class frmManageAppointment : Form {

    SqlConnection conn; //private variable available to all methods in this class

    private void frmManageAppointment_Load(object sender, EventArgs e)
    {
        Connect();
    }

    private void ExitButton_Click(object sender, EventArgs e)
    {
        this.CloseConn(); //new method here
        this.Close();
    }

    void CloseConn() {
        //we want to close/dispose the connection obect
        conn.Close();
        conn.Dispose();
    }


    public void Connect()
    {
        string conString = "<constring>";
        conn = new SqlConnection(conString); //removed type here so conn now the class level variable
        conn.Open();
        //rest of code snipped        
    }

    public Boolean MakeAppointment()
    {
        string sql = "<sql string>"; //btw, the way you're generating this string is ... not ideal.  Google sql injection
        SqlCommand cmd = new SqlCommand(sql, conn); //conn is our global variable, which we're assuming here is still open/valid (not ideal)
        return cmd.ExecuteNonQuery() > 0;
    }

}

The problem with this is that your form will be open for an indefinite length of time. This probably won't be an issue with a small app connecting to a local database where the app is the only thing connecting, but leaving the connection open wastes server resources and you have no guarantee that the conn object is still valid when you later try to use it in MakeAppointment(). You could add checks that it is actually open and call Connect() again if it's not, but that's a bunch of work for no gain. A better pattern/habit to adopt when working with databases (or any network resource) is to only open them when you need to do work and close/release them as soon as you're done. So that said, here's what your code would look like.

public class frmManageAppointment : Form {

    //no need for a Connect() method, conn global variable, or a CloseConn() method
    //your other methods pertaining to the UI would need to be copied over

    public Boolean MakeAppointment()
    {
        string sql = "<sql string>"; //again, what you're doing here is bad
        using(SqlConnection conn = new SqlConnection("<connectionstring>") {
            conn.Open();
            using(SqlCommand cmd = new SqlCommand(sql, conn)) {
                return cmd.ExecuteNonQuery() > 0;
            }
        }
    }
}

You only open the connection immediately before using it (the only place you're doing work in this example is in MakeAppointment()), and by wrapping the connection and command in using statements, they're automatically disposed of (by calling those objects Dispose() method) as soon as they go out of scope.