このコードはどこで改善できますか？

Question

using System; 
using System.Collections.Generic; 
using System.Linq; 
using System.Text; 

namespace MorseCode 
{ 
    public class MorseCodeConverter 
    { 
     public Dictionary<string, string> morseCode;   

     public MorseCodeConverter() 
     {    
      LoadMorseCode(); 
     } 

     private void LoadMorseCode() 
     { 
      morseCode = new Dictionary<string, string>(); 

      morseCode.Add("a", ".-"); 
      morseCode.Add("b", "-..."); 
      morseCode.Add("c", "-.-."); 
      morseCode.Add("d", "-.."); 
      morseCode.Add("e", "."); 
      morseCode.Add("f", "..-."); 
      morseCode.Add("g", "--."); 
      morseCode.Add("h", "...."); 
      morseCode.Add("i", ".."); 
      morseCode.Add("j", ".---"); 
      morseCode.Add("k", "-.-"); 
      morseCode.Add("l", ".-.."); 
      morseCode.Add("m", "--"); 
      morseCode.Add("n", ".-"); 
      morseCode.Add("o", "---"); 
      morseCode.Add("p", ".--."); 
      morseCode.Add("q", "--.-"); 
      morseCode.Add("r", ".-."); 
      morseCode.Add("s", "..."); 
      morseCode.Add("t", "-"); 
      morseCode.Add("u", "..-"); 
      morseCode.Add("v", "...-"); 
      morseCode.Add("w", ".--"); 
      morseCode.Add("x", "-..-"); 
      morseCode.Add("y", "-.--"); 
      morseCode.Add("z", "--.."); 
      morseCode.Add("1", ".----"); 
      morseCode.Add("2", "..---"); 
      morseCode.Add("3", "...--"); 
      morseCode.Add("4", "....-"); 
      morseCode.Add("5", "....."); 
      morseCode.Add("6", "-...."); 
      morseCode.Add("7", "--..."); 
      morseCode.Add("8", "---.."); 
      morseCode.Add("9", "----."); 
      morseCode.Add("0", "-----"); 
     } 

     /// <summary> 
     /// Returns a translated word from English to Morse code. 
     /// </summary> 
     /// <param name="word">Word to be translated.</param> 
     /// <returns>A string of Morse code.</returns> 
     public string Translate(string word) 
     { 
      foreach (char letter in word) 
      { 
       StringBuilder morse = new StringBuilder(); 
       morse.Append(morseCode[letter.ToString()]); 
       return morse.ToString(); 
      } 

      return String.Empty; 
     } 
    } 
} 

私はこの週末に退屈し、モールスコードアプリを構築しています。

私はStringBuilderを使用しているので、すべての文字連結で新しい文字列オブジェクトを作成することはありませんが、十分に使用していないようです。

私が作ることができる目立つ改善はありますか？

Answer 1

は、私は、コードを改善する方法です。その中には以前に言及されているものもあれば、そうでないものもあります。

public static class MorseCodeConverter { 
    private static readonly Dictionary<char, string> mapping = new Dictionary<char, string>() { 
     { 'a', ".-" }, 
     { 'b', "-..." }, 
     { 'c', "-.-." }, 
     { 'd', "-.." }, 
     { 'e', "." }, 
     { 'f', "..-." }, 
     { 'g', "--." }, 
     { 'h', "...." }, 
     { 'i', ".." }, 
     { 'j', ".---" }, 
     { 'k', "-.-" }, 
     { 'l', ".-.." }, 
     { 'm', "--" }, 
     { 'n', "_." }, 
     { 'o', "---" }, 
     { 'p', ".--." }, 
     { 'q', "--.-" }, 
     { 'r', ".-." }, 
     { 's', "..." }, 
     { 't', "-" }, 
     { 'u', "..-" }, 
     { 'v', "...-" }, 
     { 'w', ".--" }, 
     { 'x', "-..-" }, 
     { 'y', "-.--" }, 
     { 'z', "--.." }, 
     { '0', "-----" }, 
     { '1', ".----" }, 
     { '2', "..---" }, 
     { '3', "...--" }, 
     { '4', "....-" }, 
     { '5', "....." }, 
     { '6', "-...." }, 
     { '7', "--..." }, 
     { '8', "---.." }, 
     { '9', "----." } 
    }; 

