Advice to improve code in simple webpage

Question

I'm trying to make a few webpages that will allow us in the tech support team at the factory I work at to provide support faster by adding a simple interface to actions we perform everyday directly on the database.

The following is for a simple page where we will enter a list of serial numbers and select from a few choices of actions to perform on them (actions are mostly executing a stored procedure or query in order to change something in the database). It consists of a texbox where we will enter a list of serials, a dropdown list and a checkbox. These basically move the serials to a diferent location and remove its flag as a finished product in the database. It also has two gridviews to show the serial data before and after the change. All this is inside panels to make each visible/invisible easily depending on where we are in the execution. The actuall calls to the database are in a different class which i'm not including here but are very straightforward parametrized queries and stored procedures.

I'm by no means an expert programmer but very interested in moving my career in that direction. I would like to know how a more experienced professional would have done this.

This is the code behind the webpage. Any comments are welcome, thank you very much.

namespace UnpackTool
{
public partial class _Default : System.Web.UI.Page
{
    protected void Page_Load(object sender, EventArgs e)
    {

    }

    #region ViewStates

    private List<string> CRIDS {
        get
        {
            if (ViewState["CRIDS"] != null )
            { 
                return (List<string>)ViewState["CRIDS"];
            }

            return null;
        }
        set
        {
            ViewState["CRIDS"] = value;
        }
    }

    private List<string> SerialNumbers
    {
        get
        {
            if (ViewState["SerialNumbers"] != null)
            {
                return (List<string>)ViewState["SerialNumbers"];
            }

            return null;
        }
        set
        {
            ViewState["SerialNumbers"] = value;
        }
    }

    private DataTable SerialData
    {
        get
        {
            if (ViewState["SerialData"] != null)
            {
                return (DataTable)ViewState["SerialData"];
            }

            return null;
        }
        set
        {
            ViewState["SerialData"] = value;
        }

    }


    #endregion

    #region Helpers

    //Clear viewstate values and grids to start over
    private void ClearSerials()
    {
        CRIDS = null;
        SerialNumbers = null;
        SerialData = null;
        CRIDS = null;
        SearchGrid.DataSource = null;
        ResultsGrid.DataSource = null;
        SearchGrid.DataBind();
        ResultsGrid.DataBind();

    }

    // Switch view between panels according to current state
    public void SwitchView(string sender)
    {
        if (sender == "Search")
        {
            Panel1.Visible = false;
            Panel2.Visible = true;
            Panel3.Visible = false;
            SerialBox.Enabled = false;
        }
        if (sender == "Cancel")
        {
            Panel1.Visible = true;
            Panel2.Visible = false;
            Panel3.Visible = false;
            SerialBox.Enabled = true;
            UnpackCheck.Checked = false;
            ProcessList.SelectedValue = "NO CHANGE";
            HideErrorMessage();


        }
        if (sender == "Accept")
        {
            Panel1.Visible = false;
            Panel2.Visible = false;
            Panel3.Visible = true;
            SerialBox.Enabled = false;


        }
        if (sender == "Return")
        {
            Panel1.Visible = true;
            Panel2.Visible = false;
            Panel3.Visible = false;
            SerialBox.Enabled = true;
            SerialBox.Text = "";
            UnpackCheck.Checked = false;
            ProcessList.SelectedValue = "NO CHANGE";
            HideErrorMessage();

        }


    }

    private void ShowErrorMessage(string Error)
    {
        ErrorLabel.Text = Error;
        ErrorLabel.ForeColor = System.Drawing.Color.Red;
        ErrorLabel.Visible = true;
    }

    private void HideErrorMessage()
    {
        ErrorLabel.Text = "";
        ErrorLabel.ForeColor = System.Drawing.Color.Black;
        ErrorLabel.Visible = false;
    } 

    #endregion

    #region Events

    //Read from textbbox and get serial info from db
    protected void SearchBtn_Click(object sender, EventArgs e)
    {
        GetSerials();
        SwitchView("Search");

        //Only get serial info from db if actual data is entered in textbox
        if (SerialNumbers.Count > 0)
        {
            GetSerialData();

            GetModelProcess();


        }

    }

    //Check if serials should be moved and/or unpacked, and perform actions
    protected void AcceptBtn_Click(object sender, EventArgs e)
    {
        //only do something if valid data is entered
        if (SerialNumbers.Count > 0)
        {
            try
            {
                //If Unpack is checked, serials are unpacked
                if (UnpackCheck.Checked)
                {
                    UnpackSerials();
                }

                //When value is NO CHANGE, serials arent moved
                if (ProcessList.SelectedValue.ToString() != "NO CHANGE")
                {
                    MoveSerials();
                }

                //Get serial info again to see results
                GetSerialData();

                SwitchView("Accept");

                HideErrorMessage();
            }

            catch (Exception ex)
            {
                ShowErrorMessage(ex.Message);
            }
        }
    }

