4
\$\begingroup\$

I'm writing a Morse code translator for homework in C#. It takes an input from the user and returns the Morse code version of their input. I understand that this code may look horrible, so how could I improve it in terms of efficiency and readability?

Program.cs

using System;
using System.Collections.Generic;

namespace MorseCodeTranslator
{
    class Program
    {
        static Dictionary<char, string> translator;

        static void Main(string[] args)
        {
            InitialiseDictionary();
            getUserInput();
        }

        private static void InitialiseDictionary()
        {
            char dot = '.';
            char dash = '−';

            translator = new Dictionary<char, string>()
            {
                {'a', string.Concat(dot, dash)},
                {'b', string.Concat(dash, dot, dot, dot)},
                {'c', string.Concat(dash, dot, dash, dot)},
                {'d', string.Concat(dash, dot, dot)},
                {'e', dot.ToString()},
                {'f', string.Concat(dot, dot, dash, dot)},
                {'g', string.Concat(dash, dash, dot)},
                {'h', string.Concat(dot, dot, dot, dot)},
                {'i', string.Concat(dot, dot)},
                {'j', string.Concat(dot, dash, dash, dash)},
                {'k', string.Concat(dash, dot, dash)},
                {'l', string.Concat(dot, dash, dot, dot)},
                {'m', string.Concat(dash, dash)},
                {'n', string.Concat(dash, dot)},
                {'o', string.Concat(dash, dash, dash)},
                {'p', string.Concat(dot, dash, dash, dot)},
                {'q', string.Concat(dash, dash, dot, dash)},
                {'r', string.Concat(dot, dash, dot)},
                {'s', string.Concat(dot, dot, dot)},
                {'t', string.Concat(dash)},
                {'u', string.Concat(dot, dot, dash)},
                {'v', string.Concat(dot, dot, dot, dash)},
                {'w', string.Concat(dot, dash, dash)},
                {'x', string.Concat(dash, dot, dot, dash)},
                {'y', string.Concat(dash, dot, dash, dash)},
                {'z', string.Concat(dash, dash, dot, dot)},
                {'0', string.Concat(dash, dash, dash, dash, dash)},
                {'1', string.Concat(dot, dash, dash, dash, dash)},  
                {'2', string.Concat(dot, dot, dash, dash, dash)},
                {'3', string.Concat(dot, dot, dot, dash, dash)},
                {'4', string.Concat(dot, dot, dot, dot, dash)},
                {'5', string.Concat(dot, dot, dot, dot, dot)},
                {'6', string.Concat(dash, dot, dot, dot, dot)},
                {'7', string.Concat(dash, dash, dot, dot, dot)},
                {'8', string.Concat(dash, dash, dash, dot, dot)},
                {'9', string.Concat(dash, dash, dash, dash, dot)}
            };
        }

        public static void getUserInput()
        {
            string input;
            Console.WriteLine("What did you want to say?");
            input = Console.ReadLine();
            input = input.ToLower();
            Console.WriteLine("Your output is: " + translate(input));
            Console.WriteLine("Press enter to end.");
            Console.ReadLine();
        }

