このC＃コードを最適にリファクタリングするにはどうすればよいですか？

次のコードをリファクタリングしたいのですが、どのように通知ハンドラを挿入しますか？必要に応じて最小限のオリジナルコード変更と最適なリファクタリング私はすでに以下のようにそれをリファクタリングしているこのC＃コードを最適にリファクタリングするにはどうすればよいですか？

public class TestEventHandlers 
{ 
    public TestEventHandlers() { } 

    public void OpenMarket(Page page) 
    { 
     var Id = page.Request["MarketId"]; 

     var repository = new EntityRepository(); 
     IEntity market = repository.GetById(Id); 

     if (market.State != "Open") 
     { 
      throw new Exception("The market is not open!"); 
     } 
     else 
     { 
      market.Open(); 

      repository.SaveChangesTo(market); 

      var smtpClient = new SmtpClient(); 

      var message = new MailMessage(); 
      message.Subject = "market open"; 
      message.Body = market.ToString() + " was open."; 
      message.To.Add(new MailAddress("[email protected]")); 

      smtpClient.Send(message); 
     } 
    } 

    public void CloseMarket(Page page) 
    { 
     var Id = page.Request["MarketId"]; 
     var repository = new EntityRepository(); 
     IEntity market = repository.GetById(Id); 

     if (market.State == "Close") 
     { 
      throw new Exception("The market is already close!"); 
     } 
     else 
     { 
      market.Close(); 

      repository.SaveChangesTo(market); 

      var smtpClient = new SmtpClient(); 

      var message = new MailMessage(); 
      message.Subject = "market closed"; 
      message.Body = market.ToString() + " has been closed."; 
      message.To.Add(new MailAddress("[email protected]")); 

      smtpClient.Send(message); 
     } 
    } 
}

public class TestEventHandlers 
{ 
     public TestEventHandlers() { } 

     public void OpenMarket(Page page) 
     { 
      var Id = page.Request["MarketId"]; 
      if (id!=null) 
      { 
      var repository = new EntityRepository(); 
      IEntity market = repository.GetById(Id); 

      if (market.State != "Open") 
      { 
       throw new Exception("The market is not open!"); 
      } 
      else 
      { 
       market.Open(); 

       repository.SaveChangesTo(market); 

       SendEmailNotification("market open", market.ToString() + " was open.", "[email protected]"); 
      } 
      } 
      else 
      { 
       throw new Exception("Id can not be null"); 
      } 
     } 

     private static void SendEmailNotification(string subject,string body,string emailAddress) 
     { 
      var smtpClient = new SmtpClient(); 

      var message = new MailMessage(); 

      message.Subject = subject; 
      message.Body = body; 
      message.To.Add(new MailAddress(emailAddress)); 

      smtpClient.Send(message); 
     } 

     public void CloseMarket(Page page) 
     { 
      var Id = page.Request["MarketId"]; 
      if(id!=null) 
      { 
      var repository = new EntityRepository(); 
      IEntity market = repository.GetById(Id); 

      if (market.State == "Close") 
      { 
       throw new Exception("The market is already close!"); 
      } 
      else 
      { 
       market.Close(); 

       repository.SaveChangesTo(market); 

       SendEmailNotification("market closed", market.ToString() + " has been closed.", "[email protected]"); 
      } 
     } 
      else 
      { 
       throw new Exception("Id can not be null"); 
      } 
     } 
    }

出典

2016-11-05 Neo

あなたは 'CloseMarket'と' OpenMarket'をマージしたいかもしれません。その状態を「閉じる」または「開く」に変更するだけです。パラメータは2つのメソッドを1に減らします。 – Badiparmagi

私はBadipamagiの提案に行きます。また、ヌルチェックも追加してください。 SendNotificationでも同じ新しいパラメータを使用できます。また、他の場所に通知を送信しない場合は、それをマージすることもテストポイントから行うこともできますし、懸念の分離を維持するためにそのまま放置することもできます。 –

もヌルチェックを追加しましたが、それは他のクラスやメソッドに巨大なリファクタリングを行うので、1つのメソッドとして作るのは良い考えではありません。つまり、 'OpenMarket（Page page）とCloseMarket（Pageページ）' – Neo

この@Neoを試してみてください私はこのようなものでいいと思う：で

public interface INotifier 
{ 
    void SendEmailNotification(string subject, string body, string emailAddress); 
} 