    //Reset view
    protected void CancelBtn_Click(object sender, EventArgs e)
    {
        SwitchView("Cancel");
        ClearSerials();

    }

    //Reset view
    protected void ReturnBtn_Click(object sender, EventArgs e)
    {
        SwitchView("Return");
        ClearSerials();
    }

    #endregion

    #region DataAccess

    //Get serial info from db
    private void GetSerials()
    {
        //If CrateID is selected search by CRID
        if (InputType.SelectedValue.ToString() == "CrateID")
        {
            //Read from textbox into CRID viewstate list
            CRIDS = new List<string>(SerialBox.Text.Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries));

            //Method received list of CRID and returns list of serials
            SerialNumbers = Dal.ExecuteCRIDSerialQuery(CRIDS).AsEnumerable().Select(x => x[0].ToString()).ToList();

        }
        else
        {
            //Read from textbox into SerialNumber viewstate list
            SerialNumbers = new List<string>(SerialBox.Text.Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries));
        }

    }



    private void UnpackSerials()
    {
        //Unpack method receives list, returns nothing
        Dal.ExecuteUnpackQuery(SerialNumbers);

    }

    private void MoveSerials()
    {
        //Move serials, executes stored procedure, receives single serialnumber, iterate through list executing for each
        if (SerialData.Rows.Count > 0)
        {
            foreach (DataRow row in SerialData.Rows)
            {
                Dal.ExecuteMoveQuery(row["TSN"].ToString(), row["Model"].ToString(), ProcessList.SelectedValue.ToString());
            }
        }

    }

    private void GetSerialData()
    {
        DataTable serialdata = new DataTable();

        //Get serial data from db, method receives list of serials, returns data table
        SerialData = Dal.ExecuteResultsQuery(SerialNumbers);

        //Fill datagrids
        SearchGrid.DataSource = SerialData;
        ResultsGrid.DataSource = SerialData;
        SearchGrid.DataBind();
        ResultsGrid.DataBind();
    }

    //Gets processed used by the models listed in the serial data, can receive different models, binds to dropdownlist
    private void GetModelProcess()
    {
        if (SerialNumbers != null)
        {
            DataTable Processes = Dal.ExecuteProcessByModelQuery(SerialData.AsEnumerable().Select(x => x["Model"].ToString()).ToList());
            ProcessList.DataSource = Processes;
            ProcessList.DataTextField = "Process_Name";
            ProcessList.DataValueField = "Process_Name";
            ProcessList.DataBind();
            //When value is NO CHANGE, serials arent moved
            ProcessList.SelectedValue = "NO CHANGE";

        }
    }

    #endregion


}
}

Answer 1

A few examples of opportunities to:

• Be more concise with the flow of control to improve readability and reduce the likelyhood of introducing bugs later on
• Breaking out/abstracting some operations to prevent duplication

Conditionals

The SwitchView() method will pass though each if statement even though it will match either none or one. In some cases avoiding unnecessary conditionals might be a performance consideration, but here it's just to be concise and improve readability:

You could use either a series of if() {} else if() {}'s:

if (condition1)
{
    //stuff
}
else if (condition2)
{
    //stuff
}
else if (condition3)
{
    //stuff
}
else
{
    //nothing matched
}

Or in this case as you're checking only for string equality a switch as follows (generally worth thinking about case sensitivity here as well):

public void SwitchView(string sender)
{
    switch (sender)
    {
        case "Search":
            Panel1.Visible = false;
            Panel2.Visible = true;
            Panel3.Visible = false;
            SerialBox.Enabled = false;
            break;
        case "Cancel":
            Panel1.Visible = true;
            Panel2.Visible = false;
            Panel3.Visible = false;
            SerialBox.Enabled = true;
            UnpackCheck.Checked = false;
            ProcessList.SelectedValue = "NO CHANGE";
            HideErrorMessage();
            break;
        case "Accept":
            Panel1.Visible = false;
            Panel2.Visible = false;
            Panel3.Visible = true;
            SerialBox.Enabled = false;
            break;
        case "Return":
            Panel1.Visible = true;
            Panel2.Visible = false;
            Panel3.Visible = false;
            SerialBox.Enabled = true;
            SerialBox.Text = "";
            UnpackCheck.Checked = false;
            ProcessList.SelectedValue = "NO CHANGE";
            HideErrorMessage();
            break;
        //optionally you could handle a situation of an unexpected sender as follows
        //default:
        //  throw new Exception("Unexpected sender '" + sender + "' in SwitchView()");
    }
}

To remove some of those strings you might also define an enum like this giving you a strongly typed method of indicating what kind of action is triggering the view change:

public enum ActionViewStyle { Search, Cancel, Accept, Return };