        private static string translate(string input)
        {
            System.Text.StringBuilder sb = new System.Text.StringBuilder();
            foreach(char character in input)
            {
                if(translator.ContainsKey(character))
                {
                    sb.Append(translator[character] + " ");
                } else if (character == ' ')
                {
                    sb.Append("/ ");
                } else 
                {
                    sb.Append(character + " ");
                }
            }
            return sb.ToString();
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ It's a bit weird to see the symbols called 'dot' and 'dash' when they normally are pronounced 'di' and 'dah', but that's language rather than code. \$\endgroup\$ – Pete Kirkham Mar 10 '16 at 13:48
  • \$\begingroup\$ Take a look at the StringBuilder.AppendFormat() method. In translate() the string concatenation is not complex but in the long run string formatters are cleaner, easier to read, and less error prone than + operation concatenation. \$\endgroup\$ – radarbob Mar 19 '16 at 4:22
  • 1
    \$\begingroup\$ translate() would be more robust if it accepted capital letters by ToLowering them before dictionary lookup, \$\endgroup\$ – radarbob Mar 19 '16 at 4:29
  • 1
    \$\begingroup\$ Why is ' ' not in the dictionary? \$\endgroup\$ – radarbob Mar 19 '16 at 4:32
6
\$\begingroup\$

Is translator really a translator? IMO a better name could be dictionary.

I also don't understand an idea to create variables like dot and dash. {'a', ".-"} would be much clearer.

Don't use misleading function names. The function getUserInput does much more than getting something from user! It also translates the text and prints it. So, in Main, I'd write:

input = getUserInput();
morse = translate(input);
PrintOnScreen(morse);

Be consistent in naming: now some functions have names starting with capital letter, some not.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer! Now making the changes. Is there a better data structure to use instead of a dictionary? I'm not familiar with all the different storage methods with C#. \$\endgroup\$ – 3lliot Mar 10 '16 at 17:35
  • \$\begingroup\$ I think Piotr is hinting at the NAME of your variable, I think a proper compromise would be translatorDictionary. \$\endgroup\$ – Chris Hayes Mar 11 '16 at 7:32
  • \$\begingroup\$ dictionary is a lousy name. It IS a dictionary. How about something with the word encoder in it? \$\endgroup\$ – radarbob Mar 19 '16 at 4:26
  • \$\begingroup\$ Using a variable for the dot and dash is actually a good thing, as this can allow easier modification of the programme to be configured by the user in the future. (I may want to use different characters for them.) \$\endgroup\$ – 410_Gone Aug 7 '16 at 5:29
4
\$\begingroup\$

The answers are perfect. But I want to improve. I changed your code little bit.
In any case a developper wants to use it; Here is what I did:

  • Change dictionary
  • Specifiy private/public access modifiers
  • Take press enter to exit code outside of the "GetUserInput"
  • Change dictionary name as _morseAlphabetDictionary
  • Removing dash/dot local values.
  • Change initialise as initialize
  • Get user input now detects "empty inputs"
public class Program
{
    private static Dictionary<char, string> _morseAlphabetDictionary;

    static void Main()
    {
        InitializeDictionary();

        Console.WriteLine("What did you want to say?");
        string userInput = GetUserInput();
        Console.WriteLine("Morse alphabet output is: " + Translate(userInput));

        Console.WriteLine("[Press ANY KEY to exit]");
        Console.ReadLine();
    }

    private static void InitializeDictionary()
    {
        _morseAlphabetDictionary = 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', "----."}
                                   };
    }

    private static string GetUserInput()
    {
        string input = Console.ReadLine();

        if (! string.IsNullOrEmpty(input))
        {
            input = input.ToLower();
        }

        return input;
    }

    private static string Translate(string input)
    {
        StringBuilder stringBuilder = new StringBuilder();

        foreach (char character in input)
        {
            if (_morseAlphabetDictionary.ContainsKey(character))
            {
                stringBuilder.Append(_morseAlphabetDictionary[character] + " ");
            }
            else if (character == ' ')
            {
                stringBuilder.Append("/ ");
            }
            else
            {
                stringBuilder.Append(character + " ");
            }
        }

        return stringBuilder.ToString();
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Awesome, looks pretty similar to my code after the edits now. :) \$\endgroup\$ – 3lliot Mar 28 '16 at 11:16
  • \$\begingroup\$ I'd rather turn InitializeDictionary into a side-effect free function and use it as initializer for _morseAlphabetDictionary. Or just drop the function entirely, it doesn't really add anything. \$\endgroup\$ – CodesInChaos Mar 29 '16 at 13:24
2
\$\begingroup\$
  • Everything above is good input
  • The code I have does this in a WinForms application, but the principle of converting text to morse is the same.
  • You can do it many ways. I am showing a dictionary and a switch statement approach.
  • The dictionary is a good approach and gives good flexibility as others stated
  • The dot dash terminology is arbitrary and is fine. As stated above, it gives flexibility to use anything in place of it, ex: "AB ABB ABAB" etc.
  • The approach I took splits the message into separate strings everywhere there is a space and treats that as a word
    • (assuming there is a single space between each word in the message you want to convert).
  • Then you can iterate through each word and perform any word-specific operations you want.
  • When you are converting each word, you iterate through each character and you convert it to the morse equivalent and then append it to your "morse" string.
  • A stringbuilder would also work instead of morseCode += "something"
  • When you do morseCode += GetMorseForChar(character) you can use GetMorseForChar() or _morseAlphabetDictionary[character].
  • The latter gives you the "morse" value for the character key.

    static string Dot = "∙ ";
    static string Dash = "- ";
    private static Dictionary<char, string> _morseAlphabetDictionary = CreateMorseAlphabetDictionary();
    
    private static Dictionary<char, string> CreateMorseAlphabetDictionary()
    {
        return new Dictionary<char, string>()
        {
                { 'A', Dot + Dash },
                { 'B', Dash + Dot + Dot + Dot },
                { 'C', Dash + Dot + Dash + Dot },
                { 'D', Dash + Dot + Dot },
                { 'E', Dot },
                { 'F', Dot + Dot + Dash + Dot },
                { 'G', Dash + Dash + Dot },
                { 'H', Dot + Dot + Dot + Dot },
                { 'I', Dot + Dot },
                { 'J', Dot + Dash + Dash + Dash },
                { 'K', Dash + Dot + Dash },
                { 'L', Dot + Dash + Dot + Dot },
                { 'M', Dash + Dash },
                { 'N', Dash + Dot },
                { 'O', Dash + Dash + Dash },
                { 'P', Dot + Dash + Dash + Dot },
                { 'Q', Dash + Dash + Dot + Dash },
                { 'R', Dot + Dash + Dot },
                { 'S', Dot + Dot + Dot },
                { 'T', Dash },
                { 'U', Dot + Dot + Dash },
                { 'V', Dot + Dot + Dot + Dash },
                { 'W', Dot + Dash + Dash },
                { 'X', Dash + Dot + Dot + Dash },
                { 'Y', Dash + Dot + Dash + Dash },
                { 'Z', Dash + Dash + Dot + Dot },
                { ' ', "\n" },
                { '_', "\n" },
                { '0', Dash + Dash + Dash + Dash + Dash },
                { '1', Dot + Dash + Dash + Dash + Dash },
                { '2', Dot + Dot + Dash + Dash + Dash },
                { '3', Dot + Dot + Dot + Dash + Dash },
                { '4', Dot + Dot + Dot + Dot + Dash },
                { '5', Dot + Dot + Dot + Dot + Dot },
                { '6', Dash + Dot + Dot + Dot + Dot },
                { '7', Dash + Dash + Dot + Dot + Dot },
                { '8', Dash + Dash + Dash + Dot + Dot },
                { '9', Dash + Dash + Dash + Dash + Dot }
             };
          }
    
          private void calculateMorseButton_Click(object sender, EventArgs e)
          {
             string[] words = englishTxtBox.Text.ToUpper().Split(' ');
             string morseCode = ConvertWordsToMorse(words);
    
             MessageBox.Show(morseCode);
          }
    
          private string ConvertWordsToMorse(string[] words)
          {
             string morseCode = string.Empty;
             foreach (string word in words)
             {
                char[] chars = word.ToCharArray();
                morseCode += ConvertLettersToMorse(chars) + "\n";//$"{"",7}";
             }
             return morseCode;
          }
    
          private string ConvertLettersToMorse(char[] chars)
          {
             string morseCode = string.Empty;
             foreach (char character in chars)
             {
                morseCode += GetMorseForChar(character) + $"{"",3}";
             }
             return morseCode;
          }
    
          private string GetMorseForCharFromDictionary(char character)
          {
             return _morseAlphabetDictionary[character];
          }
    
          private string GetMorseForChar(char character)
          {
             string dot = "∙ ";
             string dash = "- ";
    
             switch (character)
             {
                case 'A': return dot + dash;
    
                case 'B': return dash + dot + dot + dot;
    
                case 'C': return dash + dot + dash + dot;
    
                case 'D': return dash + dot + dot;
    
                case 'E': return dot;
    
                case 'F': return dot + dot + dash + dot;
    
                case 'G': return dash + dash + dot;
    
                case 'H': return dot + dot + dot + dot;
    
                case 'I': return dot + dot;
    
                case 'J': return dot + dash + dash + dash;
    
                case 'K': return dash + dot + dash;
    
                case 'L': return dot + dash + dot + dot;
    
                case 'M': return dash + dash;
    
                case 'N': return dash + dot;
    
                case 'O': return dash + dash + dash;
    
                case 'P': return dot + dash + dash + dot;
    
                case 'Q': return dash + dash + dot + dash;
    
                case 'R': return dot + dash + dot;
    
                case 'S': return dot + dot + dot;
    
                case 'T': return dash;
    
                case 'U': return dot + dot + dash;
    
                case 'V': return dot + dot + dot + dash;
    
                case 'W': return dot + dash + dash;
    
                case 'X': return dash + dot + dot + dash;
    
                case 'Y': return dash + dot + dash + dash;
    
                case 'Z': return dash + dash + dot + dot;
    
                case ' ': return "\n";
    
                case '_': goto case ' ';
    
                case '0': return dash + dash + dash + dash + dash;
    
                case '1': return dot + dash + dash + dash + dash;
    
                case '2': return dot + dot + dash + dash + dash;
    
                case '3': return dot + dot + dot + dash + dash;
    
                case '4': return dot + dot + dot + dot + dash;
    
                case '5': return dot + dot + dot + dot + dot;
    
                case '6': return dash + dot + dot + dot + dot;
    
                case '7': return dash + dash + dot + dot + dot;
    
                case '8': return dash + dash + dash + dot + dot;
    
                case '9': return dash + dash + dash + dash + dot;
    
                default: return "";
             }
          }
    
\$\endgroup\$
0
\$\begingroup\$

using a dictionary is a great way to organize the language and even keep it open for extension.

Naming conventions are really important and not following the 'norm' for a language can be sacrilege to some programmers and for good reason. It allows programmers to understand at a glance the scope and purpose of the variable, function, class, etcetera.

C# has some common naming conventions, namely (this is not holy writ):

  • Pascal Case for method names, properties, classes: ie MyBigClass
  • Underscore plus camel case for private variables: ie _thisIsMyVariable

also see: https://msdn.microsoft.com/en-us/library/ff926074.aspx

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.