Click here to Skip to main content
15,867,453 members
Please Sign up or sign in to vote.
0.00/5 (No votes)
Good day to all, I'm just a practicing programmer. i was hoping if some of you could teach me how could i compress or simplify my codes.

C#
private void txtTime1_TextChanged(object sender, EventArgs e)
        {
            double fixAmount = double.Parse(txtFixAmount.Text);
            fixAmount = 15000 / 26 / 8;
            if (cbType1.SelectedItem.ToString() == "Regular")
            {
                double x = fixAmount * 1.25;
                if (txtTime1.Text.Length > 0)
                {
                    txtAmount1.Text = (x * double.Parse(txtTime1.Text)).ToString();
                }
                else if (txtTime1.Text.Length <= 0 || txtTime1.Text == null)
                {
                    txtAmount1.Text = "0.00";
                }
            }

            else if (cbType1.SelectedItem.ToString() == "Double")
            {
                double x = fixAmount * 2.5;
                if (txtTime1.Text.Length > 0)
                {
                    txtAmount1.Text = (x * double.Parse(txtTime1.Text)).ToString();
                }
                else if (txtTime1.Text.Length <= 0 || txtTime1.Text == null)
                {
                    txtAmount1.Text = "0.00";
                }
            }
        }

        private void txtTime2_TextChanged(object sender, EventArgs e)
        {
            double fixAmount = double.Parse(txtFixAmount.Text);
            fixAmount = 15000 / 26 / 8;
            if (cbType2.SelectedItem.ToString() == "Regular")
            {
                double x = fixAmount * 1.25;
                if (txtTime2.Text.Length > 0)
                {
                    txtAmount2.Text = (x * double.Parse(txtTime2.Text)).ToString();
                }
                else if (txtTime2.Text.Length <= 0 || txtTime2.Text == null)
                {
                    txtAmount2.Text = "0.00";
                }
            }

            else if (cbType2.SelectedItem.ToString() == "Double")
            {
                double x = fixAmount * 2.5;
                if (txtTime2.Text.Length > 0)
                {
                    txtAmount2.Text = (x * double.Parse(txtTime2.Text)).ToString();
                }
                else if (txtTime2.Text.Length <= 0 || txtTime2.Text == null)
                {
                    txtAmount2.Text = "0.00";
                }
            }
        }



i couldn't upload an image to give you a much broader idea.

1. I have 4 items here
• combobox = (Name - cbType1 & 2)
• Textbox = (Name - txtTime1 & 2)
• Textbox = (Name - txtAmount1 & 2)
• Textbox = (Name - FixAmount)

2. later i'll have a button underneath that will get the sum of txtAmount1 & txtAmount2


Thank you all.
Posted
Comments
Afzaal Ahmad Zeeshan 30-Mar-15 21:40pm    
Which of these do you want to merge?

You probably already have noticed that there is a lot of "similar" code there (and asked your question for that reason). And in fact you can simplify it a lot.

Before we start with that, let's correct one flaw in your code:
You parse the text entered into txtFixAmount into fixAmount and on the following line you assign the result of 15000 / 26 / 8 to fixAmount. So the text from txtFixAmount has no effect at all. Maybe your intention was to multiply or divide the entered value by the result of 15000 / 26 / 8 ? I'll go with the assumption that you wanted to multiply it.

First code-simplification: The only thing that makes the code of the "if"-branch different from the "else if"-branch is your multiplier for fixAmount (1.25 resp. 2.5). If you introduce a new variable (I will call it multiplier here) you can reduce the code that depends on the "if else-if" to assigning either 1.25 or 2.5 to multiplier. And everything else just follows and uses multiplier instead of a hard-coded value.
Also: If you need a converted value (in this case: cbType1.SelectedItem.ToString()) multiple times, assign it to a variable (selectedItem here) and use that variable in the following code instead:

C#
private void txtTime1_TextChanged(object sender, EventArgs e)
{
    double fixAmount = double.Parse(txtFixAmount.Text);
    fixAmount *= 15000 / 26 / 8;

    string selectedItem = cbType1.SelectedItem.ToString();
    double multiplier = 0;

    if (selectedItem == "Regular")
    {
        multiplier = 1.25;
    }
    else if (selectedItem == "Double")
    {
        multiplier = 2.5;
    }

    double x = fixAmount * multiplier;
    if (txtTime1.Text.Length > 0)
    {
        txtAmount1.Text = (x * double.Parse(txtTime1.Text)).ToString();
    }
    else if (txtTime1.Text.Length <= 0 || txtTime1.Text == null)
    {
        txtAmount1.Text = "0.00";
    }
}


Next up: Good practices.

