Nested for and if loops in C

Question

I've been working through Harvard's CS50 course and I have a question regarding design & use of nested for and if loops. I've submitted a problem set and I'm now just conducting a "post-mortem" to see if I could have written it more efficiently. In particular, I have a function which takes 2 arguments: a user's piece of text, and a 26-letter cipher key. it then converts the plain text into cipher text by replacing each character with the corresponding cipher key value.

I was wondering if the way I have written it here would be considered poor design? Having layers of for and if loops seems very clunky to me. (In particular looping through my alphabet string for each character, then reassigning that character to the corresponding cipher key, seems convoluted?)

Thanks,

// function for converting plain text to cipher text
string substitution(string text, string cipher)
{
    string alphabet = ("abcdefghijklmnopqrstuvwxyz");
    
    
    // for each character:
    for (int i = 0, n = strlen(text); i < n; i++)
    {
        // check if character is in the alphabet:
        if ((text[i] >= 'a' && text[i] <= 'z') || (text[i] >= 'A' && text[i] <= 'Z'))
        {
            // find position in alphabet by index, then convert to same index in the cipher string
            for (int j = 0; j < 26; j++)
            {
                if (text[i] == alphabet[j])
                {
                    text[i] = cipher[j];
                    break;
                }
                if (text[i] + 32 == alphabet[j])
                {
                    text[i] = cipher[j] - 32;
                    break;
                }
            }
        }
    }
    return text;
}

Answer 1

If you are going to make assumptions about the character code ordering and relationships such as upper/lowercase conversion, then the alphabet array serves little purpose. Its only purpose can be to decouple the code from platform defined character set ordering.

In practice it is probably a fair bet that your assumptions are valid for any platform you are likely to encounter, but if you are going to define alphabet then you should perhaps be consistent and make the rest of the code independent of the platform character-set. To do that you should use the ctype.h header. The following code does that and also eliminates some other issues such as "magic numbers".

#include <string.h>
#include <ctype.h>
#include <stdbool.h>

// function for converting plain text to cipher text
string substitution(string text, string cipher)
{
    static const char alphabet[] = "abcdefghijklmnopqrstuvwxyz";
    static const size_t ALPHABET_LEN = sizeof(alphabet) / sizeof(*alphabet) ;
    
    // for each character:
    size_t text_len = strlen( text ) ;
    for( int i = 0; i < text_len; i++ )
    {
        // check if character is in the alphabet:
        if( isalpha( text[i] ) )
        {
            // find position in alphabet by index, then convert to same index in the cipher string
            for (int j = 0; j < ALPHABET_LEN; j++)
            {
                bool is_upper = isupper( text[i] ) ;
                if( !is_upper && text[i] == alphabet[j])
                {
                    text[i] = cipher[j];
                    break;
                }
                else if( is_upper && tolower(text[i]) == alphabet[j])
                {
                    text[i] = toupper( cipher[j] ) ;
                    break;
                }
            }
        }
    }
    
    return text;
}

It is perhaps a matter of opinion, but many coding standards allow break only as a switch-case delimiter. break and continue for aborting a loop can create poor and confusing structure and control flow and can make debugging more complex as the control flow can "jump over" break-points. To avoid the use of break:

        bool match = false ;
        for (int j = 0; !match && j < ALPHABET_LEN; j++)
        {
            bool is_upper = isupper( text[i] ) ;
            if( !is_upper && text[i] == alphabet[j])
            {
                text[i] = cipher[j];
                match = true ;
            }
            else if( is_upper && tolower(text[i]) == alphabet[j] )
            {
                text[i] = toupper( cipher[j] ) ;
                match = true ;
            }
        }

Answer 2

this should do the trick a bit easier

string substitution(string text, string cipher)
{    
    // for each character:
    for (int i = 0, n = strlen(text); i < n; i++)
    {
        // check if character is in the alphabet:
        if (text[i] >= 'a' && text[i] <= 'z') 
        {
            text[i] = cipher[text[i] - 97]; // index of 'a' in alphabet is 97
        }
        else if (text[i] >= 'A' && text[i] <= 'Z'))
        {
            text[i] = cipher[text[i] - 65] - 32; // index of 'A' in alphabet is 65
        }
    }
    return text;
}

Answer 3

You can compact the character search using strchr function, which does... the same thing you do. Not much efficiency gain in there.

To calculate the index you can use direct subtraction:

// check if character is in the alphabet:
if ((text[i] >= 'a' && text[i] <= 'z') {
    j = text[i]-'a';
    text[i] = cipher[j];
} else if ((text[i] >= 'A' && text[i] <= 'Z') {
    j = text[i]-'A';
    text[i] = cipher[j] + 32;
}

You could also pre-calculate a table of all 255 possible char values (zero excluded), and set the value to zero if the character must not be ciphered, the cipher value otherwise.

Then:

for (i = 0; text[i]; i++) {
    if (table[text[i]]) {
        text[i] = table[text[i]];
    }
}

This makes sense if you need to cipher long texts, of course, otherwise you spend more in calculating the table than in using it.

Also, since the branching is on many architectures more expensive than simply assigning, consider MikeCAT's excellent suggestion whereby the "non-cipher" characters are replaced by themselves, so that text[i] = table[text[i]] actually does nothing:

for (i = 0; text[i]; i++) {
    text[i] = table[text[i]];
}

Answer 4

You used this for checking if text[i] is an alphabet instead of isalpha(), so I assume you limit the target environment of your program to where character code for alphabets are continuous (like ASCII).

        if ((text[i] >= 'a' && text[i] <= 'z') || (text[i] >= 'A' && text[i] <= 'Z'))

Under this limitation, you can simply subtract 'a' or 'A' to get the indice of characters without using loops to find the character.

This means the inner loop part

            for (int j = 0; j < 26; j++)
            {
                if (text[i] == alphabet[j])
                {
                    text[i] = cipher[j];
                    break;
                }
                if (text[i] + 32 == alphabet[j])
                {
                    text[i] = cipher[j] - 32;
                    break;
                }
            }

can be written as

            if (text[i] >= 'a' && text[i] <= 'z')
            {
                text[i] = cipher[text[i] - 'a'];
            }
            else
            {
                text[i] = cipher[text[i] - 'A'] - 32;
            }