1

I tried the Caesar Problem. Everything is going well, except when I want to test it. It shows that my output and the test output are the same, but my code couldn't pass the test. Even though I checked the '\n' character, I couldn't find the problem in my code. is there anyone who faces a similar problem?

The Code

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

#define MAXSIZE 100

int plaintext(int k);
int rotate(char chara, int k);

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

    if (argc == 1 || argc > 2)
    {
        printf("Usage: %s key\n", argv[0]);
        return 1;
    }
    else
    {
        int check = isdigit(argv[1][0]);
        int checktwo = argv[1][1];
        if (check == 0)
        {
            printf("Usage: %s key\n", argv[0]);
            return 1;
        }
        else if (checktwo >= '\x41' && checktwo <= '\x7A')
        {
            printf("Usage: %s key\n", argv[0]);
            return 1;
        }
        else
        {

            k = atoi(argv[1]);
        }

        plaintext(k);
        return 0;
    }
}

int plaintext(int k)
{
    char text[MAXSIZE];
    int index = 0;
    char ch;
    int re;
    printf("plaintext:  ");
    while (index < MAXSIZE - 1)
    {
        scanf("%c", &ch);
        if (ch == '\n')
        {
            break;
        }
        else
        {
            text[index] = ch;
            index++;
        }
    }
    text[index] = '\0';

    int cap;
    int lenght = strlen(text);
    printf("ciphertext: ");
    for (int i = 0; i <= lenght; i++)
    {
        if (text[i] >= '\0' && text[i] <= '\64')
        {
            printf("%c", text[i]);
        }
        else if (text[i] >= '\123' && text[i] <= '\127')
        {
            printf("%c", text[i]);
        }
        else if (text[i] >= '\x41' && text[i] <= '\x5A')
        {
            cap = (int) text[i] + 32;
            re = rotate(cap, k);
            printf("%c", toupper(re));
        }
        else if (text[i] >= '\x61' && text[i] <= '\x74')
        {
            re = rotate(text[i], k);
            printf("%c", re);
        }
        else
        {
            printf("%c", text[i]);
        }
    }
    return 0;
}

int rotate(char chara, int k)
{
    int start = 96;
    int end = 122;
    int decrypt;
    int encrypt;
    encrypt = (int) chara + k % (start - end);
    decrypt = encrypt - k % (start - end);

    if (encrypt > end)
    {
        encrypt = ((encrypt - end) + 96);
        return encrypt;
    }

    return encrypt;
}


Check50 Result

enter image description here

1
  • thanks for adding code!
    – UpAndAdam
    Commented Jan 12 at 16:04

1 Answer 1

1

Actual issues

  • You are not printing a newline after the last character of the ciphertext

Minor things that are REALLY worth examining

  • Instead of returning 0 on the happy path in main, you should probably be returning the value that plaintext(k) returns? ( which for some reason is always 0, in which case why have it return anything? Make it void if you never return anything)

  • I would simplify your usage check and just say if (argc != 2)

  • If you are going to use multiple returns which you do I don't think it's really needed to put everything into an else block after your usage check since you return 1 if they hit that error. The only possibility is else so the nesting is not needed.

  • You don't need or use check variable other than for that single check so consider eliminating the variable and just performing the check directly in the if statement. And do this check before dealing with checktwo. and again you can embrace short circuiting that you won't continue and dont need to keep nesting but if you want to leave all the nesting that's fine too.

  • in plaintext you could combine some of your if statements together for example

if (text[i] >= '\0' && text[i] <= '\64')
{
    printf("%c", text[i]);
}
else if (text[i] >= '\123' && text[i] <= '\127')
{
    printf("%c", text[i]);
}

could be condensed into

if ((text[i] >= '\0' && text[i] <= '\64') || (text[i] >= '\123' && text[i] <= '\127'))
{
    printf("%c", text[i]);
}

but I would simplify the whole thing down much further:

if (text[i] >= '\x41' && text[i] <= '\x5A')
{
    cap = (int) text[i] + 32;
    re = rotate(cap, k);
    printf("%c", toupper(re));
}
else if (text[i] >= '\x61' && text[i] <= '\x74')
{
    re = rotate(text[i], k);
    printf("%c", re);
}
else
{
    printf("%c", text[i]);
}
  • given that you use toupper() for capitalizing, why not be consistent and use tolower() instead of + 32? consistency is generally a good thing.

  • consistency and constants It's also unclear to me why you would use 96 (or 97 see below) in rotate but used hex back in plaintext where you used '\x61' ( you could have just used 0x61 but thats another can of worms ). Either way pick a format and be consistent. Even better don't use magic numbers and set them to a constant with a descriptive name. One possible example could be const char VAL_a = 'a'; or const char VAL_a = 97; or const char VAL_a = 0x61; and you could also define VAL_z, VAL_A, VAL_Z, similarly as well as const char NUM_LETTERS = 26;. I would make all five of those variables global constants. This would clean up all of your code and make it much more readable, less error prone, and more consistent.

  • your rotation function is much more complicated then needed, contains extra variables, isn't consistent about the values it uses vs variables and is generally much more complicated then it needs to be.

    • decrypt variable is never used.
    • the core logic is more complicated than it needs to be and could all be done in one go. In order to perform the rotation efficiently you need to translate the letter from whatever it's actual value is to a number that represents how many letters it is above 'a'. This allowed you to do a proper modulo in one shot. So something like this ( notice how I changed the signature to return a char instead of an int to be consistent, you would have to change the type of re to be char. )
char rotate(char input, int rotation)
{
    int x = input - 'a'; //now x is how many characters above 'a', x is 0 for input 'a'
    int offset = (x + k) % 26; //adds the rotation and modulos the whole thing by 26 to get what the final value should be relative to 'a' = 0.
    char output = 'a' + offset; // add the offset to 'a' to get the output value
    return output;
}
  • In your original code your value for the module operand was (start-end) which is effectively a constant for you of (-26) but for some reason you are doing the math out every single time. that said -26 will be the same as 26 for modulo.
  • I'm not sure why you used start and end of 96 and 122 instead of 97 and 123, then you could have used start as your sentinel value. If you had done this in your parlance you could have said encrypt = (chara - start + k) % 26 + start for encrypt and that would have been it.
  • the only reason it is working is because at the end in your 'if' statement you do encrypt = (encrypt - end) + 96 which you should have at least wrote as encrypt = (encrypt - end) + start but this turns into encrypt = encrypt - 26 and is effectively one more round of the modulo applied safely that you could have performed all in one go... I didn't follow this initially from the combination of not using variables consistently for a value and using values for them that don't actually seem to make sense was incredibly confusing.
  • admittedly I came in with bias knowing the optimal solution so something that didnt follow that seemed strange to me and I prematurely assumed you had an error in rotate. There is nothing wrong about your rotate but I would REALLY REALLY suggest that you use start in place of 96 since you defined that to be the value of start, and I would strongly suggest using the values of 97 and 123 for start and end, making them constants, and making 26 a constant.

If you had posted the the textual output from check50 and the program instead of images I would have been able to quickly spot your one main issue. I can't see the images due to firewall from where I mostly participate from.

4
  • Thanks for the useful and most beneficial points regarding my code. I appreciate the time you took to reply. As per your suggestion, I refactored my code. 1. removed returned and made the void plaintext(int k) void 2. I made the conditional statements in the code inclusive and grouped them. 3. I applied consistency and avoided unnecessary memory usage. such `int k, decrypt, cap' 4. avoided recalculating and made CPU a little bit at ease. 6. The way you have written the rotate function is really minimal, and I like it. I will apply it.
    – Subhan SA
    Commented Jan 12 at 23:36
  • It says my output and test output are the same, but it still shows errors. Should I rewrite my entire code using some other technique and design? like putting all the ciphered characters into an array and looping through that array. But isn't it using memory locations that we shouldn't use? ##:( encrypts "a" as "b" using 1 as key ***Cause*** expected "ciphertext: b\...", not "ciphertext: b\..." ***Log*** running ./caesar 1... sending input a... checking for output "ciphertext: b\n"... ***Expected Output:*** ciphertext: b ***Actual Output:*** ciphertext: b
    – Subhan SA
    Commented Jan 12 at 23:37
  • But adding a new line after the void plaintext(int k) function in the main function, just like adding a new line after the loop inside the void plaintext(int k) function, didn't help me pass the test. Below is the output of check50 as text.
    – Subhan SA
    Commented Jan 12 at 23:37
  • without seeing your new code i can't tell... but it sounds like your print of newline is wrong based on the output. or you can't copy and paste very well.
    – UpAndAdam
    Commented Jan 15 at 3:47

You must log in to answer this question.

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