Statement checker c# using forms and frontend

Question

I'm just here for suggestions and code optimisations, what i could've done better, are there any better coding practices... There's github repo link if you're interested in the code with a form too https://github.com/throtz/Operators You basically have 2 textboxes in which you enter numbers and choose with a combobox what you want to do with those two numbers.

using System;
using System.Windows.Forms;
namespace Operators
{
 public partial class Operators : Form
 {
 public Operators()
 {
 InitializeComponent();
 }
 private void Form1_Load_1(object sender, EventArgs e)
 {
 comboBox.Items.AddRange(new string[] { "+", "-", "*", "/", "%", ">", "<", "=", ">=", "<=" });
 }
 private void ButtonCheck_Click_1(object sender, EventArgs e)
 {
 if (sender is null)
 {
 throw new ArgumentNullException(nameof(sender));
 }
 int a = Convert.ToInt32(textBox1.Text);
 int b = Convert.ToInt32(textBox2.Text);
 Nah(a, b);
 }
 private void Nah(int a, int b)
 {
 switch (comboBox.SelectedItem.ToString())
 {
 case "+":
 MessageBox.Show("Sum of thenumbers entered is: " + (a + b));
 break;
 case "-":
 MessageBox.Show("Subtraction of the numbers entered is: " + (a - b));
 break;
 case "*":
 MessageBox.Show("Multiplication of the numbers entered is: " + (a * b));
 break;
 case "/":
 MessageBox.Show("Division of the numbers entered is: " + (a / b));
 break;
 case "%":
 MessageBox.Show("Modulo the two numbers is: " + (a % b));
 break;
 case ">":
 MessageBox.Show("First number is bigger than the secound one " + (a > b));
 break;
 case "<":
 MessageBox.Show("First number is smaller than the secound one " + (a < b));
 break;
 case "=":
 MessageBox.Show("Numbers are equal: " + (a == b));
 break;
 case ">=":
 MessageBox.Show("First number is bigger or equal to number two " + (a >= b));
 break;
 case "<=":
 MessageBox.Show("First number is smaller or equal to number two " + (a <= b));
 break;
 default:
 MessageBox.Show("Illegal operation.");
 break;
 }
 }
 }
}

Answer 1

Review

It seems that you are trying to make a simple calculator with number comparison in C#. There are some problems in this implementation.

ToInt32:

Do you expect the input of calculation is always in int format? How about floating numbers?

Nah method:

• Naming:

  I have no idea how's the name Nah imply the operation in the method. Something like run or perform are better than Nah.

• Integer Division:

  Notice that the execution of 3 / 2 in this implementation is 1. It is a kind of integer division. Is the integer division result the correct output you expected?

• MessageBox.Show Part:

  In C# 6, you can use string interpolation and make MessageBox.Show("Sum of thenumbers entered is: " + (a + b)); into MessageBox.Show($"Sum of thenumbers entered is: {(a + b)}");. Interpolated strings are useful in string displaying and it's commonly used with Console.WriteLine or MessageBox.Show.

Error Handling

• Null or empty string:

  You use textBox1.Text and textBox2.Text as a way to read numbers from user input. How about the case of empty strings (user click button Check directly without type any character)? If textBox.Text passes in Convert.ToInt32 directly and the content is empty, System.FormatException will occur. Maybe String.IsNullOrEmpty check can be used here.

• DivideByZeroException

  DivideByZeroException Class can be used to handle an attempt to divide an number by zero.

Answer 2

• I would suggest moving your new string[] { "+", "-", "*", "/", "%", ">", "<", "=", ">=", "<=" } into a global private readonly variable. because it's a requirement in your class, and current approach will hide this requirement from the human-eye.

  class requirement should always be visible, and readable to the human-eye such as declaring variables, fields at the top of the class. It would make it easier to approach and modify.

• Convert.ToInt32 it would be fine if you know that the stored string is always a valid int, however, it would much better to use int.TryParse or better yet using Decimal.TryParse to validate the string, and avoid any parsing exceptions.

• Nah(int a, int b) not following the appropriate naming convention, always pick a good name to your variables, methods, and classes that describes their role and actions.

• Nah(int a, int b) is mixing two roles (calculate and show message), you should separate them.

Another way of doing this is to convert the operators into objects by creating a class that would describe these operators, and another class to hold the process of these operators.

Here is a quick example :

public class OperatorSign
{
 public string Sign { get; }
 public string Name { get; }
 public OperatorSign(string sign , string name)
 {
 Sign = sign;
 Name = name;
 }
}
public class MathOperation
{
 public OperatorSign Operator { get; private set; }
 public int Total { get; private set; }
 public MathOperation(OperatorSign sign)
 {
 Operator = sign;
 }
 public void Calculate(int numberX , int numberY)
 {
 switch(Operator.Sign)
 {
 case "+":
 Total = numberX + numberY;
 break;
 case "-":
 Total = numberX - numberY;
 break;
 case "*":
 Total = numberX * numberY;
 break;
 default:
 return; //do nothing.
 }
 }
 public override string ToString()
 {
 return $"{Operator.Name} of the numbers entered is: {Total}";
 }
}

Now using these, we can do this :

public partial class Operators : Form
{
 private readonly OperatorSign[] _operatorSigns = new OperatorSign[]
 {
 new OperatorSign("+", "Sum"),
 new OperatorSign("-", "Subtraction"),
 new OperatorSign("*", "Multiplication")
 };
 public Operators()
 {
 InitializeComponent();
 }
 
 private void Form1_Load_1(object sender, EventArgs e)
 { 
 comboBox.Items.AddRange(_operatorSigns.Select(x=> x.Sign));
 }
 
 private void ButtonCheck_Click_1(object sender, EventArgs e)
 {
 if (sender is null)
 {
 throw new ArgumentNullException(nameof(sender));
 }
 if(!int.TryParse(textBox1.Text, out int a))
 {
 MessageBox.Show("Invalid Number");
 }
 else if(!int.TryParse(textBox2.Text, out int b))
 {
 MessageBox.Show("Invalid Number");
 }
 else
 {
 var selectedSign = comboBox.SelectedItem.ToString();
 
 var sign = _operatorSigns.FirstOrDefault(s => s.Sign.Equals(selectedSign));
 if(sign == null)
 {
 MessageBox.Show("Invalid Operator");
 }
 else 
 {
 var mathOp = new MathOperation(sign);
 mathOp.Calculate(x , y);
 var resultMessage = mathOp.ToString();
 
 MessageBox.Show(resultMessage);
 
 }
 }
 }
 
}

Now you can extend the operators such as adding more handlers and more rules to each sign without touching the form class itself (presentation layer). You can also take advantage of class inheritance or delegate and Expression to keep your code much prettier.

Answer 3

On the user's side, there is more interaction needed than strictly necessary. You could remove the button and provide a "live update" whenever one of the numbers or the operator changes. This makes interaction with your example quicker and smoother as you avoid a change of the active window.

Instead of MessageBox.Show you could simply update ResultLabel.Text. This way you get rid of the many repetitions of MessageBox.Show. Another benefit is that if you extract the generation of the result text into a separate method, you don't need the break statements anymore. Something like this:

string GenerateResult(int a, string op, int b)
{
 switch (op) {
 case "+": return $"The sum of {a} and {b} is {a + b}";
 case "-": return $"The difference between {a} and {b} is {a - b}";
 ...
 }
}

In ButtonCheck_Click_1, you don't need to check whether sender is null since you don't use that variable afterwards.