1

I'm looking to make a method like so (which encrypts a message using Caesar Cipher, entered by the user and displays it):

void encrypt(char *message, int shift);

My code:

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

char num(char c)
{
    const char upper_alph[26] = {'A','B','C','D','E','F','G','H','I','J','K','L','M','N','O','P','Q','R','S','T','U','V','W','X','Y','Z'};
    const char lower_alph[26] = {'a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z'};

    if(isupper(c)) {
        for(int i = 0; i < 26; i++)
            if(upper_alph[i] == c)
                return i;
    } else {
        for(int i = 0; i < 26; i++)
            if(lower_alph[i] == c)
                return i;
    }
    return 0;
}

void encrypt(char *message, int shift)
{
    int i = 0;
    const char upper_alph[26] = {'A','B','C','D','E','F','G','H','I','J','K','L','M','N','O','P','Q','R','S','T','U','V','W','X','Y','Z'};
    const char lower_alph[26] = {'a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z'};
    while(message[i] != NULL)
    {
        if(isalpha(message[i]))
        {
            if(isupper(message[i])) {
                printf("%c", upper_alph[(num(message[i])+shift)%26]);
            } else {
                printf("%c", lower_alph[(num(message[i])+shift)%26]);
            }
        } else {
            printf("%c", message[i]);
        }
            i++;
    }
}

#define OK       0
#define NO_INPUT 1
#define TOO_LONG 2

static int getLine (char *prmpt, char *buff, size_t sz) {
    int ch, extra;

    // Get line with buffer overrun protection.
    if (prmpt != NULL) {
        printf ("%s", prmpt);
        fflush (stdout);
    }
    if (fgets (buff, sz, stdin) == NULL)
        return NO_INPUT;

    // If it was too long, there'll be no newline. In that case, we flush
    // to end of line so that excess doesn't affect the next call.
    if (buff[strlen(buff)-1] != '\n') {
        extra = 0;
        while (((ch = getchar()) != '\n') && (ch != EOF))
            extra = 1;
        return (extra == 1) ? TOO_LONG : OK;
    }

    // Otherwise remove newline and give string back to caller.
    buff[strlen(buff)-1] = '\0';
    return OK;
}

int main()
{
    //reverse();
    //printf("\n\n");
    int rc;
    char mes[1024];
    int sh = 0;
    rc = getLine ("Enter message to be encrypted: ", mes, sizeof(mes));
    if (rc == NO_INPUT) {
        // Extra NL since my system doesn't output that on EOF.
        printf ("\nNo input\n");
        return 1;
    }
    if (rc == TOO_LONG) {
        printf ("Input too long [%s]\n", mes);
        return 1;
    }
    encrypt(mes, 1);
    fflush(stdin);
    getchar();
    return 0;
}

Thank you to anyone who helps or tries to help.

:)

EDIT: Made many corrections. Still not working :/

EDIT2: Made a lot more corrections. Getting an access violation @ "while(*message != '\0')"

EDIT3: Updated the code above to the working code. Thank you everyone for your help!

Lee
  • 37
  • 2
  • 9
  • 3
    Why doesn't it work? What is it doing that you weren't expecting? – Gian Mar 22 '13 at 08:13
  • 1
    Quick tip: `if (isupper(c)) { num = c - 'A'; } else if (islower(c)) { num = c - 'a'; } else { num = 0; }` – Corbin Mar 22 '13 at 08:16
  • OT: the dual-case loop in `num()` is entirely unneeded. You could just use `toupper(c)` and search one map (the upper case one in case that wasn't obvious). – WhozCraig Mar 22 '13 at 08:16
  • @Corbin works in theory, but it isn't portable. Try it [with this character set](http://en.wikipedia.org/wiki/EBCDIC). – WhozCraig Mar 22 '13 at 08:17
  • 1
    @WhozCraig The entire code isn't portable - try to encrypt "Ä"... – Thorsten Dittmar Mar 22 '13 at 08:18
  • 1
    Hmm. Why is this tagged C++? As far as I can see that’s pure C code. – Konrad Rudolph Mar 22 '13 at 08:18
  • @WhozCraig Damn, did I accidentally time travel back to the 50s again?! This has to stop happening! :D (In all seriousness though, if he wants portability, his application is going to get **a lot** more complicated than this little toy.) – Corbin Mar 22 '13 at 08:19
  • @Corbin 50s? Pfft. EBCDIC is *still* in use on AS/400's and OS/390s *today*. But I concur on the chance this would ever see portability. It wasn't for the OP's benefit I even mentioned it; it was for anyone else that ever read that comment and walked away assuming it would always work. The continuity itself isn't even guaranteed by the standard (unless you're writing digits-only (0..9). – WhozCraig Mar 22 '13 at 08:22
  • @WhozCraig Fair enough... I suppose I should have at least mentioned that it's implementation dependent. It's going to be the implementation 99.999999% of the time though :P. – Corbin Mar 22 '13 at 08:26
  • Let's just say the problem is restricted to just the 7 bit ASCII codes for a-z and A-Z. – Skizz Mar 22 '13 at 08:39
  • Post-edit comment: [Seems to work](https://ideone.com/ZN9Ibk). Note that [it won't work](https://ideone.com/GshLf4) if you have a space in your message for reasons explained in my answer (which also has a solution). – Bernhard Barker Mar 22 '13 at 08:40
  • On that note, [also seems to work](http://ideone.com/YOmc9s) – WhozCraig Mar 22 '13 at 08:43

4 Answers4

2

It doesn't work because you didn't allocate memory for mes:

char mes[512]; // Enough space!

Use std::string is easier:

string mes;
int sh = 0;
cout << "Enter message to be encrypted: " << endl;
getline(cin, mes);
cout << "Enter a shift amount (1-25): " << endl;
cin >> sh;
encrypt(mes, sh);

And change encrypt function to:

void encrypt(const string &message, int shift)

And keep your characters in range:

 upper_alph[(num(message[i])+shift)%26]
 lower_alph[(num(message[i])+shift)%26]
masoud
  • 55,379
  • 16
  • 141
  • 208
2

One problem is you never wrap-around. Consider if you are passed something like 'Z' or 'z' with any positive shift, then you will just increment outside of the array.

You need to do something like:

upper_alph[(num(message[i])+shift)%26]
    and
lower_alph[(num(message[i])+shift)%26]

You also need to allocate memory for mes:

char mes[1024];

I believe your scanf is also incorrect (c is a character, s is a string):

scanf("%s", mes);

Using %s will however only read until it gets white-space, a better option may be to read the entire line with getline().

Community
  • 1
  • 1
Bernhard Barker
  • 54,589
  • 14
  • 104
  • 138
2

You'll get an "index out of bounds" error on these lines:

        if(isupper(message[i])) {
            printf("%c", upper_alph[num(message[i])+shift]);
        } else {
            printf("%c", lower_alph[num(message[i])+shift]);
        }

You need to calculate the index in advance and make sure it is between 0 and 25:

int shiftedIndex = (num(message[i]) + shift) % 26;

You are aware of the fact that your code only works with English as input language?

Thorsten Dittmar
  • 55,956
  • 8
  • 91
  • 139
2

There is a fundamental problem here that the OP isn't understanding. And that is, to the computer, letters are just numbers. It us us humans that assign meaning to those numbers, and we can't even decide on which numbers mean what (see comments on question re ASCII, EBDIC and Unicode).

Here is a table showing how the ASCII standard maps the numbers to letters.

Notice that the character 'a' is 97, 'b' is 98, 'c' is 99 and so on. The uppercase characters start at 65 and go up from there. Note also that the letter 'a' and 'A' are on the same row! This means the bit patterns of the lower 5 bits for an upper case letter and a lower case letter are the same. Finally, as the computer only ever sees characters as numbers, it can do numeric operations on them:-

'd' - 'a' == 3
100 - 97

The second thing to note is that mathematically the Caeser cipher is just an addition with a modulo:-

encoded character = (plain text character + shift) mod 26

So now the code can written much more efficiently:-

void Encode (char *message, int shift)
{
  while (*message)
  {
    char c = *message;

    if (isalpha (c)) // check c is a letter
    {
      // get the letter index: this maps 'A' to 0, 'B' to 1, etc
      // it also maps 'a' to 32 (97 - 65), 'b' to 33, etc
      c -= 'A'; 

      // this is 32 for lower case characters and 0 for upper case
      char case_of_c = c & 32; 

      // map 'a' to 'A', 'b' to 'B'
      c &= 31; 

      // the caeser shift!
      c = (c + shift) % 26; 

      // restore the case of the letter
      c |= case_of_c; 

      // remap the character back into the ASCII value
      c += 'A'; 

      // save the result of the shift
      *message = c; 
    }

    ++message;
  }
}
Skizz
  • 69,698
  • 10
  • 71
  • 108
  • Thank you for all the information! Helps me out a lot. I guess I misunderstood a lot. My updated code is here: http://pastebin.com/aETAmihe -- I'm still having problems. – Lee Mar 22 '13 at 21:21
  • I caught a bunch of mistakes. Fixing them now. – Lee Mar 22 '13 at 22:32
  • Updated code: http://pastebin.com/NgYuEUb0 Getting an error on line 10 (Access Violation). – Lee Mar 22 '13 at 22:42
  • I went back and changed the code completely, according to Dukeling's answer above. Works perfectly! Will post code above. – Lee Mar 22 '13 at 23:40