1

I'm still relatively new to C# and posting on a forum but i am trying to make a function that will help me shorten my code to make it more manageable/readable for someone who is new to coding in C# because it scared me when reading so many lines of code.

Ill show what I'm trying to do in code view and explain after just because i am not too good at explaining before hand.

https://gyazo.com/3a440ca6c4f0fc1764f7edd41081e434 - screenshot of the whole code so far.

this is the code in the button.

private void button7_Click(object sender, EventArgs e)
    { //this works and outputs the total value.

       /* int A, B, C, D, E, F, G, H, Answer;

        A = int.Parse(textBox1.Text);
        B = int.Parse(textBox2.Text);
        C = int.Parse(textBox3.Text);
        D = int.Parse(textBox4.Text);
        E = int.Parse(textBox5.Text);
        F = int.Parse(textBox6.Text);
        G = int.Parse(textBox7.Text);
        H = int.Parse(textBox8.Text);

        Answer = A + B + C + D + E + F + G + H;

        lblTotal1.Text = Answer.ToString();

        * */

        calc(textBox1.Text, textBox2.Text, textBox3.Text, textBox4.Text, textBox5.Text, textBox6.Text, textBox7.Text, textBox8.Text, lblTotal1.Text);

    }

this is the code in the function.

        private void calc(string TextBox1, string TextBox2, string TextBox3, string TextBox4, string TextBox5, string TextBox6, string TextBox7, string TextBox8, string TextAnswer)
    {
        int A, B, C, D, E, F, G, H, Total;
        A = int.Parse(TextBox1);
        B = int.Parse(TextBox2);
        C = int.Parse(TextBox3);
        D = int.Parse(TextBox4);
        E = int.Parse(TextBox5);
        F = int.Parse(TextBox6);
        G = int.Parse(TextBox7);
        H = int.Parse(TextBox8);

        Total = A + B + C + D + E + F + G + H;

        TextAnswer = Total.ToString();

    }

At first i thought it was due to using a string but i don't know whether or not this is the issue and if so how to fix it.

Would be a massive help if someone could tell me what i am doing wrong. Many thanks in advance.

Stefan
  • 211
  • 1
  • 2
  • 4

6 Answers6

4

What about this solution:

private string calc(params string[] input)
{
    return input.Select(x => int.Parse(x)).Sum().ToString();
}
Slava Utesinov
  • 13,410
  • 2
  • 19
  • 26
1

You can enumerate the controls (TextBox1..TextBox8):

private void button7_Click(object sender, EventArgs e) {
  int Total = 0;

  for (int i = 1; i <= 8; ++i)
    Total += int.Parse(Controls
      .Find(String.Format("textBox{0}", i), true)
      .First()
      .Text);

  lblTotal1.Text = Total.ToString();
} 

And so you have no need in function.

Dmitry Bychenko
  • 180,369
  • 20
  • 160
  • 215
1