public class Notifier : INotifier 
{ 
    public void SendEmailNotification(string subject,string body,string emailAddress) 
    { 
     var smtpClient = new SmtpClient(); 

     var message = new MailMessage(); 

     message.Subject = subject; 
     message.Body = body; 
     message.To.Add(new MailAddress(emailAddress)); 

     smtpClient.Send(message); 
    } 
} 

public class TestEventHandlers 
{  
    public INotifier Notifier { get; set; } 

    public TestEventHandlers() 
    {   
     Notifier = new Notifier(); 
    } 

    public void OpenMarket(Page page) 
    { 
     var Id = page.Request["MarketId"]; 
     if (id!=null) 
     { 
      var repository = new EntityRepository(); 
      IEntity market = repository.GetById(Id); 

      if (market.State != "Open") 
      { 
       throw new Exception("The market is not open!"); 
      } 
      else 
      { 
       market.Open(); 

       repository.SaveChangesTo(market); 

       Notifier.SendEmailNotification("market open", market.ToString() + " was open.", "[email protected]"); 
      } 
     } 
     else 
     { 
      throw new Exception("entityId can not be null"); 
     }   
    } 

    public void CloseMarket(Page page) 
    { 
     var Id = page.Request["MarketId"]; 
     if(id!=null) 
     { 
      var repository = new EntityRepository(); 
      IEntity market = repository.GetById(Id); 

      if (market.State == "Close") 
      { 
       throw new Exception("The market is already close!"); 
      } 
      else 
      { 
       market.Close(); 
       repository.SaveChangesTo(market); 
       Notifier.SendEmailNotification("market closed", market.ToString() + " has been closed.", "[email protected]"); 
      } 
     } 
     else 
     { 
      throw new Exception("entityId can not be null"); 
     } 
    }  
}

このINotifierはデフォルトです通常はコンストラクタによって実装されますが、アクセサーを使用して必要なときにはオーバーライドすることができます。テストしていなくても依存関係を常に注入したい場合は、コンストラクタに引数を追加するだけです。

これが役に立ちます。

出典

2016-11-05 06:49:07 s3raph86

あなたはコピー貼り人なしであなた自身のコードを書いていたかもしれません.... – khaled4vokalz

公正なポイント@ khaled4vokalz - 正直言って、あなたのコピーはOPよりも簡潔でした。私はOpen/CloseMarketメソッドのコピーを2つ持っているとは思っていませんでした。すぐに変更を削除するように編集します。 – s3raph86

あなたはそれを仲間に残すことができます....その大丈夫....：） – khaled4vokalz

を

public class TestEventHandlers 
{ 
    public void OpenMarket(Page page) 
    { 
     ChangeMarketState(page, "Open", "[email protected]"); 
    } 

    public void CloseMarket(Page page) 
    { 
     ChangeMarketState(page, "Close", "[email protected]"); 
    } 

    private static void SendEmailNotification(string subject,string body,string emailAddress) 
    { 
     var smtpClient = new SmtpClient(); 

     var message = new MailMessage(); 

     message.Subject = subject; 
     message.Body = body; 
     message.To.Add(new MailAddress(emailAddress)); 

     smtpClient.Send(message); 
    } 

    public void ChangeMarketState(Page page, string changeStateTo, string sendMailTo) 
    { 
     var Id = page.Request["MarketId"]; 
     if(Id != null) 
     { 
      var repository = new EntityRepository(); 
      IEntity market = repository.GetById(Id); 

      if (market.state == changeStateTo) 
      { 
       if(changeStateTo == "Close") 
        throw new Exception("The market is already close!"); 
       else 
        throw new Exception("The market is not open!"); 
      } 
      else 
      { 
       string currentMarketState = string.empty; 
       string mailHeader = string.empty; 
       if(changeStateTo == "Close") 
       { 
        market.Close(); 
        currentMarketState = market.ToString() + " has been closed."; 
        mailHeader = "market closed"; 
       } 
       else 
       { 
        market.Open(); 
        currentMarketState = market.ToString() + " was open."; 
        mailHeader = "market open"; 
       } 

       repository.SaveChangesTo(market); 

       SendEmailNotification(mailHeader, currentMarketState, sendMailTo); 
      } 
     } 
     else 
     { 
      throw new Exception("entityId can not be null"); 
     } 
    } 
}

出典

2016-11-05 06:36:17 khaled4vokalz

このC＃コードを最適にリファクタリングするにはどうすればよいですか？

答えて

関連する問題