2
\$\begingroup\$

This is a commonly used format for our save button for a lot of our pages. I parameterized (to prevent SQL injection) a lot of this. For this page, there's a ton of textboxes and other stuff, so excuse the length.

Can I do this differently? This accounts for the add of a new record or after an edit of an existing one. One other problem, my entryDate keeps saving as a date with time after it like "10-27-2015 0:00:00" even though I made the field as a Date type in SQL and tried formatting with it below. Why? Important to note that it's a label just informing the user when they entered the record. Normally we use the misc_class to format dates in a textbox.

One other note: I took out some extra code that involved other tabs and tables so if I'm off a squiggly bracket, I apologize.

protected void SaveData(String addedit)
{
    cookie = Request.Cookies["pAuthCookie"];
    string empno = cookie["thenum"].ToString();
    string thejunk = cookie["email_user"].ToString();
    string thelock = thejunk.Trim() + empno.Trim();

    string strError = "";
    try
    {
        String ltxtMeetDate = misc_class.format_mmddyyyy(txtMeetDate.Text);
        if (ltxtMeetDate == "")
            ltxtMeetDate = null;
        String ltxtiepstart = misc_class.format_mmddyyyy(txtiepstart.Text);
        if (ltxtiepstart == "")
            ltxtiepstart = null;
        String ltxtiepend = misc_class.format_mmddyyyy(txtiepend.Text);
        if (ltxtiepend == "")
            ltxtiepend = null;
        String ltxtcsetype = txtcsetype.Text;
        String ltxtReviewandDisc = txtReviewandDisc.Text;
        String ltxtConcerns = txtConcerns.Text;
        String lFollowupComments = txtComments.Text;
        Boolean lchkAide = chk1Aide1.Checked;
        Boolean lchkC2C = chkC2C.Checked;
        Boolean lchkPT = chkPT.Checked;
        Boolean lchkWC = chkWC.Checked;
        Boolean lchkAC = chkAC.Checked;
        Boolean lchkHarness = chkHarness.Checked;
        Boolean lchkCarseat = chkCarseat.Checked;
        Boolean lchkLastOn = chkLastOn.Checked;
        Boolean lchkNurse = chkNurse.Checked;
        Boolean lchkAide2 = chkAide.Checked;
        Boolean lcompleted = chkCompleted.Checked;
        Boolean bVerified = chkVerified.Checked;
        String strComments = txtComment.Text;
        String strHCID = DDLFullName.SelectedValue;
        String strDistrict = ddlschool_d.SelectedValue;
    Datetime today = Datetime.Today;
    String entryDate = today.ToString("d");

    //new record insert
        if (addedit == "ADD")
        {
            var connect = ConfigurationManager.ConnectionStrings["theConnectionString"].ToString();
            var query = "INSERT INTO tablename (arcfk,meet_date,iep_start,iep_end,school_dfk,cse_type,aide_1to1,curbtocurb,Parent,wheelchair,ac_on_bus,harness,carseat,lastonfirstoff,nurseonbus,pers_aide,num_months,pt_ft,recommendations,parent_concerns,districtsend,eval_req,her_eval,dist_auth,fam_consent,comments,follow_up,evalformby,hep_rep,others_pres,disciplines,empfk,tablelock,IEPDirect,completed,entrydate) VALUES (@arcfk,@meet_date,@iep_start,@iep_end,@school_dfk,@cse_type,@aide_1to1,@curbtocurb,@Parent,@wheelchair,@ac_on_bus,@harness,@carseat,@lastonfirstoff,@nurseonbus,@pers_aide,@num_months,@pt_ft,@recommendations,@parent_concerns,@districtsend,@eval_req,@her_eval,@dist_auth,@fam_consent,@comments,@follow_up,@evalformby,@hep_rep,@others_pres,@disciplines,@empfk,@tablelock,@IEPDirect,@completed,@entrydate)";
                using (var conn = new SqlConnection(connect))
                {
                    using (var cmd = new SqlCommand(query, conn))
                    {
                        cmd.Parameters.Add("@arcfk", SqlDbType.Int);
                        cmd.Parameters.Add("@meet_date", SqlDbType.Date);
                        cmd.Parameters.Add("@iep_start", SqlDbType.Date);
                        cmd.Parameters.Add("@iep_end", SqlDbType.Date);
                        cmd.Parameters.Add("@school_dfk", SqlDbType.Int);
                        cmd.Parameters.Add("@cse_type", SqlDbType.Int);
                        cmd.Parameters.Add("@aide_1to1", SqlDbType.Bit);
                        cmd.Parameters.Add("@curbtocurb", SqlDbType.Bit);
                        cmd.Parameters.Add("@Parent", SqlDbType.Bit);
                        cmd.Parameters.Add("@wheelchair", SqlDbType.Bit);
                        cmd.Parameters.Add("@ac_on_bus", SqlDbType.Bit);
                        cmd.Parameters.Add("@harness", SqlDbType.Bit);
                        cmd.Parameters.Add("@carseat", SqlDbType.Bit);
                        cmd.Parameters.Add("@lastonfirstoff", SqlDbType.Bit);
                        cmd.Parameters.Add("@nurseonbus", SqlDbType.Bit);
                        cmd.Parameters.Add("@pers_aide", SqlDbType.Bit);
                        cmd.Parameters.Add("@num_months", SqlDbType.Int);
                        cmd.Parameters.Add("@pt_ft", SqlDbType.Char);
                        cmd.Parameters.Add("@recommendations", SqlDbType.VarChar);
                        cmd.Parameters.Add("@parent_concerns", SqlDbType.VarChar);
                        cmd.Parameters.Add("@districtsend", SqlDbType.Bit);
                        cmd.Parameters.Add("@eval_req", SqlDbType.Bit);
                        cmd.Parameters.Add("@her_eval", SqlDbType.Bit);
                        cmd.Parameters.Add("@dist_auth", SqlDbType.Bit);
                        cmd.Parameters.Add("@fam_consent", SqlDbType.Bit);
                        cmd.Parameters.Add("@comments", SqlDbType.VarChar);
                        cmd.Parameters.Add("@follow_up", SqlDbType.VarChar);
                        cmd.Parameters.Add("@IEPDirect", SqlDbType.Date);
                        cmd.Parameters.Add("@evalformby", SqlDbType.VarChar);
                        cmd.Parameters.Add("@hep_rep", SqlDbType.VarChar);
                        cmd.Parameters.Add("@others_pres", SqlDbType.VarChar);
                        cmd.Parameters.Add("@disciplines", SqlDbType.VarChar);
                        cmd.Parameters.Add("@empfk", SqlDbType.Int);
                        cmd.Parameters.Add("@tablelock", SqlDbType.Char);
                        cmd.Parameters.Add("@completed", SqlDbType.Bit);
                        cmd.Parameters.Add("@entrydate", SqlDbType.Date);

                        cmd.Parameters["@arcfk"].Value = Convert.ToInt32(strHCID);
                        cmd.Parameters["@school_dfk"].Value = Convert.ToInt32(strDistrict);
                        if (ltxtMeetDate != null)
                        {
                            cmd.Parameters["@meet_date"].Value = ltxtMeetDate;
                        }
                        else
                        {
                            cmd.Parameters["@meet_date"].Value = DBNull.Value;
                        }
                        if (ltxtiepstart != null)
                        {
                            cmd.Parameters["@iep_start"].Value = ltxtiepstart;
                        }
                        else
                        {
                            cmd.Parameters["@iep_start"].Value = DBNull.Value;
                        }
                        if (ltxtiepend != null)
                        {
                            cmd.Parameters["@iep_end"].Value = ltxtiepend;
                        }
                        else
                        {
                            cmd.Parameters["@iep_end"].Value = DBNull.Value;
                        }
                        if (String.IsNullOrEmpty(ltxtcsetype) == false)
                            cmd.Parameters["@cse_type"].Value = Convert.ToInt32(ltxtcsetype);
                        else
                            cmd.Parameters["@cse_type"].Value = DBNull.Value;
                        cmd.Parameters["@aide_1to1"].Value = lchkAide;
                        cmd.Parameters["@curbtocurb"].Value = lchkC2C;
                        cmd.Parameters["@Parent"].Value = lchkPT;
                        cmd.Parameters["@wheelchair"].Value = lchkWC;
                        cmd.Parameters["@ac_on_bus"].Value = lchkAC;
                        cmd.Parameters["@harness"].Value = lchkHarness;
                        cmd.Parameters["@carseat"].Value = lchkCarseat;
                        cmd.Parameters["@lastonfirstoff"].Value = lchkLastOn;
                        cmd.Parameters["@nurseonbus"].Value = lchkNurse;
                        cmd.Parameters["@pers_aide"].Value = lchkAide2;
                        cmd.Parameters["@num_months"].Value = Int32.Parse(rblClassification.SelectedValue);
                        cmd.Parameters["@pt_ft"].Value = rblStatus.SelectedValue;
                        cmd.Parameters["@recommendations"].Value = ltxtReviewandDisc;
                        cmd.Parameters["@parent_concerns"].Value = ltxtConcerns;

                        if (RadioButton1.Checked == true)
                            cmd.Parameters["@districtsend"].Value = true;
                        else if (RadioButton2.Checked == true)
                            cmd.Parameters["@districtsend"].Value = false;
                        else cmd.Parameters["@districtsend"].Value = DBNull.Value;

                        if (RadioButton3.Checked == true)
                            cmd.Parameters["@eval_req"].Value = true;
                        else if (RadioButton4.Checked == true)
                            cmd.Parameters["@eval_req"].Value = false;
                        else cmd.Parameters["@eval_req"].Value = DBNull.Value;

                        if (RadioButton5.Checked == true)
                            cmd.Parameters["@her_eval"].Value = true;
                        else if (RadioButton6.Checked == true)
                            cmd.Parameters["@her_eval"].Value = false;
                        else cmd.Parameters["@her_eval"].Value = DBNull.Value;

                        if (RadioButton7.Checked == true)
                            cmd.Parameters["@dist_auth"].Value = true;
                        else if (RadioButton8.Checked == true)
                            cmd.Parameters["@dist_auth"].Value = false;
                        else cmd.Parameters["@dist_auth"].Value = DBNull.Value;

                        if (RadioButton9.Checked == true)
                            cmd.Parameters["@fam_consent"].Value = true;
                        else if (RadioButton10.Checked == true)
                            cmd.Parameters["@fam_consent"].Value = false;
                        else cmd.Parameters["@fam_consent"].Value = DBNull.Value;

                        cmd.Parameters["@comments"].Value = strComments;
                        cmd.Parameters["@follow_up"].Value = lFollowupComments;

                        String strIEPDirect = misc_class.format_mmddyyyy(txtShareDate.Text);
                        if (strIEPDirect == "")
                            strIEPDirect = null;

                        if (string.IsNullOrEmpty(strIEPDirect) == false)
                            cmd.Parameters["@IEPDirect"].Value = strIEPDirect;
                        else
                        {
                            cmd.Parameters["@IEPDirect"].Value = DBNull.Value;
                        }
                        cmd.Parameters["@evalformby"].Value = txtEvalCompletedBy.Text;
                        cmd.Parameters["@hep_rep"].Value = txtHEPREPAttending.Text;
                        cmd.Parameters["@others_pres"].Value = txtOthersAttending.Text;
                        cmd.Parameters["@disciplines"].Value = txtDiscipline.Text;
                        cmd.Parameters["@empfk"].Value = empno;
                        cmd.Parameters["@tablelock"].Value = thelock;
                        cmd.Parameters["@completed"].Value = lcompleted;
                        cmd.Parameters["@entrydate"].Value = lentryDate;
                        conn.Open();
                        cmd.ExecuteNonQuery();
                    }
                }
            }
        else
        {  /** save on an EDIT*/
                Int32 fieppk = Convert.ToInt32(TxtKey.Text);
                var connect = ConfigurationManager.ConnectionStrings["theConnectionString"].ToString();
                var query = "UPDATE tablename SET [num_months]=@num_months,[pt_ft]=@pt_ft,[meet_date] = @meet_date, [iep_start] = @iep_start, [iep_end] = @iep_end, [cse_type] = @cse_type, [aide_1to1] = @aide_1to1, [curbtocurb] = @curbtocurb, [Parent] = @Parent, [wheelchair] = @wheelchair, [ac_on_bus] = @ac_on_bus, [harness] = @harness, [carseat] = @carseat, [lastonfirstoff] = @lastonfirstoff, [nurseonbus] = @nurseonbus, [pers_aide] = @pers_aide, [recommendations] = @recommendations, [parent_concerns] = @parent_concerns,[districtsend]=@districtsend,[eval_req]=@eval_req,[her_eval]=@her_eval,[dist_auth]=@dist_auth,[fam_consent]=@fam_consent,[comments]=@comments,[follow_up]=@follow_up,[IEPDirect]=@IEPDirect,[evalformby]=@evalformby,[hep_rep]=@hep_rep,[others_pres]=@others_pres,[disciplines]=@disciplines,[empfk]=@empfk,[tablelock]=@tablelock,[completed]=@completed where ieppk=" + fieppk.ToString();
                using (var conn = new SqlConnection(connect))
                {
                    using (var cmd3 = new SqlCommand(query, conn))
                    {
                        cmd3.Parameters.Add("@num_months", SqlDbType.Int);
                        cmd3.Parameters.Add("@pt_ft", SqlDbType.Char);
                        cmd3.Parameters.Add("@meet_date", SqlDbType.Date);
                        cmd3.Parameters.Add("@iep_start", SqlDbType.Date);
                        cmd3.Parameters.Add("@iep_end", SqlDbType.Date);
                        cmd3.Parameters.Add("@school_dfk", SqlDbType.Int);
                        cmd3.Parameters.Add("@cse_type", SqlDbType.Int);
                        cmd3.Parameters.Add("@aide_1to1", SqlDbType.Bit);
                        cmd3.Parameters.Add("@ieppk", SqlDbType.Int);
                        cmd3.Parameters.Add("@curbtocurb", SqlDbType.Bit);
                        cmd3.Parameters.Add("@Parent", SqlDbType.Bit);
                        cmd3.Parameters.Add("@wheelchair", SqlDbType.Bit);
                        cmd3.Parameters.Add("@ac_on_bus", SqlDbType.Bit);
                        cmd3.Parameters.Add("@harness", SqlDbType.Bit);
                        cmd3.Parameters.Add("@carseat", SqlDbType.Bit);
                        cmd3.Parameters.Add("@lastonfirstoff", SqlDbType.Bit);
                        cmd3.Parameters.Add("@nurseonbus", SqlDbType.Bit);
                        cmd3.Parameters.Add("@pers_aide", SqlDbType.Bit);
                        cmd3.Parameters.Add("@recommendations", SqlDbType.VarChar);
                        cmd3.Parameters.Add("@parent_concerns", SqlDbType.VarChar);
                        cmd3.Parameters.Add("@districtsend", SqlDbType.Bit);
                        cmd3.Parameters.Add("@eval_req", SqlDbType.Bit);
                        cmd3.Parameters.Add("@her_eval", SqlDbType.Bit);
                        cmd3.Parameters.Add("@dist_auth", SqlDbType.Bit);
                        cmd3.Parameters.Add("@fam_consent", SqlDbType.Bit);
                        cmd3.Parameters.Add("@comments", SqlDbType.VarChar);
                        cmd3.Parameters.Add("@follow_up", SqlDbType.VarChar);
                        cmd3.Parameters.Add("@IEPDirect", SqlDbType.Date);
                        cmd3.Parameters.Add("@evalformby", SqlDbType.VarChar);
                        cmd3.Parameters.Add("@hep_rep", SqlDbType.VarChar);
                        cmd3.Parameters.Add("@others_pres", SqlDbType.VarChar);
                        cmd3.Parameters.Add("@disciplines", SqlDbType.VarChar);
                        cmd3.Parameters.Add("@empfk", SqlDbType.Int);
                        cmd3.Parameters.Add("@tablelock", SqlDbType.Char);
                        cmd3.Parameters.Add("@completed", SqlDbType.Bit);

                        if (ltxtMeetDate != null)
                        {
                            cmd3.Parameters["@meet_date"].Value = ltxtMeetDate;
                        }
                        else
                        {
                            cmd3.Parameters["@meet_date"].Value = DBNull.Value;
                        }
                        if (ltxtiepstart != null)
                        {

                            cmd3.Parameters["@iep_start"].Value = ltxtiepstart;
                        }
                        else
                        {
                            cmd3.Parameters["@iep_start"].Value = DBNull.Value;
                        }
                        if (ltxtiepend != null)
                        {

                            cmd3.Parameters["@iep_end"].Value = ltxtiepend;
                        }
                        else
                        {
                            cmd3.Parameters["@iep_end"].Value = DBNull.Value;
                        }
                        cmd3.Parameters["@school_dfk"].Value = Convert.ToInt32(strDistrict);
                        cmd3.Parameters["@num_months"].Value = Int32.Parse(rblClassification.SelectedValue);
                        cmd3.Parameters["@pt_ft"].Value = rblStatus.SelectedValue;
                        if (String.IsNullOrEmpty(ltxtcsetype) == false)
                            cmd3.Parameters["@cse_type"].Value = Convert.ToInt32(ltxtcsetype);
                        else
                            cmd3.Parameters["@cse_type"].Value = DBNull.Value;

                        cmd3.Parameters["@aide_1to1"].Value = lchkAide;
                        cmd3.Parameters["@ieppk"].Value = fieppk;
                        cmd3.Parameters["@curbtocurb"].Value = lchkC2C;
                        cmd3.Parameters["@Parent"].Value = lchkPT;
                        cmd3.Parameters["@wheelchair"].Value = lchkWC;
                        cmd3.Parameters["@ac_on_bus"].Value = lchkAC;
                        cmd3.Parameters["@harness"].Value = lchkHarness;
                        cmd3.Parameters["@carseat"].Value = lchkCarseat;
                        cmd3.Parameters["@lastonfirstoff"].Value = lchkLastOn;
                        cmd3.Parameters["@nurseonbus"].Value = lchkNurse;
                        cmd3.Parameters["@pers_aide"].Value = lchkAide2;
                        cmd3.Parameters["@recommendations"].Value = ltxtReviewandDisc;
                        cmd3.Parameters["@parent_concerns"].Value = ltxtConcerns;

                        if (RadioButton1.Checked == true)
                            cmd3.Parameters["@districtsend"].Value = true;
                        else if (RadioButton2.Checked == true)
                            cmd3.Parameters["@districtsend"].Value = false;
                        else cmd3.Parameters["@districtsend"].Value = DBNull.Value;

                        if (RadioButton3.Checked == true)
                            cmd3.Parameters["@eval_req"].Value = true;
                        else if (RadioButton4.Checked == true)
                            cmd3.Parameters["@eval_req"].Value = false;
                        else cmd3.Parameters["@eval_req"].Value = DBNull.Value;

                        if (RadioButton5.Checked == true)
                            cmd3.Parameters["@her_eval"].Value = true;
                        else if (RadioButton6.Checked == true)
                            cmd3.Parameters["@her_eval"].Value = false;
                        else cmd3.Parameters["@her_eval"].Value = DBNull.Value;

                        if (RadioButton7.Checked == true)
                            cmd3.Parameters["@dist_auth"].Value = true;
                        else if (RadioButton8.Checked == true)
                            cmd3.Parameters["@dist_auth"].Value = false;
                        else cmd3.Parameters["@dist_auth"].Value = DBNull.Value;

                        if (RadioButton9.Checked == true)
                            cmd3.Parameters["@fam_consent"].Value = true;
                        else if (RadioButton10.Checked == true)
                            cmd3.Parameters["@fam_consent"].Value = false;
                        else cmd3.Parameters["@fam_consent"].Value = DBNull.Value;

                        cmd3.Parameters["@comments"].Value = txtComment.Text.ToString();
                        cmd3.Parameters["@follow_up"].Value = txtComments.Text.ToString();

                        String strIEPDirect = txtShareDate.Text;

                        if (String.IsNullOrEmpty(strIEPDirect) == false)
                            cmd3.Parameters["@IEPDirect"].Value = Convert.ToDateTime(strIEPDirect);
                        else
                        cmd3.Parameters["@IEPDirect"].Value = DBNull.Value;
                        cmd3.Parameters["@evalformby"].Value = txtEvalCompletedBy.Text;
                        cmd3.Parameters["@hep_rep"].Value = txtHEPREPAttending.Text;
                        cmd3.Parameters["@others_pres"].Value = txtOthersAttending.Text;
                        cmd3.Parameters["@disciplines"].Value = txtDiscipline.Text;
                        cmd3.Parameters["@empfk"].Value = empno;
                        cmd3.Parameters["@tablelock"].Value = thelock;
                        cmd3.Parameters["@completed"].Value = lcompleted;
                        conn.Open();
                        cmd3.ExecuteNonQuery();
                    }
                }
            }
        Gridview1.DataBind();
    }
    catch (Exception ex)
    {
        strError = ex.Message;
    }

}
\$\endgroup\$
3
\$\begingroup\$

This reads like VB6 code-behind, and in many ways it's extraordinarily similar to that - a single click handler that knows everything, does everything.

Take this chunk here:

    String ltxtMeetDate = misc_class.format_mmddyyyy(txtMeetDate.Text);
    if (ltxtMeetDate == "")
        ltxtMeetDate = null;
    String ltxtiepstart = misc_class.format_mmddyyyy(txtiepstart.Text);
    if (ltxtiepstart == "")
        ltxtiepstart = null;
    String ltxtiepend = misc_class.format_mmddyyyy(txtiepend.Text);
    if (ltxtiepend == "")
        ltxtiepend = null;
    String ltxtcsetype = txtcsetype.Text;
    String ltxtReviewandDisc = txtReviewandDisc.Text;
    String ltxtConcerns = txtConcerns.Text;
    String lFollowupComments = txtComments.Text;
    Boolean lchkAide = chk1Aide1.Checked;
    Boolean lchkC2C = chkC2C.Checked;
    Boolean lchkPT = chkPT.Checked;
    Boolean lchkWC = chkWC.Checked;
    Boolean lchkAC = chkAC.Checked;
    Boolean lchkHarness = chkHarness.Checked;
    Boolean lchkCarseat = chkCarseat.Checked;
    Boolean lchkLastOn = chkLastOn.Checked;
    Boolean lchkNurse = chkNurse.Checked;
    Boolean lchkAide2 = chkAide.Checked;
    Boolean lcompleted = chkCompleted.Checked;
    Boolean bVerified = chkVerified.Checked;
    String strComments = txtComment.Text;
    String strHCID = DDLFullName.SelectedValue;
    String strDistrict = ddlschool_d.SelectedValue;