1) double.Parse will throw an Exception if the entered text can't be parsed into a double. If you use double.TryParse instead, you will get a boolean result indicating whether the parsing succeeded or not which allows you to handle that case gracefully. We can apply this to both lines where you currently use double.Parse.

2) Whenever you use an "else-if" you should also have a concluding "else", because, in this case, you might change your program so that there are more options than "Regular" and "Double" but might forget to include an extra "else-if" that deals with that case at first. If you have a concluding "else" that throws an Exception you will become aware of your mistake very quickly and won't have to search through your code to find the bug.

C#
private void txtTime1_TextChanged(object sender, EventArgs e)
{
    double fixAmount;
    if (Double.TryParse(txtFixAmount.Text, out fixAmount))
    {
        fixAmount *= 15000 / 26 / 8;

        string selectedItem = cbType1.SelectedItem.ToString();
        double multiplier;

        if (selectedItem == "Regular")
        {
            multiplier = 1.25;
        }
        else if (selectedItem == "Double")
        {
            multiplier = 2.5;
        }
        else
        {
            throw new NotImplementedException(String.Format("Unrecognized selection: {0}", selectedItem));
        }

        double x = fixAmount * multiplier;
        double time;
        if (Double.TryParse(txtTime1.Text, out time))
        {
            txtAmount1.Text = (x * time).ToString();
        }
        else
        {
            txtAmount1.Text = "0.00";
        }
    }
    else
    {
        txtFixAmount.Text = "0.00";
    }
}


Next code-simplification: Looking at your original code, you will recognize that txtTime1_TextChanged and txtTime2_TextChanged essentially do the exact same thing, just for different ComboBoxes and TextBoxes. You can eliminate this de-facto code-duplication as well. For that you have to replace the hard-coded use of specific ComboBox- and TextBox-objects by variables and have your TextChanged-EventHandler-Methods call that new method with the appropriate ComboBox- and TextBox-objects:

C#
private void txtTime1_TextChanged(object sender, EventArgs e)
{
    ProcessTextChanged(cbType1, txtTime1, txtAmount1);
}

private void txtTime2_TextChanged(object sender, EventArgs e)
{
    ProcessTextChanged(cbType2, txtTime2, txtAmount2);
}

private void ProcessTextChanged(ComboBox cbType, TextBox txtTime, TextBox txtAmount)
{
    double fixAmount;
    if (Double.TryParse(txtFixAmount.Text, out fixAmount))
    {
        fixAmount *= 15000 / 26 / 8;

        string selectedItem = cbType.SelectedItem.ToString();
        double multiplier;

        if (selectedItem == "Regular")
        {
            multiplier = 1.25;
        }
        else if (selectedItem == "Double")
        {
            multiplier = 2.5;
        }
        else
        {
            throw new NotImplementedException(String.Format("Unrecognized selection: {0}", selectedItem));
        }

        double x = fixAmount * multiplier;
        double time;
        if (Double.TryParse(txtTime.Text, out time))
        {
            txtAmount.Text = (x * time).ToString();
        }
        else
        {
            txtAmount.Text = "0.00";
        }
    }
    else
    {
        txtFixAmount.Text = "0.00";
    }
}


Final words:
- Theoretically cbType.SelectedItem could be null. It would be good practise to check that before accessing it with .ToString().
- In case some input can't be parsed you shouldn't just silently reset the text to "0.00" but also show some notification to the user, e.g. a MessageBox.
- The strings "Regular" and "Double" shouldn't be hard-coded here - it would be better to have one "central" point in your program where you initialize variables with it and use these to fill your ComboBoxes as well as use them for the comparisons here.

Hope it helps :-)
 
Share this answer
 
Comments
PracticeMakesPerfect 31-Mar-15 2:06am    
Thanks for the tips sir.. will take note everything sir...
Sascha Lefèvre 31-Mar-15 2:11am    
You're welcome!
Maciej Los 31-Mar-15 2:37am    
Nice explanation ;)
Sascha Lefèvre 31-Mar-15 2:45am    
Thank you :-)
You can use the sender parameter to know who is calling a method.
For example:
C#
private void txtTime_Generic_TextChanged(object sender, EventArgs e)
{
    TextBox tb = (sender as TextBox);
    if (tb != null)  // Just make sure it is a TextBox calling the method
    {
        // Common code
        // ...
        
        if (tb.Name == "txtTime1")
        {
            // Do This
        }
        else if (tb.Name = "txtTime2")
        {
            // Do that
        }
        else
        {
            // Maybe do nothing
        }
    }
}


Then assign this method to the TextChanged events of both txtTime1 and txtTime2.
 
Share this answer
 
v2

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900