Morse Code Converter in C

Question

I'm learning C so I thought to apply it to miniproject on reddit. The smorse function takes in a string and spits out some morse code. It works if you supply the function with only lowercase letters. I feel awkward making a first for loop in my smorse function to calculate the size of buffer I need to malloc and then making a second for loop to add the characters.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

char **master = NULL;

char* smorse(char *str) {
    int i;
    char *ele;
    int end = strlen(str);
    char *output = "";
    int totalSize = 0;

    for (i = 0; i < end; i++) {
        ele = master[(*str) - 97];
        totalSize += sizeof(ele) * sizeof(char);
        str++;
    }

    output = malloc(totalSize);
    str -= end;

    for (i = 0; i < end; i++) {
        ele = master[(*str) - 97];
        strcat(output, ele);
        str++;
    }

    return output;
}

int main(int argc, char *argv[]) {
    int i;
    char key[] = ".- -... -.-. -.. . ..-. --. .... .. .--- -.- .-.. -- -. --- .--. --.- .-. ... - ..- ...- .-- -..- -.-- --..";
    master = (char **)malloc(26 * sizeof(char *));
    char *ele = strtok(key, " ");

    for (i = 0; i < 26; i++) {
        if (i != 0)
            ele = strtok(NULL, " ");
        master[i] = (char *)malloc(sizeof(ele) * sizeof(char));
        master[i] = ele;
    }

    printf("%s\n", smorse("sos"));
    printf("%s\n", smorse("daily"));
    printf("%s\n", smorse("programmer"));
    printf("%s\n", smorse("bits"));
    printf("%s\n", smorse("three"));

    free(master);
    return 0;
}

Answer 1

regarding:

int main(int argc, char *argv[]) { 

Since the parameters are not used, the compiler will output two messages about unused parameters. Strongly suggest using the signature:

int main( void ) 

regarding:

master[i] = (char *)malloc(sizeof(ele) * sizeof(char));

1) the returned type is void* which can be assigned to any pointer. Casting just clutters the code, making it more difficult to understand, debug. etc. Suggest removing that cast.

2) The expression: sizeof( char ) is defined in the C standard as 1. Multiplying by 1 has no effect.

Suggest removing that expression.

3) always check (!=NULL) the returned value to assure the operation was successful. If not successful, call

perror( "malloc failed" ); 

to output both your error message 'malloc failed' and the text reason the system thinks the error occurred to stderr.

regarding:

int end = strlen(str); 

The function: strlen() returns a size_t which is an unsigned long int. This will result in the compiler outputting a message about this (error prone) implicit conversion. Suggest:

size_t end = strlen( str );

regarding:

totalSize += sizeof(ele) * sizeof(char); 

the variable ele is a pointer, so sizeof( ele ) results in the size of a pointer. Depending on the underlying hardware architecture the resulting value will be 4 or 8

The array master[] can be easily directly coded, so no need to waste time, code, CPU cycles developing that array. Suggest:

char **master[] =
{
    ".-",
    "-...",
   etc.
};

Then in smorse() a loop similar to:

for( size_t i = 0; str[i]; i++ )
{ 
    printf( "%s ", master[ tolower( str[i] ) ] );
}

puts( "" );

where the tolower() is from the header file: ctype.h

in main(), these statements:

printf("%s\n", smorse("sos"));
printf("%s\n", smorse("daily"));
printf("%s\n", smorse("programmer"));
printf("%s\n", smorse("bits"));
printf("%s\n", smorse("three"));

can be replaced with:

smorse( "sos" );
smorse( "daily" );
etc.

Answer 2

Memory leak

Consider this (I removed the pointless cast and identity multiplication):

    master[i] = malloc(sizeof ele);
    master[i] = ele;

Here, we allocate memory (if malloc() succeeds), but then immediately overwrite our one and only pointer to it, so we're unable to ever free it. The first assignment should just be removed, leaving only master[i] = ele.

Another memory leak:

printf("%s\n", smorse("sos"));

The smorse() function returns a pointer to allocated memory, which we need to release when we've finished using it:

const char *m = smorse("sos");
puts(m);
free(m);

(The puts() call is exactly equivalent to using printf("%s\n", m), but more readable and possible more efficient.)

Character coding assumptions

