2016-04-15 2 views
3

私はちょうどいくつかのスイッチステートメントに少し問題があります。私は最終目的を達成するためのより良い方法があると感じています。c#ヌルチェック付きのリファクタリングスイッチ文

本質的に私はメソッドにviewmodelを渡しています。このメソッドは、最初にビューモデルが関連するオブジェクトをデータベースから取得し、その後、switch文は特定のプロパティに対してnullチェックを行います。その結果に基づいて、別のswitch文は、ビューモデルに対して別のnullチェックを行います。各ポイントでデータベースからオブジェクトに値が割り当てられ、最後にデータベースの更新が行われます。ここで

コード

 public async Task UpdateContractWithRepository(ViewModel viewModel) 
    { 
     // Get the contract from db 
     Contract contract = GetContract(viewModel.Id); 

     switch (viewModel.RepositoryId == null) 
     { 
      case true: 
       switch (contract.RepositoryId == null) 
       { 
        case true: 
         // nothing to do 
         // no change 
         break; 
        case false: 
         // Unassign Repository 
         UpdateRepositoryAssignment(contract.RepositoryId, false); 

         // Update properties 
         contract.RepositoryId = null; 
         contract.ConsumedUnits = null; 
         break; 
       } 
       break; 
      case false: 
       switch (contract.RepositoryId == null) 
       { 
        case true: 
         // assign repository 
         UpdateRepositoryAssignment(viewModel.RepositoryId, true); 

         // Get repository 
         Repository repository = GetRepository(viewModel.RepositoryId); 

         // Update properties 
         contract.RepositoryId = repository.Id; 
         contract.ConsumedUnits = repository.Units; 
         break; 
        case false: 
         // assign repository 
         UpdateRepositoryAssignment(viewModel.RepositoryId, true); 

         // Get repository 
         Repository repository = GetRepository(viewModel.RepositoryId); 

         // Update properties 
         contract.RepositoryId = repository.Id; 
         contract.ConsumedUnits = repository.Units; 
         break; 
       } 
       break; 

     } 

     UpdateContract(contract); 
    } 

は、私は私はおそらくswitch文を廃止し、文ではなく、まだ私が言うことができるものから、いくつかのネストが存在することになる場合には使用することができます思っています。誰かが何か提案があったかどうかだけ考えました。

私はここでswitch文をリファクタリングしましたが、実際にはnullチェックをカバーしていないようです。

ご協力いただきましてありがとうございます。

+0

なぜ簡単なif/then/else文でswitch文を使用していますか? if文に切り替えるほうが良いのではないでしょうか? また、スイッチで多くの作業をしている場合は、独自の(プライベート)メソッドを組み込む方が良いでしょう。 –

+0

2番目のswitch文にはバグがあるようです...真と偽の両方が同じコードを実行します... – forsvarir

答えて

1

bool IsVMRepoNull = viewModel.RepositoryId == null; 
bool IsContractRepoNull = contract.RepositoryId == null; 

if(IsVMRepoNull && !IsContractRepoNull) 
{ 
    UpdateRepositoryAssignment(contract.RepositoryId, false); 

// Update properties 
    contract.RepositoryId = null; 
    contract.ConsumedUnits = null; 
} 
else if(!IsVMRepoNull) 
{ 
    UpdateRepositoryAssignment(viewModel.RepositoryId, true); 

    // Get repository 
    Repository repository = GetRepository(viewModel.RepositoryId); 

    // Update properties 
    contract.RepositoryId = repository.Id; 
    contract.ConsumedUnits = repository.Units; 
} 
1

どこにいるのか分かりませんが、スイッチをif/elseに置き換えるのは、ほとんど同じようにswitchifに置き換えるだけです。

1

私はそう複雑にする必要はないと思います。

