Advice to improve code in simple webpage

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 CRIDS {
get
{
if (ViewState["CRIDS"] != null )
{
return (List)ViewState["CRIDS"];
}

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

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

return null;
}
set
{

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 CRIDS
{
    get
    {
        if (ViewState[VS_KEY_CRIDS] != null)
        {
            return (List)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(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 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 o

StackExchange Code Review Q#37688, answer score: 6