The magic value 97 in a couple of places suggests that we're assuming that characters are encoded as ASCII values. That's not a portable assumption: C permits a wide range of encodings. Notably, EBCDIC systems do not have letters in the same positions as ASCII (or even as a contiguous block, so replacing with 'a' doesn't solve the problem).

Also, there's no checking that the input to the function contains only characters that we can handle. If we pass anything outside of the expected range, then we index master outside its bounds, which is Undefined Behaviour in C. That means that the program may do anything - if we're lucky, it will simply crash, helping us to identify the problem, but there's no guarantee of that.

We ought to be converting more than just letters - digits have a standard Morse representation, as do certain other useful symbols.

Value conversions

It seems that this has been compiled with a very low level of warnings enabled. Building with my usual compiler flags results in over a dozen warnings, most about suspect conversions.

To pick a couple of examples:

int end = strlen(str);

strlen() returns a size_t, which is of different signedness to int, and very likely to be bigger, so there's a risk of undefined behaviour here.

char *output = "";

It's dangerous to point a modifiable pointer at constant data (a string literal is of type char const*).

printf("%s\n", smorse("sos"));

Passing a string literal loses constness here - smorse() ought to accept pointer to const instead.

Answer 3

I see some things that may help you improve your program.

Omit unused variables

Because argc and argv are unused, you could use the alternative form of main:

int main()

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. In this case master can easily be eliminated and I'll show below.

Minimize the scope of variables

Somewhat related to eliminating global variables is the idea that variables should be defined in the minimum practical scope. In this case, for instance, key doesn't really belong in main but should instead be in smorse as I'll describe in the next suggestion.

Use a better data structure

Right now, key[] is a poorly named data structure which is then post-processed at runtime to create the data structure that's actually used. Better would be to simply use a better data structure in the first place. Here's how I'd do it:

static const char* morse[] = {".-", "-...", "-.-.", "-..", ".", "..-.", "--.", "....", "..", ".---", "-.-", ".-..", "--", "-.", "---", ".--.", "--.-", ".-.", "...", "-", "..-", "...-", ".--", "-..-", "-.--", "--.."};

Fix the bug

Right now, output is uninitialized when the call to strcat is made, making this undefined behavior. You don't want that! Instead, initialize output like this:

output[0] = '\0';

Check for NULL pointers

If the call to malloc fails, the program will dereference a NULL pointer which is undefined behavior. Instead, explicitly check for a NULL pointer. You may also want to check to make sure that smorse has not been passed a NULL pointer.

Don't leak memory

Right now, because the pointer that smorse returns is only passed to printf, it can't be freed, which means that the program leaks memory. I'd change main to look like this:

int main() {
    const char *words[] = { "sos", "daily", "programmmer", "bits", "three", NULL };
    for (const char **word = words; *word; ++word) {
        char *morse = smorse(*word);
        puts(morse);
        free(morse);
    }
}

Eliminate "magic numbers"

There are a few numbers in the code, such as 26 and 97 that have a specific meaning in their particular context. By using named constants such as LetterCount or the character constant 'a', the program becomes easier to read and maintain.

Use const where practical

The keys variable should never be changed, and so it should be declared const. Further, it would be better to have the argument to smorse also be a const char * to indicate to the caller that the passed word is not changed.

Perform input sanitation

The smorse function is not particularly robust for general user input. For instance, if the user passes the string "Z93!", this code doesn't catch the fact that these letters are out of range and attempts to address an array with a negative index value. On my machine, this causes a segfault and a program crash.

Create a test function

Since you have some known inputs and expected outputs, it would make sense to use those to create some test vectors that could be used by a test function.

Answer 4

main()

It looks like this line it trying to allocate some memory to store one of the morse code strings (e.g., "-...").

master[i] = (char *)malloc(sizeof(ele) * sizeof(char));

However, ele is a char* and sizeof(char) is 1. So, this allocates memory for one char*, not enough to store a string of up to 4 char plus a terminating NULL. To get the length of the string pointed to by ele use strlen().

A pointer to the allocated memory is then stored in master[i]. But, the very next line:

master[i] = ele;

overwrites that pointer with the pointer returned by strtok(). The malloced memory is goes unused and un-freed. And is in fact not needed.

This loop could be written like:

int i = 0;
for (char *ele = strtok(key, " "); ele; ele = strtok(NULL, " ")) {
    master[i++] = ele;
}

Or better yet, just hard code master, like user3629249 said.

smorse() returns a pointer to malloc'd memory that is never freed.

smorse()

For proper morse code, you need a short space, or break, between letters and a slightly longer one between words. The inter-letter space can be a space at the end of each master code.

The longest morse code string is 5 characters (including a space at the end of each code), so you could just do:

output = (char *)malloc(5 * strlen(str))

In this loop, i is never used:

for (i = 0; i < end; i++) {
    ele = master[(*str) - 97];
    strcat(output, ele);
    str++;
}

It could be coded like:

for (char *c = str; *c; c++) {
    ele = master[(*c) - 97];
    strcat(output, ele);
}