public async Task UpdateContractWithRepository(ViewModel viewModel){ 
     Contract contract = GetContract(viewModel.Id); 

     if (viewModel.RepositoryId == null) 
     { 
      if(contract.RepositoryId != null){ 
       UpdateRepositoryAssignment(contract.RepositoryId, false); 
       contract.RepositoryId = null; 
       contract.ConsumedUnits = null; 
      } 
     } 
     else 
     { 
      if (contract.RepositoryId == null) 
      { 
       UpdateRepositoryAssignment(viewModel.RepositoryId, true); 
       Repository repository = GetRepository(viewModel.RepositoryId); 
       contract.RepositoryId = repository.Id; 
       contract.ConsumedUnits = repository.Units; 
      } 
      else 
      { 
       UpdateRepositoryAssignment(viewModel.RepositoryId, true); 
       Repository repository = GetRepository(viewModel.RepositoryId); 
       contract.RepositoryId = repository.Id; 
       contract.ConsumedUnits = repository.Units; 
      } 
     } 
     UpdateContract(contract); 
    } 
0

私は本当にブール条件でif....else ifを使用します。 私はそれが同じ仕事をすることを考えて、これを試してみてください。あなただけの2 bools取る場合は、全体のコードを簡素化することができる

public async Task UpdateContractWithRepository(ViewModel viewModel) 
{ 
    // Get the contract from db 
    Contract contract = GetContract(viewModel.Id); 

    bool modelRepositoryKnown = viewModel.RepositoryId != null; 
    bool contractRepositoryKnown = contract.RepositoryId != null; 

    if (modelRepositoryKnown) 
    { 
     if (contractRepositoryKnown) 
     { 
      // assign repository 
      UpdateRepositoryAssignment(viewModel.RepositoryId, true); 

      // Get repository 
      Repository repository = GetRepository(viewModel.RepositoryId); 

      // Update properties 
      contract.RepositoryId = repository.Id; 
      contract.ConsumedUnits = repository.Units; 
     } 
     else 
     { 
      // assign repository 
      UpdateRepositoryAssignment(viewModel.RepositoryId, true); 

      // Get repository 
      Repository repository = GetRepository(viewModel.RepositoryId); 

      // Update properties 
      contract.RepositoryId = repository.Id; 
      contract.ConsumedUnits = repository.Units; 
     } 
    } 
    else if(contractRepositoryKnown) // Model-Repository unknown but Contract-Repository Known 
    { 
     // Unassign Repository 
     UpdateRepositoryAssignment(contract.RepositoryId, false); 

     // Update properties 
     contract.RepositoryId = null; 
     contract.ConsumedUnits = null; 
     break; 
    } 
    UpdateContract(contract); 
} 
0

をあなたのネストされたスイッチの条件を組み合わせることにより、あなたはそれを達成することができますが、意味のある変数-名前でそれを読みやすくすることができます複数の条件を使用するif else文に変換します。

if(viewModel.RepositoryId == null && !contract.RepositoryId) 
{ 
    // Unassign Repository 
    UpdateRepositoryAssignment(contract.RepositoryId, false); 
    // Update properties 
    contract.RepositoryId = null; 
    contract.ConsumedUnits = null; 
} 
else if(!viewModel.RepositryId && contract.RepositoryId == null) 
{ 
    // assign repository 
    UpdateRepositoryAssignment(viewModel.RepositoryId, true); 
    // Get repository 
    Repository repository = GetRepository(viewModel.RepositoryId); 
    // Update properties 
    contract.RepositoryId = repository.Id; 
    contract.ConsumedUnits = repository.Units; 
} 
else if(!viewModel.RepositryId && !contract.RepositoryId == null) 
{ 
    // assign repository 
    UpdateRepositoryAssignment(viewModel.RepositoryId, true); 
    // Get repository 
    Repository repository = GetRepository(viewModel.RepositoryId); 
    // Update properties 
    contract.RepositoryId = repository.Id; 
    contract.ConsumedUnits = repository.Units; 
} 
関連する問題