Quite a few things that are improvable in your code.

  1. The implementation of calc is incorrect. You are trying to return the answer via the TextAnswer argument. Why? Make the method return the value itself, no need to use an out argument (more on this in #3).

    Only when a method must return more than one value should you consider using out which passes the argument by reference:

    private void calc(string TextBox1, string TextBox2, string TextBox3, string TextBox4, string TextBox5, string TextBox6, string TextBox7, string TextBox8, out string TextAnswer) //note the out keyword prior to TextAnswer

    You could also use ref instead of out but out conveys the meaning that the argument is used as an output plus it enables some compiler help to make sure the variable is set before exiting the method. To learn more about the subject, read this and this.

    The default in C# is that arguments are passed by value. That is, a copy of the argument is passed to the method. That means that even if you modify it, you are only modifying the copy, the original stays the same. In your case, you are simply modifying TextAnswer which is a copy of lblTotal1.Text.

    Again, let me make it clear, there is absolutely no need to use reference arguments in your case. As a sidenote, in your case you wouldn't be able to use out anyways as you can not pass by reference properties.

  2. Your helper method calc is horribly named. Don't use abbreviations that obfuscate your code. Name it correctly: calculateSum for example.

  3. Your helper method has a strange contract. Its name conveys that it simply calculates a value but it doesn't return anything and it apparently creates a side effect; showing the calculation result in the user interface. A better contract would be the following:

    private int calculateSum(string text1, string text2, string text3, string text4, string text5, string text6, string text7, string text8)

    Now the contract is clear: calculateSum is a function that takes 8 string arguments and somehow produces and returns a result.

    The implementation of the method would be:

    private int calculateSum(string text1, string text2, string text3, string text4, string text5, string text6, string text7, string text8)
    {
        int A, B, C, D, E, F, G, H;
        A = int.Parse(TextBox1);
        B = int.Parse(TextBox2);
        C = int.Parse(TextBox3);
        D = int.Parse(TextBox4);
        E = int.Parse(TextBox5);
        F = int.Parse(TextBox6);
        G = int.Parse(TextBox7);
        H = int.Parse(TextBox8);
    
        return A + B + C + D + E + F + G + H;
    }
    

    And now you'd call it as follows;

    private void button7_Click(object sender, EventArgs e)
    {
         lblTotal1.Text = calculateSum(textBox1.Text, textBox2.Text, ...);
    }
    
  4. Your code does not handle incorrect inputs. What happens if one of the arguments passed to calculateSum is not really a valid representation of a number? Say 134w4. int.Parse("134w4") will fail miserably, your application will crash and your user will cry unable to understand what went wrong.

    You have tons of options here, I'd recommend you start reading here.

    private int calculateSum(string text1, string text2, string text3, string text4, string text5, string text6, string text7, string text8)
    {
        int A, B, C, D, E, F, G, H;
    
        try
        {
            A = int.Parse(TextBox1);
            B = int.Parse(TextBox2);
            C = int.Parse(TextBox3);
            D = int.Parse(TextBox4);
            E = int.Parse(TextBox5);
            F = int.Parse(TextBox6);
            G = int.Parse(TextBox7);
            H = int.Parse(TextBox8);
        }
        catch (FromatException)
        {
            MessageBox.Show("Ooops! You entered an invalid number.");
        }
        catch (OverflowEcxeption)
        {
            MessageBox.Show("Ooops! You entered an number that it too big.");
        }
        return A + B + C + D + E + F + G + H;
    }
    
  5. Can we do any better? Well yes, calculateSum seems too clunky with all these arguments. Lets improve it. Weclome to the params keyword:

    private int calculateSum(params string[] arguments)
    {
         int sum = 0;
    
         try
         {
             foreach (var arg in arguments)
             {
                 sum += int.Parse(arg);
             }
         }
         catch (FromatException)
         {
             MessageBox.Show("Ooops! You entered an invalid number.");
         }
         catch (OverflowEcxeption)
         {
             MessageBox.Show("Ooops! You entered an number that it too big.");
         }
    
         return sum;
    }
    
  6. Best practices will tell you to start using int.TryParse() instead of int.Parse(). Also, it will provide the perfect example to understand the out keyword. Check out how it works, understand it. I'll leave that to you.

InBetween
  • 32,319
  • 3
  • 50
  • 90
  • Thank you this makes a lot of sense, i really appreciate how much detail you have gone into to explain what i have done wrong. Thanks, i will look into everything you have said further – Stefan May 10 '16 at 10:10
0

I think you can better use instead of strings the NumericUpDown control. It is a specific control for entering numeric values and has the following advantages compared to using string input controls:

  • Only able to enter numeric values
  • Has two small buttons for increasing/decreasing
  • Has a special property to get the value
  • You don't need to convert to strings to sum up.

See How do I make a textbox that only accepts numbers? for more info.

Community
  • 1
  • 1
Michel Keijzers
  • 15,025
  • 28
  • 93
  • 119
0

You could write calc to use a params array

private string Calc(params string[] values)
{
  int total = 0;

  foreach(var value in values)
  {
    total += int.Parse(value);
  }

  return total.ToString();
}

params indicates that the function takes a variable number of arguments, so in the above the function takes 0 or more strings. The compiler will store the passed values in an array, so that you can access them using regular array accessing code.

If you want to use Linq then you can use the Sum extension method:

private string Calc(params string[] values)
{
  int total = values.Sum(value => int.Parse(value));
  return total.ToString();
}
Sean
  • 60,939
  • 11
  • 97
  • 136
0

I'm assuming you want to know what is wrong with your code rather than rewriting it. Your calc function calculates the proper value however the button7_Click function never receives it. Setting TextAnswer equal to the answer will not work since it is being passed by value instead of reference. Alternatively you can return the value and set it equal to lblTotal1.Text. Examples for both are below.

For passing by reference

private void button7_Click(object sender, EventArgs e)
{
    calc(textBox1.Text, textBox2.Text, textBox3.Text, textBox4.Text, textBox5.Text, textBox6.Text, textBox7.Text, textBox8.Text, out lblTotal1.Text);
}
private void calc(string TextBox1, string TextBox2, string TextBox3, string TextBox4, string TextBox5, string TextBox6, string TextBox7, string TextBox8, out string TextAnswer)
{
    // Same code as original
}

For returning the value

private void button7_Click(object sender, EventArgs e)
{
     lblTotal1.Text = calc(textBox1.Text, textBox2.Text, textBox3.Text, textBox4.Text, textBox5.Text, textBox6.Text, textBox7.Text, textBox8.Text);
}
// Note the changed calc parameters
private void calc(string TextBox1, string TextBox2, string TextBox3, string TextBox4, string TextBox5, string TextBox6, string TextBox7, string TextBox8)
{
    // Same code as original but change last line to
    return Total.ToString();
}
M-bot
  • 1
  • 3