public void SwitchView(ActionViewStyle sender)
{
    switch (sender)
    {
        case ActionViewStyle.Search:
            Panel1.Visible = false;
            //etc...
    }
}

Magic strings/numbers

It's often considered good practice to avoid the use of magic strings and numbers where practical. For example, the following is easier to read, you can find all references, and prevents bugs introduced by a typo in some instance of the string:

private const string VS_KEY_CRIDS = "CRIDS";
private const string VS_KEY_SERIAL_NUMBERS = "SerialNumbers";
private const string VS_KEY_SERIAL_DATA = "SerialData";
private const string MSG_PROCS_NO_CHANGE = "NO CHANGE";
private const string COL_NAME_TSN = "TSN";
private const string COL_NAME_MODEL = "Model";
//etc...

private List<string> CRIDS
{
    get
    {
        if (ViewState[VS_KEY_CRIDS] != null)
        {
            return (List<string>)ViewState[VS_KEY_CRIDS];
        }

        return null;
    }
    set
    {
        ViewState[VS_KEY_CRIDS] = value;
    }
}

You might also consider the approach for other strings reused throughout the class.

There are a few approaches to storing strings/numbers, probably try to make a decision balancing the benefits abstracting the configuration, vs the complexity it introduces to what might otherwise be a trivial operation. Some options are:

• Local constants (above). Easy, simple, not so flexible
• Static class which can be referenced elsewhere
• External/independent configuration (file, database, etc)

Splitting Strings

I notice you're splitting up the user input like this in a few places:

new List<string>(SerialBox.Text.Split(new string[] { "\r\n" }, StringSplitOptions.RemoveEmptyEntries));

Nothing wrong with this, but you might consider breaking out the delimiter definition to a single location you can refer to, and the operation of getting a list of values from a delimited string into it's own method if it's something you'll be doing a lot:

//static string[] valueDelimiters = new string[] { Environment.NewLine };
static string[] valueDelimiters = new string[] { "\r\n", "\n" }; //if you want to handle LF and CRLF

private IEnumerable<string> GetValuesAsList(string values, string [] delimiters)
{
    return values.Split(valueDelimiters, StringSplitOptions.RemoveEmptyEntries).ToList();
}

private void GetSerials()
{
    //If CrateID is selected search by CRID
    if (InputType.SelectedValue.ToString() == "CrateID")
    {
        //Read from textbox into CRID viewstate list
        CRIDS = GetValuesAsList(SerialBox.Text, valueDelimiters);

        //Method received list of CRID and returns list of serials
        SerialNumbers = Dal.ExecuteCRIDSerialQuery(CRIDS).AsEnumerable().Select(x => x[0].ToString()).ToList();
    }
    else
    {
        //Read from textbox into SerialNumber viewstate list
        SerialNumbers = GetValuesAsList(SerialBox.Text, valueDelimiters);
    }
}

Input Sanitising

As you're taking user input from SerialBox and passing it into a data access layer which may be executing some sql query, it's worth checking to make sure that you're guarding against injection with something like parameterised queries

Error Handling

Looking at the calls into the data access layer, I'm just wondering if they might ever throw an exception (data repository unreachable, malformed query, bad data, etc). If so it would be a worthwhile investment in handling them and logging/display an appropriate message.

Answer 2

Simplify your properties with the as operator:

private List<string> CRIDS
{
    get
    {
        return ViewState["CRIDS"] as List<string>;
    }

    set
    {
        ViewState["CRIDS"] = value;
    }
}

private List<string> SerialNumbers
{
    get
    {
        return ViewState["SerialNumbers"] as List<string>;
    }

    set
    {
        ViewState["SerialNumbers"] = value;
    }
}

private DataTable SerialData
{
    get
    {
        return ViewState["SerialData"] as DataTable;
    }

    set
    {
        ViewState["SerialData"] = value;
    }
}

Answer 3

For your SwitchView() function, i'd change the if statement to a switch, and simplify it for better readability.

public void SwitchView(string sender)
{
    Panel1.Visible = false;
    Panel2.Visible = false;
    Panel3.Visible = false;
    SerialBox.Enabled = false;
    UnpackCheck.Checked = false;

    switch (sender)
    {
        case "Search":
            Panel2.Visible = true;
            UnpackCheck.Checked = true;
            break;
        case "Cancel":
            Panel1.Visible = true;
            SerialBox.Enabled = true;
            ProcessList.SelectedValue = "NO CHANGE";
            HideErrorMessage();
            break;
        case "Accept":
            Panel3.Visible = true;
            break;
        case "Return":
            Panel1.Visible = true;
            SerialBox.Enabled = true;
            SerialBox.Text = "";
            ProcessList.SelectedValue = "NO CHANGE";
            HideErrorMessage();
            break;
    }
}