    public static string ToMorseCode(string word) { 
     word = word.ToLowerInvariant(); 

     StringBuilder morse = new StringBuilder(); 

     foreach (char letter in word) { 
      if (!mapping.ContainsKey(letter)) { 
       throw new ArgumentException("word contains a letter that cannot be converted", "word"); 
      } 

      morse.Append(mapping[letter]); 
     } 

     return morse.ToString(); 
    } 
} 

最初に静的であることです。これにより、変換メソッドを使用するためにオブジェクトをインスタンス化する必要がなくなります。また、辞書は一度だけ作成されることを意味します。

辞書の初期化構文を使用して、辞書の作成を少し洗練させ、 "mapping.Add ..."の繰り返しを避けました。

誰かがそれを読んだり修正したりする必要のないように私は辞書を秘密にしました（変数を公開する場合は、その名前はSentenceCaseの名前です）。

これは、読まれるように設計されたマッピングであり、変更されていないという考えを表現するために読んでいます。 readonlyは浅い不変性しかないことに注意してください。新しい辞書をマッピングに割り当てることはできませんが、すでに存在する辞書を変更することはできます。辞書は内部目的のためだけのものなので、深く不変にする努力をしていないのは大丈夫ですが、公にアクセスできれば、私はそれを深く不変にします。

辞書はまた、検索の前に文字を文字列に変換する必要がないように、charからstringまでです。

変換の進行方向を明確にするために、メソッドの名前をToMorseCodeに変更しました。 MorseCodeConverter.Translateは、平文からモースコードへ、またはモンスコードから平文への変換になる可能性があります。

単語は小文字に変換されます。モールス符号は実際にはケーシング感覚がないためです。

ループはマッピングが最初に存在するかどうかをチェックし、そうでない場合は例外をスローします。 Try/Getパターンに対して例外を投げることを選択しました。私の意見では、変換できない文字を例外的な環境に変換しようとしています。辞書検索のようなものではありません。

もちろん、文字列ビルダー変数を移動してループ外に戻すと、コードは文字列全体を変換するようになりました。

Answer 2

マップされていない文字を使用すると、例外が発生します。 TryGet()メソッドを使用できます。

Answer 3

ループの外でstringbuilder宣言を移動する - 管理されたヒープを泣かせている;）（また、あなたの翻訳がヒットした最初の文字のみを返すように見えるか、空の文字列 - あなたはしたくない全体の変換された単語を返す）

（編集 - ？）

public string Translate(string word) 
{ 
    StringBuilder morse = new StringBuilder(); 

    foreach (char letter in word) 
    { 
     string morseValue = ""; 
     if (morseCode.TryGetValue(letter.ToString(), out morseValue)) 
     { 
      morse.Append(morseValue); 
     } 
    } 

    return morse.ToString(); 
} 

0123）

public string Translate(string word) 
    { 
     StringBuilder morse = new StringBuilder(); 

     foreach (char letter in word) 
     { 
      if (morseCode.ContainsKey(letter.ToString())) 
      { 
       morse.Append(morseCode[letter.ToString()]); 
      } 
     } 

     return morse.ToString() ; 
    } 

を悪い文字を処理するために、あなたの辞書をチェック（そして、ここでdigEmAllの提案modは完全を期すためにもありますが含ま追加

ちょうど上記の変更とスタジオでこれをクランク、御馳走に動作します：

public static class program 
{ 
    public static void Main() 
    { 
     var c = new MorseCodeConverter(); 
     var x = c.Translate("hello"); 
    } 
} 

