7
\$\begingroup\$

I have a Morse Code translator, which can translate text to Morse code and vice-versa. Note that for the two files that the first three lines are generated by the Code::Blocks IDE.

translator.h

#ifndef SPLIT_H_INCLUDED
#define SPLIT_H_INCLUDED
#endif // SPLIT_H_INCLUDED

#include <iostream>
#include <map>
#include <string>
#include "split.h"

std::string SPACE (" ");
std::map<char, std::string> charMap;
std::map<std::string, char> morseMap;
bool hasInit = false;

void init()
{
    charMap['a'] = ".-";
    charMap['b'] = "-...";
    charMap['c'] = "-.-.";
    charMap['d'] = "-..";
    charMap['e'] = ".";
    charMap['f'] = "..-.";
    charMap['g'] = "--.";
    charMap['h'] = "....";
    charMap['i'] = "..";
    charMap['j'] = ".---";
    charMap['k'] = "-.-";
    charMap['l'] = ".-..";
    charMap['m'] = "--";
    charMap['n'] = "-.";
    charMap['o'] = "---";
    charMap['p'] = ".--.";
    charMap['q'] = "--.-";
    charMap['r'] = ".-.";
    charMap['s'] = "...";
    charMap['t'] = "-";
    charMap['u'] = "..-";
    charMap['v'] = "...-";
    charMap['w'] = ".--";
    charMap['x'] = "-..-";
    charMap['y'] = "-.--";
    charMap['z'] = "--..";
    charMap['0'] = "-----";
    charMap['1'] = ".----";
    charMap['2'] = "..---";
    charMap['3'] = "...--";
    charMap['4'] = "....-";
    charMap['5'] = ".....";
    charMap['6'] = "-....";
    charMap['7'] = "--...";
    charMap['8'] = "---..";
    charMap['9'] = "----.";
    charMap['.'] = ".-.-.-";
    charMap[','] = "--..--";
    charMap['?'] = "..--..";
    charMap[' '] = "/";
    for (std::map<char, std::string>::iterator i = charMap.begin(); i != charMap.end(); i++) {
        morseMap[i -> second] = i -> first;
    }
}

std::string translateToMorse(std::string plainText)
{
    if (!hasInit) {
        init();
    }
    std::string result;
    for (int i = 0; plainText[i]; i++) {
        result.append(charMap[plainText[i]]);
        result.append(SPACE);
    }
    return result;
}

std::string translateFromMorse(std::string morseText)
{
    if (!hasInit) {
        init();
    }
    std::string result;
    std::vector<std::string> morse = split(morseText, SPACE[0]);
    for (std::vector<std::string>::iterator i = morse.begin(); i != morse.end(); i++) {
        result.push_back(morseMap[*i]);
    }
    return result;
}

split.h

#ifndef SPLIT_H_INCLUDED
#define SPLIT_H_INCLUDED
#endif // SPLIT_H_INCLUDED

#include <string>
#include <sstream>
#include <vector>

std::vector<std::string> &split(const std::string &s, char delim, std::vector<std::string> &elems) {
    std::stringstream ss(s);
    std::string item;
    while (std::getline(ss, item, delim)) {
        elems.push_back(item);
    }
    return elems;
}


std::vector<std::string> split(const std::string &s, char delim) {
    std::vector<std::string> elems;
    split(s, delim, elems);
    return elems;
}

Concerns:

  • Are the three generated lines necessary and are they used correctly?
  • Is there a way to simplify the code?
\$\endgroup\$
13
\$\begingroup\$

I see a number of things that may help you improve your code.

Headers should not contain code

Header files in C++ should generally only contain other headers, definitions and structure and class definitions. They should not contain code implementations, so neither of these .h files should really be .h files. Instead they should both be .cpp files because they contain code implementations or they should be split into separate declaration/implementation (.h/.cpp) pieces.

Use objects

The init() function seems to me that it would be better implemented as an object constructor and then the individual functions could be static member functions. Here's another way to do Morse conversion.

Eliminate global variables

There is really no reason here to have global variables. Instead, these should be wrapped into an object, or at least a namespace.

Use const where practical

The SPACE variable should be const as should be the arguments to the functions.

Pass complex objects by reference

The std::string arguments to either of the functions should be a reference to avoid an unnecessary copy. That is the function could instead be:

std::string translateToMorse(const std::string &plainText)

Consider the user

If someone very reasonably tries to translate a callsign such as W1AW they'll get back the string ".----" which is only the Morse code for '1'. The problem is that the code only accepts lowercase characters and neither throws an error nor gives any indication that the uppercase characters were simply ignored. That's not very helpful to the user. Consider instead throwing an exception. Simply ignoring characters is unlikely to be a desirable behavior in any case.

Eliminate split

The one place that split is used is in the translateFromMorse routine, but it's really not needed there. Instead you could introduce new functions that would translate a word at a time and use std::stringstream to extract a word at a time for translation.

\$\endgroup\$
7
\$\begingroup\$

Regarding your concerns:

Is the three generated lines necessary?

They're called header (or #include) guards, and you can read about them here. They're used to avoid indirectly including copies of the same shared header code.

So, your code should look like this:

#ifndef SPLIT_H_INCLUDED
#define SPLIT_H_INCLUDED
// Put your translator.h code here
#endif // SPLIT_H_INCLUDED
\$\endgroup\$
  • 1
    \$\begingroup\$ You're welcome. You could take a look at this implementation here to get some ideas for simplifying your code. codereview.stackexchange.com/questions/71228/morse-code-emitter \$\endgroup\$ – user3490458 Nov 14 '15 at 19:18
  • \$\begingroup\$ And you don't want to have #ifndef SPLIT_H_INCLUDED in multiple files or only one of the files will get compiled. I prefer #pragma once instead of #ifndef FILENAME for that reason. \$\endgroup\$ – Jack Deeth Aug 21 '17 at 15:09
5
\$\begingroup\$

As an ex Navy telegraphist, I can tell you that you are missing some characters commonly used in Morse. For example, to identify an error the code is 8 x e (eeeeeeee), to start and end the text of a message bt sent as a single character, to terminate a transmission ar sent as a single character plus others.

You may also want to take into account different transmission speeds. 12 words a minute broadcast sounds a whole lot different to a manual 30 words a minute from a professional.

You will have to teach it what a blank is, the space between words. An accurate example with translation can be seen in the Dam Busters film towards the end when the raid is happening.

\$\endgroup\$
  • \$\begingroup\$ You make several good points, especially about the prosigns. My Morse technique was never that good, but I've long been impressed by those who can pound out 30 wpm or better with just a straight key! \$\endgroup\$ – Edward Nov 15 '15 at 1:28
  • \$\begingroup\$ Thanks Edward 30wpm at 98% was the passing out minimum in my day, loooong time ago \$\endgroup\$ – Dandman Nov 15 '15 at 1:33
  • \$\begingroup\$ We used BEEFEATER for rythm and speed \$\endgroup\$ – Dandman Nov 15 '15 at 1:34

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.