0

thanks for helping me out :)

When I am running my code I get a weird output, an intractable char that is being added randomly to my output. The weird thing is that that the char that is being added is changed with every run of the code! Super weird and would love someone to tell what I did to create this weird behavior because I can't seem to understand.

The pattern I tested it with - ZYXWVUTSRQPONMLKJIHGFEDCBA

I tested it with a simple input of the char - A.

Here is the code:


int main(int argc, char *argv[])
{
    for (int j = 0; j < strlen(argv[1]); j++) //checking whether there is a digit in the input
    {
        if (argc != 2 ) //if there is a digit or more than 1 argumant in the input
        {
            printf("Usage: %s key\n", argv[0]);
            return 1;
        }
        else if (isdigit(argv[1][j])) //check if the input is only alphabeatic
        {
            printf("Key must contain alphabeatic characters.\n");
            return 1;
        }
        else if (strlen(argv[1]) != 26) //check if the input is 26 chars
        {
            printf("Key must contain 26 characters.\n");
            return 1;
        }
        for (int k = j+1; k < strlen(argv[1]); k++) //check if the input has duplicated values
        {
            if (argv[1][j] == argv[1][k])
            {
                printf("No repeated chars.\n");
                return 1;
            }
        }
    }
        string plaintext = get_string("plaintext: ");
        char letter,final_letter;
        char ciphertext[strlen(plaintext)];
        for (int i = 0; i < strlen(plaintext); i++)
        {
            if ((plaintext[i] >= 'a' && plaintext[i] <= 'z') || (plaintext[i] >= 'A' && plaintext[i] <= 'Z')) //check if the char is a letter
            {
                //printf("letter to chiper is %c\n", plaintext[i]);
                int ciphered_letter_index = (int)plaintext[i]-65; //understand the alphabetic position of the letter
                if (ciphered_letter_index > 25) //if the letter is lowercase we need to reduce it to uppercase as its ascii number is lower
                {
                    ciphered_letter_index = ciphered_letter_index - 32; //between every ascii lowercases and upercase letter the diffrenece is 32
                }
                letter = argv[1][ciphered_letter_index]; // gives as the position of the same letter in the provided pattern
                if ((plaintext[i] >= 'a' && plaintext[i] <= 'z') && (letter >= 'A' && letter <= 'Z'))  //if the original input was in lower case we need to return it to its original form
                {
                    letter = (char)((int)letter + 32);
                }
                else if ((plaintext[i] >= 'A' && plaintext[i] <= 'Z') && (letter >= 'a' && letter <= 'z')) //if the Uppercase input was in lower case we need to return it to its original form
                {
                    letter = (char)((int)letter - 32);
                }
                ciphertext[i] = letter; //append the new letter in the ciphered text
            }
            else //if the char is not a letter example- space or a ","
            {
                letter = plaintext[i];
                ciphertext[i] = letter;
            }
        }
        printf("ciphertext: %s\n",ciphertext);
        return 0;
}
0

The stray chars are printed because there's no end of string marker in the ciphertext[] array. Also, ciphertext is too small by 1 to store the EOS marker, \0.

A couple of programming hints and tips: First, isalpha(), isupper() and islower() are your friends. So are toupper() and tolower() and all their related cousin functions. ;-)

Second, look at this code:

letter = (char)((int)letter - 32);

Let's think about what it does. Note that a char is also treated as a one-byte signed int. First, it calculates letter - 32. To be honest, I'm not sure if the intermediate result is cast as a char or an int. However, this code then casts it as an int, and then casts it as a char and assigns it to a char. A bit redundant, isn't it? It's not wrong, but is it efficient?

Now, let's look at a different version:

letter = letter - 32;  

This will do the calculation and immediately cast the result as a char (if it isn't already cast as a char.)

Even simpler, there's this:

letter -= 32;

This final version simply subtracts 32 and stores the result back in letter, as a char.

Finally, a suggestion. In programming, there are usually multiple ways to accomplish a task. Usually, though, it's best practice to break complex tasks down into more, simpler tasks, and to decouple those tasks from each other. In this case, the code above in the major IF code block handles both upper and lower case letters and kinda goes back and forth between them. The code is tightly coupled and interrelated. Wouldn't it make things simpler to write two blocks of code here, one for the upper case letters and another to handle the lower case letters? Of course this is an opinion. I'll leave it to you.

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)

1
  • Thanks for the tips :) Managed to make my code cleaner! Jul 11 '20 at 20:55

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .