1
\$\begingroup\$

I've not posted binding specific messages to the gridview part. This shows updating that particular message or message from a list of messages databound with an XML file. My main concern is omitting redundant code in the else part. Any suggestions?

protected void grdMessage_RowUpdating(object sender, GridViewUpdateEventArgs e)
{
    DataRow xRow;
    DataSet mdsRedirect = new DataSet();
    mdsRedirect.ReadXml(st);

    if (Request["MessageID"] != null && Convert.ToInt16(Request["MessageID"]) != 0)
    {   
        //Find specific item from XML and set updated value
        mdsRedirect.Tables[0].DefaultView.RowFilter = "MessageID=" + Convert.ToInt16(Request["MessageID"]);
        if (mdsRedirect.Tables[0].DefaultView.Count > 0)
        {
            mdsRedirect.Tables[0].DefaultView[0]["MessageText"] = ((TextBox)(grdMessage.Rows[e.RowIndex].FindControl("txtupdMessage"))).Text;
            mdsRedirect.Tables[0].DefaultView.RowFilter = string.Empty;
        }
        else {
            xRow = mdsRedirect.Tables[0].Rows[e.RowIndex];
            xRow["MessageText"] = ((TextBox)(grdMessage.Rows[e.RowIndex].FindControl("txtupdMessage"))).Text;
        }
    }
    else
    {
        xRow = mdsRedirect.Tables[0].Rows[e.RowIndex];
        xRow["MessageText"] = ((TextBox)(grdMessage.Rows[e.RowIndex].FindControl("txtupdMessage"))).Text;
    }

    grdMessage.EditIndex = -1;
    mdsRedirect.WriteXml(st);
    mdsRedirect.Dispose();
    getxml();//read XML file and bind it
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Telling us what the code does is a code start. \$\endgroup\$ – Bobby Nov 29 '11 at 13:45
  • \$\begingroup\$ I've not posted binding specific message to gridview part. This shows updating that particular message or message from list of messages databound with XML file. My main concern was to omit redundant code in else part. Any suggestion? \$\endgroup\$ – user686021 Nov 29 '11 at 16:48
  • \$\begingroup\$ You can simplify DataSet mdsRedirect = new DataSet(); through mdsRedirect.Dispose(); by wrapping the creating within a using block. \$\endgroup\$ – Jesse C. Slicer Aug 7 '14 at 18:15
2
\$\begingroup\$

Not sure if it's "optimized", but here's something of a simplification:

    protected void grdMessage_RowUpdating(object sender, GridViewUpdateEventArgs e)
    {
        string messageIdString = Request["MessageID"];
        int messageId = messageIdString == null ? 0 : Convert.ToInt16(messageIdString);

        using (DataSet mdsRedirect = new DataSet())
        {
            DataTable table = mdsRedirect.Tables[0];
            string updateMessageText = ((TextBox)grdMessage.Rows[e.RowIndex].FindControl("txtupdMessage")).Text;

            mdsRedirect.ReadXml(st);
            table.DefaultView.RowFilter = messageId == 0 ? table.DefaultView.RowFilter : "MessageID=" + messageId;
            if ((messageId != 0) && (table.DefaultView.Count > 0))
            {
                // Find specific item from XML and set updated value
                table.DefaultView[0]["MessageText"] = updateMessageText;
                table.DefaultView.RowFilter = string.Empty;
            }
            else
            {
                table.Rows[e.RowIndex]["MessageText"] = updateMessageText;
            }

            grdMessage.EditIndex = -1;
            mdsRedirect.WriteXml(st);
        }

        getxml(); // read XML file and bind it
    }
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This is much better and thanks for reminding me about Using, but is there any way we can omit UpdateRowMessageText to be appear twice and make it still work? \$\endgroup\$ – user686021 Nov 29 '11 at 16:50
  • \$\begingroup\$ It possibly can with some convoluted looking if statements. Let me try that out and see what you think. \$\endgroup\$ – Jesse C. Slicer Nov 29 '11 at 16:53
  • \$\begingroup\$ See how that works for you. \$\endgroup\$ – Jesse C. Slicer Nov 29 '11 at 17:04
2
\$\begingroup\$

I'm not a C# guru, so just some general advice:

This line is really-really long, hard to read and there are too many dots in it:

mdsRedirect.Tables[0].DefaultView[0]["MessageText"] = ((TextBox)(grdMessage.Rows[e.RowIndex].FindControl("txtupdMessage"))).Text;

The code use mdsRedirect.Tables[0] a lot of times, create a local variable for that at the beginning of the method:

MyTable table = mdsRedirect.Tables[0];
...
table.DefaultView[0]["MessageText"] = ((TextBox)(grdMessage.Rows[e.RowIndex].FindControl("txtupdMessage"))).Text;

I would also extract the following method:

private String getTextFromRow(int rowIndex) {
    Row row = grdMessage.Rows[rowIndex];
    Control control = row.FindControl("txtupdMessage");
    TextBox textBox = (TextBox) control;
    return textBox.Text;
}

Then the code:

MyTable table = mdsRedirect.Tables[0];
...
table.DefaultView[0]["MessageText"] = getTextFromRow(e.RowIndex);
| improve this answer | |
\$\endgroup\$

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.