Datetime today = Datetime.Today;
String entryDate = today.ToString("d");

You're reading controls into an important amount of locals, that collectively represent your model - which is a missed abstraction opportunity, where you get to abstract away all these funky UI identifiers into a nice little object that's only responsible for taking that state and conveying it to the "save" logic.

public class SomeMeaningfulIdentifierForThatModel
{
    public string FormattedMeetingDate { get; set; }
    public string FormattedIndependentEntryPointStartDate { get; set; }
    public string FormattedIndependentEntryPointEndDate { get; set; }
    public string CarSeatExtendedType { get; set; }
    //...
    public bool HasHarness { get; set; }
    public bool HasCarseat { get; set; }
    //...
    public bool IsCompleted { get; set; }
    public bool IsVerified { get; set; }
    public string Comments { get; set; }
    public string HumanComputerInteractionDesign { get; set; }
    public string District { get; set; }

    public string FormattedEntryDate { get { return DateTime.Today.ToString("d"); }
}

If some of these identifiers make no sense to you, rest assured that the corresponding identifiers made no sense to me in your code: avoid non-trivial and domain-specific abbreviations in code - a maintainer shouldn't need to go through the [non-existent?] documentation to understand what PT, WC, AC, C2C, CSE and IEP stand for.

Now, you can have a method that's solely responsible for taking UI controls' properties and returning an instance of that SomeMeaningfulIdentifierForThatModel class.


protected void SaveData(String addedit)

You're abusing the string type here.

if (addedit == "ADD")

You have two possible states. Either its "ADD" (but not "add" or "Add"), or it's someting else that means you're editing, not adding. This looks like a job for an enum.

public enum Operation
{
    Insert,
    Update,
    //Delete
}

And then you have

protected void SaveData(Operation operation)

And

if (operation == Operation.Insert)

And your decision is no longer stringly typed.


I would actually recommend a radically different approach. This ADO.NET stuff (SqlConnection, SqlCommand & friends) is from another decade, another era.

Make an Entity Framework DbContext with an entity type (the model I was referring to) that maps to that TableName table, with members that map to its columns. Entity Framework will generate the parameterized SQL for you, leaving you with code that essentially looks like:

using (var context = new MyDbContext(connectionString))
{
    var entity = context.TheRecords.SingleOrDefault(e => e.Id == fieppk);
    if (entity == null)
    {
        // actually, extract this scope to its own method
        var newEntity = new TheRecord
        {
            MeetDate = model.MeetDate,
            IEPStart = model.IEPStart,
            IEPEnd = model.IEPEnd,
            //...
        };
        context.TheRecords.Add(newEntity);
    }
    else
    {
        entity.MeetDate = model.MeetDate;
        entity.IEPStart = model.IEPStart;
        entity.IEPEnd = model.IEPEnd;
        //...extract this scope to a separate method, too
    }

    context.SaveChanges();
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I really appreciate the response. What's most interesting to me is I didn't realize what we were doing was so out of date. I suspected it but wasn't 100%. Either way, for now, I'm going to play around with some of the ideas you've tossed out there. I will make a class for my UI identifiers. And then try working with enum's instead of string. Thank you! So much to learn. A bit overwhelming but I'm ready to try.. \$\endgroup\$ – pacpie Nov 2 '15 at 15:33
  • \$\begingroup\$ One quick clarification (I may have a few)... Re: public class SomeMeaningfulIdentifierForThatModel This is a separate from my code behind file, right? Like, it should exist in my App_code folder (where we store some of our classes in Visual Studio). Should this be one continuous class because we have a lot of other pages similar in format, so would I make all of those UI controls apart of this class? \$\endgroup\$ – pacpie Nov 2 '15 at 18:18
3
\$\begingroup\$

First problem I'd solve is code duplication. Your huge function (BTW it's really too long to fit a single method) has many almost equal lines of code. To build a SQL command you may use little Reflection and anonymous classes:

static void CreateAndExecuteInsertCommand(string tableName, object parameters)
{
    var parametersType = parameters.GetType();
    var properties = parametersType.GetProperties();

    var sql = String.Format("INSERT INTO [{0}] ({1}) VALUES ({2})",
        tableName,
        String.Join(",", properties.Select(x => x.Name)),
        String.Join(",", properties.Select(x => String.Format("@{0}", x.Name))));

    var connectionString = ConfigurationManager.ConnectionStrings["name"].ConnectionString;
    using (var connection = new SqlConnection(connectionString))
    {
        connection.Open();

        using (var command = connection.CreateCommand())
        {
            command.CommandText = sql;
            foreach (var property in properties)
            {
                command.Parameters.AddWithValue(String.Format("@{0}", property.Name),
                    property.GetValue(parameters));
            }

            command.ExecuteNonQuery();
        }
    }
}

You have now a basic tool you may reuse. This function is far from perfect but we will try to improve it later. Note that this approach (object's properties mapped into SQL parameters) is what's used, for example, in StackExchange's Dapper mapper.

You also perform many unwanted and dangerous conversions. For example:

String ltxtiepend = misc_class.format_mmddyyyy(txtiepend.Text);
if (ltxtiepend == "")
    ltxtiepend = null;
...
if (ltxtMeetDate != null)
{
    cmd.Parameters["@meet_date"].Value = ltxtMeetDate;
}
else
{
    cmd.Parameters["@meet_date"].Value = DBNull.Value;
}

All these code is unnecessary because @meet_date is declared as SqlDbType.Date then misc_class.format_mmddyyyy should already return DateTime?. If it doesn't then make an helper function ParseDateTime() to do it and write it only once. Again: do not repeat yourself.

Let's see how code should look like:

CreateAndExecuteInsertCommand("tablename", new
{
   meet_date = ParseDateTime(txtMeetDate.Text),
   aide_1to1 = chk1Aide1.Checked,
   entrydate = DateTime.Today
});

We still have to handle that unfriendly code to read radio buttons, let's make an helper function also for that:

bool? RadiosToBoolean(RadioButton buttonForTrue, RadioButton buttonForFalse)
{
    if (buttonForTrue.Checked)
        return true;

    if (buttonForFalse.Checked)
        return false;

    return null;
}

Now you can use it:

CreateAndExecuteInsertCommand("tablename", new
{
   meet_date = ParseDateTime(txtMeetDate.Text),
   aide_1to1 = chk1Aide1.Checked,
   entrydate = DateTime.Today,
   fam_consent = RadiosToBoolean(RadioButton9, RadioButton10),
});

And so on, the other lines are pretty similar. Now it's time to take a look back to existing code. Pick a common naming convention and follow it. RadioButton9 is a terrible name for something because you have no idea what it is for, even little copy & paste coding can introduce subtle bugs. Rename all your controls to meaningful names and possibly do not use abbreviations: disk space is cheap, your time is not then make it clear (I don't know what fam_consent is for so I put a dummy name here):

fam_consent = RadiosToBoolean(rdAllowFamYes, rdAllowFamNo),

Also misc_class.format_mmddyyyy isn't a good name and it doesn't follow common naming conventions. Moreover a misc class is almost always an indicator that your design failed somewhere because it does too many unrelated things. Split things to multiple classes and add extensions methods. When you will have better grouped helper methods then you will probably see objects instead of helpers and a better design will start to come out. Do it step by step refactoring where appropriate.

Now pick your method signature:

protected void SaveData(String addedit)

First of all SaveData() does two things when each function should perform only one task. Split your code into two functions and you also will get rid of that ugly string used instead of an enum.

protected void InsertNewRecord() { }

and

protected void UpdateRecord() { }

When you're about to write UpdateRecord() you will note we may/should reuse CreateAndExecuteInsertCommand() because it'll perform almost same task. It's time to refactor our function!

string CreateInsertCommand(string tableName, object parameters)
{
    var properties = parameters.GetType().GetProperties();

    return String.Format("INSERT INTO [{0}] ({1}) VALUES ({2})",
        tableName,
        String.Join(",", properties.Select(x => String.Format("[{0}]", x.Name))),
        String.Join(",", properties.Select(x => String.Format("@{0}", x.Name))));
}

string CreateUpdateCommand(string tableName, object parameters, object condition)
{
    var parametersProperties = parameters.GetType().GetProperties();
    var conditionProperties = condition.GetType().GetProperties();

    return String.Format("UPDATE [{0}] {1} WHERE {2}",
        tableName,
        String.Join(",", parametersProperties.Select(x => String.Format("[{0}]=@{0}", x.Name))),
        String.Join(" AND ", conditionProperties.Select(x => String.Format("{0}={1}", x.Name, x.GetValue(condition)))));
}

Code to execute a SQL command is pretty generic and can be abstracted away:

void ExecuteCommand(string sql, object parameters)
{
    var connectionString = "...";
    using (var connection = new SqlConnection(connectionString))
    {
        connection.Open();

        using (var command = connection.CreateCommand())
        {
            command.CommandText = sql;
            foreach (var property in parameters.GetType().GetProperties())
            {
                command.Parameters.AddWithValue(String.Format("@{0}", property.Name),
                    property.GetValue(parameters));
            }

            command.ExecuteNonQuery();
        }
    }
}

Your hypothetical InsertNewRecord() function will then be:

protected void InsertNewRecord()
{
    var record = new
    {
       meet_date = ParseDateTime(txtMeetDate.Text),
       aide_1to1 = chk1Aide1.Checked,
       entrydate = DateTime.Today,
       fam_consent = RadiosToBoolean(rdAllowFamYes, rdAllowFamNo),
       // And so on
    });

