de-vraag
  • 質問
  • タグ
  • ユーザー
通知:
報酬:
登録
登録すると、質問に対する返答やコメントが通知されます。
ログイン
すでにアカウントをお持ちの方は、ログインして新しい通知を確認してください。
追加された質問、回答、コメントには報酬があります。
さらに
ソース
編集
ゲストユーザ
質問

C#WPFシンプル電卓

私は 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();
    }

}
8 2015-10-26T08:50:27+00:00 4
コードレビュー
c#
calculator
wpf
mbeckish
26日 10月 2015 в 12:23
2015-10-26T12:23:10+00:00
さらに
ソース
編集
#84935388

誰かが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に設定していることがわかります。 (実際にはそうしませんでしたが、いくつかのことを詳しく説明しておくとバグの追跡がより簡単になります。常にではありませんが、時には)

10
0
t3chb0t
26日 10月 2015 в 9:57
2015-10-26T09:57:00+00:00
さらに
ソース
編集
#84935387

これは私が改善すべきだと思うものです:


  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を作成するだけで済みます。

8
0
Ella
26日 10月 2015 в 7:08
2015-10-26T19:08:36+00:00
さらに
ソース
編集
#84935390

MVVMパターン

このパターンは、明確でテスト可能な方法でビジネスロジックと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:


...
  
...
  
...
   
... 
    

3
0
Sebastian Paaske Tørholm
26日 10月 2015 в 2:53
2015-10-26T14:53:02+00:00
さらに
ソース
編集
#84935389

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">
0
0
質問の追加
カテゴリ
すべて
技術情報
文化・レクリエーション
生活・芸術
科学
プロフェッショナル
事業内容
ユーザー
すべて
新しい
人気
1
Денис Анненский
登録済み 2日前
2
365
登録済み 6日前
3
True Image
登録済み 6日前
4
archana agarwal
登録済み 1週間前
5
Maxim Zhilyaev
登録済み 1週間前
© de-vraag :年
ソース
codereview.stackexchange.com
ライセンス cc by-sa 3.0 帰属