6
\$\begingroup\$

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


}
}
\$\endgroup\$
1
  • \$\begingroup\$ No, please don't advise to add a switch statement. It's clear that you should replace these conditions with polymorphism (sub classes). You create a Interface or Abstract base class that you use in your class, then each sub class defines its own behavior. This will make your code easier to read and maintain. \$\endgroup\$ – Jocke Dec 19 '13 at 8:10
6
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for taking your time with this answer, it brings up some good points I hadn't considered. I'm already using paremeterised queries, but I did add error handling which had passed me by in some cases, changed the SwitchView method to a switch and removed the magic strings. \$\endgroup\$ – spacesam Dec 19 '13 at 18:56
5
\$\begingroup\$

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;
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ +1, also not returning a null value indicating 'no values', better to return an empty list. Might need to initialise ViewState["xyz"] == null elsewhere though, or check and return the empty list in the getter. \$\endgroup\$ – Kyle Dec 19 '13 at 1:33
  • 1
    \$\begingroup\$ This subtly changes the behaviour. Before if the wrong type was placed in a field you would get a cast exception, in this case it will silently return null, potentially hiding a bug. Personally I'd just drop the null check and do a straight cast, e.g. return (List<string>)ViewState["CRIDS"];. I don't believe you need to set to null first either, but then I haven't used ViewState in years - the MSDN docs should be able to tell you. \$\endgroup\$ – MrKWatkins Dec 19 '13 at 10:16
3
\$\begingroup\$

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;
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ This was already mentioned in Kyle's answer. \$\endgroup\$ – Mathieu Guindon Dec 19 '13 at 3:49
  • 2
    \$\begingroup\$ It was, but i expanded on it by resetting the panel visibility up the top so it's much clearer to see what's happening in each case. Simply changing it from if to switch seems pointless, ie a change for the sake of change. \$\endgroup\$ – mnsr Dec 19 '13 at 3:56
  • 1
    \$\begingroup\$ I think the change from if/else to switch is made useful even if just for the readability, and implicit understanding that 'we're dealing with cases that match sender and nothing else' as soon as you see the switch keyword, rather than needing to look at each x == y conditional. I agree with your simplification by removing the common components to improve readability, definitely good, though this could be done in exactly the same fashion with an if/elseif/etc block if you're that way inclined. \$\endgroup\$ – Kyle Dec 19 '13 at 4:10
  • 2
    \$\begingroup\$ Don't get me wrong, I agree with you. Yes you could simply change it from if to switch, or keep the if and do what I did and both are fine, but if you combine them, you end up in a much better place. :) \$\endgroup\$ – mnsr Dec 19 '13 at 4:44

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.