    ExecuteCommand(CreateInsertCommand("tableName", record), record);
}

Of course much more should be done (for example error handling, see also Know when to retry or fail when calling SQL Server from C#?) and CreateUpdateCommand() is pretty naive - especially about condition because it does not use parameters (bad!!!) and it doesn't handle strings - but this an exercise for the reader...


Edit: When all of this is done you may consider a bigger and more drastic change: abandon direct SQL manipulation and rely on an ORM as Mat suggested in his answer. That's a big design change but probably (unless you're dealing with a very complex scenario) it's best approach on long term. Of course you'll need to change almost all code in your application then it may not be viable as refactoring step.

For this you may consider a step by step approach first introducing strongly typed classes for your model and using them with CreateInsertCommand()/CreateUpdateCommand() methods. When all your code will rely on strongly typed classes then you may be ready to move to an ORM.


About your other question: I'd ask that on Stack Overflow, not on Code Review but I'd (blindly) guess value is saved properly but you see a time part when you show that value somewhere. It's a presentation issue, not a storage issue.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Why call ToString() on sql if sql is already a string? \$\endgroup\$ – Mathieu Guindon Oct 28 '15 at 0:59
  • \$\begingroup\$ Mat, absolutely no reason. I first declared as StringBuilder. \$\endgroup\$ – Adriano Repetti Oct 28 '15 at 6:48
  • \$\begingroup\$ Wow. Thank you so much. I need to study this some more and start to implement this. SO much of our code is what I pasted and I want to change it for the better and this is a good first step. I definitely would also consider the "ORM" step as well but I need to do my homework on that. I may have some additional clarifications very soon. Thank you so much.. \$\endgroup\$ – pacpie Oct 29 '15 at 0:48
  • \$\begingroup\$ I'm having an issue in: void ExecuteCommand(string sql, object parameters), the line " property.GetValue(parameters));" in the foreach loop keeps producing a " CS1501: No overload for method 'GetValue' takes 1 arguments" error... \$\endgroup\$ – pacpie Nov 2 '15 at 18:39
  • \$\begingroup\$ It's new on framework 4.5. You have to use older overload with an extra null parameter for indexed properties! \$\endgroup\$ – Adriano Repetti Nov 2 '15 at 18:58

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.