Где я могу улучшить этот код?

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 ...».

Я сделал словарь закрытым, так как никто больше не должен его читать или модифицировать (n.b., если вы делаете переменные общедоступными, соглашение заключается в SentenceCase их имени).

Я сделал это только для чтения, чтобы выразить некоторые мысли, что это сопоставление, предназначенное для чтения, а не для изменения. Обратите внимание, что readonly является только неглубокой неизменностью - вы не можете назначить новый словарь для сопоставления, но вы можете изменить словарь, который уже существует. Поскольку словарь предназначен только для внутренних целей, не стоит прилагать усилий к тому, чтобы сделать его глубоко неизменным, но если бы он был общедоступным, я бы сделал его глубоко неизменным.

Словарь также относится к символу char, чтобы избежать необходимости преобразования символов в строки перед поиском.

Я изменил имя метода на ToMorseCode, чтобы уточнить, в каком направлении происходит преобразование. MorseCodeConverter.Translate может перейти от открытого текста к morsecode или от morsecode до открытого текста, это трудно сказать.

Слово сначала преобразуется в нижний регистр, так как код Морзе действительно не имеет смысла в корпусе.

Петля проверяет, существует ли первое сопоставление, и выбрасывает исключение, если нет. Я выбрал исключение из шаблона Try/Get, потому что, по-моему, попытка конвертировать символы, которые не могут быть преобразованы, должна быть исключительным обстоятельством, а не тем, что можно было бы ожидать, как поиск в словаре.

И, конечно же, перемещая переменную строкового строба и возвращаясь за пределы цикла, код теперь переводит всю строку.

Answer 2

Использование букв, которые не отображаются, приводит к исключению. Возможно, вы захотите использовать метод TryGet().

Answer 3

Переместить декларацию строковой надстройки за пределы цикла - вы делаете крик управляемой кучи;) (Также - похоже, что ваш перевод будет возвращать только первый символ, который он ударил, или пустую строку - не хотите ли вы вернуть все преобразованное слово)

(Edit - добавил Содержит проверить словарь обрабатывать недопустимые символы)

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 как хорошо для полноты)

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

Просто коленчатый это в студии с изменениями выше, работает лакомство:

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

(Наборы х в "" ......-...-..--- "")

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) Используя шаблон TryGet вместо GetMorseCode, вы никогда не сможете узнать, какой текст передан пользователем. в случае обнаружения какого-либо мусорного символа вы должны сообщить пользователю, что проанализированный код Морзе недействителен (вы также можете выбрать исключение).

(+) Я не стану этим классом статическим. Я так много могу сделать с нестационарным классом.

Answer 6

Используйте заменить петлю строку (обратите внимание на Dict < символ, строка>):

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. Сделать Singleton.
2. Загрузите отображение только один раз, а не во всех случаях.
3. Используйте символ как ключ, не имея смысла использовать его как строку.
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);