（集合Xに "" ...... - ... - ..--- ""）を

Answer 4

私はこのクラスを完全に静的にします。最初のアクセスでモールスコード辞書を構築し、静的メソッドで翻訳します。このクラスのいくつかのインスタンスを持つ必要はありません。おそらくこれをStringクラスの拡張メソッドとして書き直してください。

Answer 5

完全なコード。

namespace MorseCode 
    { 
     public class MorseCodeConverter 
     { 
      private static readonly Dictionary<char, string> letter2MorseCode; 

      static MorseCodeConverter() 
      { 
       letter2MorseCode = new Dictionary<char, string>(); 

       letter2MorseCode.Add('a', ".-"); 
       letter2MorseCode.Add('b', "-..."); 
       letter2MorseCode.Add('c', "-.-."); 
       letter2MorseCode.Add('d', "-.."); 
       ///.....   
      } 

      /// <summary> 
      /// Returns a translated word from English to Morse code. 
      /// </summary> 
      /// <param name="word">Word to be translated.</param> 
      /// <returns>A string of Morse code.</returns> 
      public bool TryGetMorseCode(string inputWord, out string outputMorseCode) 
      { 
       bool isValisString = true; 
       StringBuilder morse = new StringBuilder(); 
       foreach (char letter in inputWord) 
       { 
        string morseCodeLetter; 
        if (letter2MorseCode.TryGetValue(letter, out morseCodeLetter)) 
        { 
         morse.Append(morseCodeLetter); 
        } 
        else { 
         isValisString = false; 
        } 
        if (!isValisString) 
        { 
         break; 
        } 
       } 
       outputMorseCode = morse.ToString(); 
       return isValisString; 
      } 
     } 
    } 

変更：コードをモールスする文字のマッピングを格納する静的辞書を用い

1）。あなたはインスタンスごとにそれを必要としません。

2）非公開で、読み取り専用です。あなたは決してそれを変更するつもりはありません。

3）静的コンストラクタ内でマッピングを初期化します。これは、オブジェクトがクラスのインスタンス化される前に一度だけ呼び出されることを確認します。

4）GetMorseCodeの代わりにTryGetパターンを使用すると、どのテキストがユーザーによって渡されるかわからない場合があります。迷惑文字が見つかった場合は、解析されたモールス符号が有効でないことをユーザーに伝える必要があります（例外をスローすることもできます）。

（+）このクラスを静的にしません。私は非静的クラスでできることがたくさんあります。

Answer 6

は（Dictの< 文字、string>を注意してください）を置き換える文字列のループを使用します。私はなるだろう変化の

public class MorseCodeConverter 
{ 
    public Dictionary<char, string> morseCode; 

    public MorseCodeConverter() 
    { 
     LoadMorseCode(); 
    } 

    private void LoadMorseCode() 
    { 
     morseCode = new Dictionary<char, string>(); 

     morseCode.Add('a', ".-"); 
     morseCode.Add('b', "-..."); 
     morseCode.Add('c', "-.-."); 
     ... 
     morseCode.Add('0', "-----"); 
    } 

    public string Translate(string word) 
    { 
     word = word.ToLower(); 
     foreach (var x in morseCode.Keys) 
     { 
      word = word.Replace(x.ToString(), morseCode[x]); 
     } 
     return word; 
    } 
} 

Answer 7

一覧：

1. がシングルトンください。
2. マッピングを一度だけロードします。すべてのインスタンスではありません。
3. charをキーとして使用し、文字列として使用することはできません。
4. 翻訳時に小文字を使用します。
5. non-English languagesの複数のモールスコードを定義して複数の言語をサポートするオプションを追加します。すべてのものを読み込む代わりに、デフォルトの英語コードを読み込んで、何らかのバリデーションを行ってエンドユーザーに必要なだけ追加することができます。

