私は WPF
に簡単な + - */電卓を実装しました。私はここで同じことのいくつかの実装があることを知っていますが、それぞれが異なっていて、これに対して異なる長所または短所があります。あなたは私を提案することができます、私のコードで何が改善されることができますか?
// calculator operations
public enum CalcOperator
{
None,
Plus,
Minus,
Times,
Divide
}
public partial class Calculator : Window
{
//decimal separator of current culture
char decimSepar = Convert.ToChar
(CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator);
private double a = 0; //the first number
private double b = 0; //second number
//last selected operation
private CalcOperator lastOper = CalcOperator.None;
public Calculator()
{
InitializeComponent();
}
//handle single digit (button)
public void HandleDigit(int i)
{
string str = txtDisp.Text;
if (lastOper == CalcOperator.None && a != Convert.ToDouble(str) ||
lastOper != CalcOperator.None && b != Convert.ToDouble(str) ||
str == "0")
{
str = string.Empty;
}
str += i.ToString();
if (lastOper == CalcOperator.None)
a = Convert.ToDouble(str);
else
b = Convert.ToDouble(str);
txtDisp.Text = str;
}
//handle calculator operation (button)
public void HandleOperator(CalcOperator oper)
{
txtDisp.Text = b.ToString();
lastOper = oper;
}
//handle decimal separator selection (button)
public void HandleDecimal()
{
if (!txtDisp.Text.Contains(decimSepar))
txtDisp.Text += decimSepar;
}
//compute the result
public void Compute()
{
double result = 0.0;
switch (lastOper)
{
case CalcOperator.Plus:
result = a + b;
break;
case CalcOperator.Minus:
result = a - b;
break;
case CalcOperator.Times:
result = a * b;
break;
case CalcOperator.Divide:
if (b == 0.0)
result = 0.0;
else
result = (double)a/b;
break;
}
txtDisp.Text = result.ToString();
lastOper = CalcOperator.None;
a = 0;
b = 0;
}
private void Window_Loaded(object sender, RoutedEventArgs e)
{
btnPlus.Tag = CalcOperator.Plus;
btnMinus.Tag = CalcOperator.Minus;
btnTimes.Tag = CalcOperator.Times;
btnDivide.Tag = CalcOperator.Divide;
}
private void OnClickDigit(object sender, RoutedEventArgs e)
{
Button btn = sender as Button;
HandleDigit(Convert.ToInt16(btn.Content.ToString()));
}
private void OnClickOperator(object sender, RoutedEventArgs e)
{
Button btn = sender as Button;
HandleOperator((CalcOperator)btn.Tag);
}
private void btnEqual_Click(object sender, RoutedEventArgs e)
{
Compute();
}
private void btnDot_Click(object sender, RoutedEventArgs e)
{
HandleDecimal();
}
}
誰かがWPF計算機を作ったのを見てうれしいです、そして私がすでに述べたことに追加できることを願っています。始める前に、「はい、これはMVVMの猛威です」と言っています。邪魔にならないようになったので、なぜ人がアプローチの背後にあるコードを実行するのでしょうか。 1つは、大量の人々が使用されていることです(私はその1人がJavaから来て、その後Winformに行き、最終的にWPFに着陸しました)2つ目は、一見コンパクトで理解しやすいことです。それで、なぜ人はそれをするべきですか? 1つの理由は、FrameworkElementプロパティをコードに入れたくないということです。 string str = txtDisp.Text;
、 txtDisp.Text = str;
、 txtDisp.Text = b.ToString();
...など
なぜそれが役に立つの?ロジックが正常であることを確認するためにコードのテストを作成するのであれば、お気に入りのテストフレームワークにウィンドウを設定したくないでしょう。 (私を試してみてください。)しかし、通常のプリミティブプロパティを使ってViewModelを設定し、それらのプロパティにWindowをバインドすると、テストがはるかに簡単になります。私のViewModelはこんな感じでこんなにソートされていると想像できる
public class CalculatorViewModel : INotifyPropertyChanged
{
public string Display {get; private set;}
public ICommand NumericButtonPressedCommand {get; private set;}
//... other things to set this all up
//called from the UI and state contains a string of 0-9
private void NumericButtonPressed(object state)
{
//convert state to int, update display
}
}
TIP you can find much more information about MVVM and how to properly bind and setup commands all over the internet.
上記のビューモデルは、次のようなもので簡単にテストできます。
public class CalculatorViewModelTest
{
[Test]
public void WhenNumericButtonIsPressedDigitIsShownOnDisplay()
{
var viewModel = new CalculatorViewModel();
NumericButtonPressedCommand.Execute("4");
Assert.That(viewModel.Display, Is.EqualTo("4"));
NumericButtonPressedCommand.Execute("4");
NumericButtonPressedCommand.Execute("2");
NumericButtonPressedCommand.Execute("4");
Assert.That(viewModel.Display, Is.EqualTo("4,242"));
}
}
これがあなたのロジックをあなたのUIから切り離すのが良い理由です。テストが簡単になります。
これについては十分に説明しました。あなたがそれを試してみることにした場合、あなたは何度も何度もそれを読むでしょう。あなたのコードはあなたのボタンの Tag
オブジェクトプロパティを使用しています。IMOは4つ(またはそれ以上)のオペレータClickListenerを設定するよりも明確ではありません。数日または数週間のうちにこのコードをもう一度見て、プラスボタンが差し引かれたと言った場合。最初に Compute
を見ると、 CalcOperator
が lastOper
に設定されていることがわかります。 OnClickOperator
から呼び出され、Tagを使用して HandleOperator
によって設定されている場所を確認します。今、あなたはTagがどこに設定されているか探しています、そしてあなたはついにあなたが誤ってPlusボタンのTagをSubtractに設定したのを見ます。コピー貼り付けエラー。個々のOperator ClickListenerを持っていれば、おそらく OnClickAddition
に移動して、誤ってlastOperをSubtractに設定していることがわかります。 (実際にはそうしませんでしたが、いくつかのことを詳しく説明しておくとバグの追跡がより簡単になります。常にではありませんが、時には)
これは私が改善すべきだと思うものです:
private double a = 0; //最初の番号 プライベートダブルb = 0。//2番目の番号
Encapsulate those in a class/struct like UserInput
public void HandleDigit(int i)
i
is not a good parameter name, it should be called digit
if this is what your method handles.
lastOper
非常に一般的な の省略形でなければ、名前は通常省略形にするべきではないと思います。この場合、私はフルネームの lastOperation
を使用します。
同じことが oper
、 decimSepar
、 txtDisp
など、他のいくつかの変数やパラメータにも当てはまります。
//電卓の操作を処理する(ボタン) public void HandleOperator(CalcOperator oper)
それらをxml-commentsにしない限り、インテリセンスはそれらを表示しないので、これらのコメントはあまり役に立ちません。また、(button)
という単語はあまり役に立ちません。
public void Compute(){...}
このメソッドは結果だけを返します。
public double Compute() { ... }
結果も出力されるため、現在のところ複数の責任があります。
txtDisp.Text = result.ToString();
この行を UpdateTextBox
のような新しいメソッドに移動し、 Compute
メソッドを呼び出した後にも lastOper
をリセットします。たとえば HandleOperator
の中など、UIを更新する他のいくつかの場所でも同じことを行います。
計算機をUIから切り離して、別々にテストできるようにします。現在、あなたはそれのためのどんな単体テストも書くことができないでしょう。ウィンドウの名前を CalculatorWindow
に変更し、計算ロジック全体を Calculator
という名前の新しいクラスに移動します。
これには、後で電卓アプリにMVVMまたはASP.NETなどを使用することにした場合、電卓自体ではなく新しいUIを作成するだけで済みます。
このパターンは、明確でテスト可能な方法でビジネスロジックとUIを分離するのに役立ちます。
CalculatorState
and CalculatorLogic
is your model/BL and it can be made shareable. Any calculation can take your model and modify it with a result or with an error message (divide by zero etc). It represents the entire I/O of your business logic (calculation on a calculator), you could expand it to support new features, such as history, with minimum refactoring of other code.
CalculatorWindow/xaml
is your UI, this is the visible part of your code
CalculatorVM
is the glue between UI and model.
// model class
public class CalculatorState : INotifyPropertyChanged /*IMPLEMENT, ON ALL PROPERTIES */ {
public bool IsError{get;set;}
public string ErrorMessage{get;set;}
//Value is what's on the calculator screen under normal conditions
public double Value {get;set;}
//the calculator's memory
private double? mem;
public double Mem {
get { return mem.GetValueOrDefault(Value); }
set { mem = value; }
}
}
// business logic
public static class CalculatorLogic{
public static readonly Action Add = (state,prm)=>state.Value = state.Mem + state.Value;
public static readonly Action Sub = (state,prm)=>state.Value = state.Mem - state.Value;
}
// VM component
public class CalculatorCommand: ICommand{
public CalculatorState State {get;set;}
public readonly Action Calculate;
public readonly bool IsTwoOpCommand;
public CalculatorCommand(Action calculate, CalculatorState state = null, bool isTwoOpCommand = true){
Calculate = calculate;
State = state;
IsTwoOpCommand = isTwoOpCommand;
}
public void Execute(double? prm){
if (State!=null){
if (Calculate!=null){
//for two-op commands without a Mem put the Value in Mem
if (!IsTwoOpCommand || State.Mem.HasValue)
Calculate(State);
else
State.Mem = State.Value;
} else {
State.IsError = true;
State.ErrorMessage = "Null function";
}
} else//throw if you wish
Debug.WriteLine("Unexpected empty state");
}
}
// View-Model, links your UI to the model
public CalculatorVM : INotifyPropertyChanged {
public readonly CalculatorState State;
public readonly ICommand AddCommand;
public readonly ICommand SubCommand;
....
public readonly ICommand NumberCommand;
public CalculatorVM(CalculatorState state){
State = state;
NumberCommand = new CalculatorCommand(c,p=>c.Value = c.Value*10 + p, State, false);
SubCommand = new CalculatorCommand(CalculatorLogic.Sub State);
AddCommand = new CalculatorCommand(CalculatorLogic.Add, State);
}
}
// View (UI). If you did the rest of the work your UI class should be mostly empty,
// most of the setup would be done in the declarative part (XAML) via bindings
// this allows you to reuse your entire business logic, unit test included
// when you decide to switch platforms (desktop, mobile, server)
public CalculatorWindow: Window{
public CalculatorWindow(){
BindingContext = new CalculatorVM(new CalculatorState());
InitializeComponent();
}
}
CalculatorWindow.xaml:
...
...
...
...
t3chb0t の回答によると、私は自分の質問のコードをより良くすることを試みました。しかし、私はまだ計算機ロジックからのUIの分離についてはよくわかりません。
CalculatorWindow
では、 CalculatorWindow
のインスタンスが挿入される Calculator
インスタンスを作成します。 CalculatorWindow
クラスで、私は現在UIの変更の代わりに何もしていません。ボタンの Tag
プロパティは XAML
内に設定されています。
public partial class CalculatorWindow : Window
{
///
/// decimal separator of current culture
///
private char decimalSeparator = Convert.ToChar
(CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator);
private Calculator calculator;
public string DisplayText
{
get { return txtDisplay.Text; }
set { txtDisplay.Text = value; }
}
public CalculatorWindow()
{
calculator = new Calculator(this);
InitializeComponent();
}
///
/// clicked on digit button
///
/// <param name="sender">
/// clicked on operation button
///
/// <param name="sender">
/// clicked on equal button
///
/// <param name="sender">
/// clicked on decimal button
///
/// <param name="sender">
次は CalcOperator enum
と UserInput
のデータ実装です。ここでは、 UserInput
のパブリックメンバーに関する唯一の問題を確認しましたが、これは非常に単純なクラスなので、軽いものにしました。
///
/// calculator operations
///
public enum CalcOperator
{
None,
Plus,
Minus,
Times,
Divide
}
///
/// user input data
///
public class UserInput
{
public double a = 0.0;
public double b = 0.0;
public CalcOperator operation = CalcOperator.None;
}
そして最後に Calculator
ロジッククラスの実装です。ここでは、 CalculatorWindow
のUI TextBox.Text
プロパティに触れているので、 public void HandleDigit(int digit)
関数が正しく実装されていないと感じます。これは今どのように改善されたでしょうか。
///
/// calculator engine
///
class Calculator
{
private UserInput input = new UserInput();
private CalculatorWindow window;
public Calculator(CalculatorWindow window)
{
this.window = window;
}
///
/// compute the result
///
///
public double Compute()
{
double result = 0.0;
switch (input.operation)
{
case CalcOperator.Plus:
result = input.a + input.b;
break;
case CalcOperator.Minus:
result = input.a - input.b;
break;
case CalcOperator.Times:
result = input.a * input.b;
break;
case CalcOperator.Divide:
if (input.b == 0.0)
result = 0.0;
else
result = (double)input.a/input.b;
break;
}
input.operation = CalcOperator.None;
input.a = 0;
input.b = 0;
return result;
}
///
/// set user input operation
///
/// <param name="operation">
/// handle single digit
///
/// <param name="digit">