HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

WPF Calculator Code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codewpfcalculator

Problem

I've coded a calculator in C# with WPF as the UI.

I wish to know mainly about these points:

  • Ways of optimizing



  • Better techniques, tactics and ways of coding this



  • All flaws on the surface as well as deep



  • Simpler logic



MainWindow.xaml.cs

```
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Data;
using System.Windows.Documents;
using System.Windows.Input;
using System.Windows.Media;
using System.Windows.Media.Imaging;
using System.Windows.Navigation;
using System.Windows.Shapes;

namespace NewCalculator
{
public partial class MainWindow : Window
{
static int[] numbersArray = new int[10];
static string[] operatorsArray = new string[9];

static string storageVariable;
static int numbersCounter = 0;
static int operatorsCounter = 0;
static int total = 0;
static bool totalled = false;

public MainWindow()
{
InitializeComponent();
}

private void One_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "1";
storageVariable += "1";
}
private void Two_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "2";
storageVariable += "2";
}
private void Three_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "3";
storageVariable += "3";
}
private void Four_Click(

Solution

Duplication

if (totalled == true)
{
    Display.Content = "";
    totalled = false;
}
Display.Content += "1";
storageVariable += "1";

if (totalled == true)
{
    Display.Content = "";
    totalled = false;
}
Display.Content += "2";
storageVariable += "2";


All the way up to 0. (1-9 + 0)
If you can somehow figure out which button caused the click event, you can make 1 click function for all your numeric buttons. You can also do this for your +, -, / and * buttons.

Bugs

private void AC_Click(object sender, RoutedEventArgs e)
{
    Display.Content = "";
    numbersArray = null;//null pointer exception on pressing any of the arithmetic buttons
    operatorsArray = null;//null pointer exception on pressing any of the arithmetic buttons
    storageVariable = null;//null pointer exception on pressing any of the numeric buttons
    numbersCounter = 0;
    operatorsCounter = 0;
    total = 0;
}


Don't set to null, that leads to things breaking. AC clears the current calculation on a calculator - so set the values back to the initial values.

static void setNumber(String Number)
{
    numbersArray[numbersCounter] = Convert.ToInt16(Number);//no bounds check
    storageVariable = null;//possible null pointer when clicking arithmetic button
    numbersCounter++;
}
static void setOperator(String Op)
{
    operatorsArray[operatorsCounter] = Op;//no bounds check
    operatorsCounter++;
}


You're missing bounds checks. Because you've limited the amount of numbers and operations that are allowed, it's possible to get an out of range exception (or whatever C#'s exception is for going out of an array's bounds).

Code Snippets

if (totalled == true)
{
    Display.Content = "";
    totalled = false;
}
Display.Content += "1";
storageVariable += "1";

if (totalled == true)
{
    Display.Content = "";
    totalled = false;
}
Display.Content += "2";
storageVariable += "2";
private void AC_Click(object sender, RoutedEventArgs e)
{
    Display.Content = "";
    numbersArray = null;//null pointer exception on pressing any of the arithmetic buttons
    operatorsArray = null;//null pointer exception on pressing any of the arithmetic buttons
    storageVariable = null;//null pointer exception on pressing any of the numeric buttons
    numbersCounter = 0;
    operatorsCounter = 0;
    total = 0;
}
static void setNumber(String Number)
{
    numbersArray[numbersCounter] = Convert.ToInt16(Number);//no bounds check
    storageVariable = null;//possible null pointer when clicking arithmetic button
    numbersCounter++;
}
static void setOperator(String Op)
{
    operatorsArray[operatorsCounter] = Op;//no bounds check
    operatorsCounter++;
}

Context

StackExchange Code Review Q#57806, answer score: 8

Revisions (0)

No revisions yet.