このすべて言った、ここでは完全なコードです：

public class MorseCodeConverter 
{ 
    private static MorseCodeConverter instance; 
    private Dictionary<char, string> morseCodeMapping = new Dictionary<char, string>(); 

    public static MorseCodeConverter Instance { get { return instance; } } 

    static MorseCodeConverter() 
    { 
     instance = new MorseCodeConverter(); 
    } 

    private MorseCodeConverter() 
    { 
    LoadDefaultMapping(); 
    } 

    private void LoadDefaultMapping() 
    { 
     morseCodeMapping.Add('a', ".-"); 
     morseCodeMapping.Add('b', "-..."); 
     morseCodeMapping.Add('c', "-.-."); 
     morseCodeMapping.Add('d', "-.."); 
     morseCodeMapping.Add('e', "."); 
     morseCodeMapping.Add('f', "..-."); 
     morseCodeMapping.Add('g', "--."); 
     morseCodeMapping.Add('h', "...."); 
     morseCodeMapping.Add('i', ".."); 
     morseCodeMapping.Add('j', ".---"); 
     morseCodeMapping.Add('k', "-.-"); 
     morseCodeMapping.Add('l', ".-.."); 
     morseCodeMapping.Add('m', "--"); 
     morseCodeMapping.Add('n', ".-"); 
     morseCodeMapping.Add('o', "---"); 
     morseCodeMapping.Add('p', ".--."); 
     morseCodeMapping.Add('q', "--.-"); 
     morseCodeMapping.Add('r', ".-."); 
     morseCodeMapping.Add('s', "..."); 
     morseCodeMapping.Add('t', "-"); 
     morseCodeMapping.Add('u', "..-"); 
     morseCodeMapping.Add('v', "...-"); 
     morseCodeMapping.Add('w', ".--"); 
     morseCodeMapping.Add('x', "-..-"); 
     morseCodeMapping.Add('y', "-.--"); 
     morseCodeMapping.Add('z', "--.."); 
     morseCodeMapping.Add('1', ".----"); 
     morseCodeMapping.Add('2', "..---"); 
     morseCodeMapping.Add('3', "...--"); 
     morseCodeMapping.Add('4', "....-"); 
     morseCodeMapping.Add('5', "....."); 
     morseCodeMapping.Add('6', "-...."); 
     morseCodeMapping.Add('7', "--..."); 
     morseCodeMapping.Add('8', "---.."); 
     morseCodeMapping.Add('9', "----."); 
     morseCodeMapping.Add('0', "-----"); 
    } 

    public bool AddMorseMapping(char c, string morse) 
    { 
     if (string.IsNullOrEmpty(morse)) 
      throw new ArgumentException("Morse code can't be empty", "morse"); 
     int validCharsCount = morse.ToList().FindAll(ch => { return ch.Equals('-') || ch.Equals('.'); }).Count; 
     if (validCharsCount != morse.Length) 
      throw new ArgumentException("Invalid morse code, can contain only dash and period", "morse"); 
     char key = char.ToLower(c); 
     if (!morseCodeMapping.ContainsKey(key)) 
     { 
      morseCodeMapping.Add(key, morse); 
      return true; 
     } 
     else 
     { 
      return false; 
     } 
    } 

    /// <summary> 
    /// Returns a translated word from English to Morse code. 
    /// </summary> 
    /// <param name="word">Word to be translated.</param> 
    /// <returns>A string of Morse code.</returns> 
    public string Translate(string word) 
    { 
     StringBuilder morse = new StringBuilder(); 
     word.ToList().ForEach(letter => 
     { 
      char key = char.ToLower(letter); 
      if (morseCodeMapping.ContainsKey(key)) 
       morse.Append(morseCodeMapping[key]); 
     }); 
     return morse.ToString(); 
    } 
} 

は使用法：ここで

string morse = MorseCodeConverter.Instance.Translate(text);