Hey guys, as the title says, this is my first encryption program. Being extremely simple, I was just looking for advice on ways I could improve it. I just recently got back into programming(I tried to learn before but unfortunately lost interest while I was still learning the basics) and would appreciate any advice and/or criticism you could offer. Thanks.

#include <iostream>
#include <fstream>
#include <string>

using namespace std;


// encrypts clear text string
int encrypt(string x){

    int size = x.length();
    int ascii[size - 1];
    int change = 1;
    string hash;

    // converts the chars of string x into its ascii value
    for (int i = 0; i <= size - 1; i++)
        ascii[i] = x[i];

    // changing each ascii value by +change(dependent on number of times it loops)
    for (int i = 0; i <= size - 1; i++) {
        ascii[i]= ascii[i] + change;
        change++;
    }

    // converts the new ascii values into a hashed string
    for (int i = 0; i <= size - 1; i++)
        hash[i] = ascii[i];

    ofstream crypt;
    crypt.open("hash.txt",ofstream::app);

    cout << "Your hash is:";

    // displays hash on screen and saves it to hash.txt
    for (int i = 0; i <= size - 1; i++) {
        cout << hash[i];
        crypt << hash[i];
    }

    crypt << endl;
    crypt.close();

    cout << endl;

    return 0;
}

// decrypts hashed string
int decrypt(string y){

    int ysize = y.length();
    int yascii[ysize - 1];
    int change = 1;
    string clear;

    for (int i = 0; i <= ysize - 1; i++)
        yascii[i] = y[i];

    for (int i = 0; i <= ysize - 1; i++) {
        yascii[i] = yascii[i] - change;
        change++;
    }

    for (int i = 0; i <= ysize - 1; i++)
        clear[i] = yascii[i];

    cout << "Your message is:";
    for (int i = 0; i <= ysize - 1; i++)
        cout << clear[i];

    cout << endl;

    return 0;
}

int main(){

    string tEncrypt, task, tDecrypt;

    do{
        cout << "Encrypt:Decrypt" << endl << "::>";
        getline(cin, task);
        if ((task == "Encrypt") || (task == "encrypt") || (task == "ENCRYPT"))
        {
            cout << "What do you want to encrypt?" << endl << "::>";
            getline(cin, tEncrypt);

            encrypt(tEncrypt);
        }
        else if ((task == "Decrypt") || (task == "decrypt") || (task == "DECRYPT"))
        {
            cout << "What is your hash?" << endl << "::>";
            getline(cin, tDecrypt);
            decrypt(tDecrypt);
        }
    }while(task != "exit");

    return 0;
}

Edited 3 Years Ago by Sci3nc3F1cti0n

int ascii[size - 1];

It should be of size 'size' not 'size-1' and its index will run from 0 to size-1.

hash[i] = ascii[i];

While you are using string class you would be better off using string library function to append the character to the string thus assuring that you are not accessing element out of bounds.
Something like
hash.append(1, ascii[i])

Moreover you are adding change to the ASCII value which may exceed 256. So, If i were you, I would rather
(ascii[i] + change)%256;

A little addition.

int encrypt(string x)

I would prefer it to be int encrypt(cont string& x) as you are not changing the input string. Reference would be advantageous if your string is long.

@nchy13: I had thought about the problem with the ASCII values, but I couldn't think of a solution, modulus never even crossed my mind :/
You are, however, wrong about "It should be of size 'size' not 'size-1' and its index will run from 0 to size-1." The length of the string is a countable number, so because arrays start at 0, the size needs to be length - 1.('char' is 4 characters so an array for that would need to go from 0 to 3.
I've also made the other two changes you reccomended. I really don't know much about the string library, so thanks for that. I'll look into it.
Thank you :)

What are you saying man!!
Let string x={'1', '2', '3', '4', '5'}.
x.length=5.
You are declaring array int ascii[4]
How would you be able to store 5 values in ascii array of length 4, I can't think of.

Moreover there is abig problem with your code at line 12 which I earlier skipped.
It should be int *ascii = new int[size]
You should use new for dynamic allocation of memory. Though you can get away with it due to compiler optimisation, it is not advised in standards.

I think You are mixing up string with null-terminated char string. The length which is returned by x.length() is actual number of characters stored in the string. There is no role of '\0' here.

I'm sorry, but you're wrong.
Let string x ={'1', '2', '3', '4', '5'}.
x.length() = 5
If ascii[x.length() - 1], then it would declare ascii[4]. As you said above.
However, that means it has 5 elements because it starts from 0.
ascii[0,1,2,3,4]. I'm not mixing up anything, this is right.
Haha, I hate to say it but I don't know what you mean by

Moreover there is abig problem with your code at line 12 which I earlier skipped.
It should be int *ascii = new int[size]
You should use new for dynamic allocation of memory. Though you can get away with it due to compiler optimisation, it is not advised in standards.

Do you think you could explain? I would be very grateful.

What I mean is in int ascii[size] size should be a const variable. But as you can see size is an int variable, so it will not suffice. In C++11 size can be a constexpr as well.

Now, more important question what would one do if one doesn't have size as constant value. Here is where dynamic memory allocation comnes into picture.
Its syntax is as described above
int *ascii = new int[size].
It allocates memory equivalent of 5 int i.e, 5 bytes from heap for your purpose.

Coming back to previous discussion, in declaration of ascii[4], its index will run from 0 to 3 and not from 0 to 4 as far as I am concerned. Because according to me what const expression within square brackets indicate is the size of array that is 4 bytes.

Coming back to previous discussion, in declaration of ascii[4], its index will run from 0 to 3 and not from 0 to 4 as far as I am concerned. Because according to me what const expression within square brackets indicate is the size of array that is 4 bytes.

Oh wow, I feel like an idiot. For some reason I was thinking the decleration told what number it went to. I'm very sorry about that. You're definitely right. But the program still runs perfectly which now seems impossible since it should be leaving off a character. Any idea why it still works?

Oh okay, I've changed it. Thank you for your explanation and patience in dealing with me, I really appreciate it! :)

After I changed int ascii[size] to int *ascii = new int[size] the program started crashing after I typed what I wanted it to encrypt.
Segmentation fault (core dumped)
Process returned 139 (0x8B)

It happens sometimes. I also thought at first that I was wrong because of your surety but then I saw that it was string and not null terminated char string.

You were lucky that it was working. See, your ascii array had size size-1 but you were still accessing ascii[size-1] by running indexes from 0 to size-1 which would have been appropriate if size of ascii were to be size.
See, unlike Java C++ makes no attempt to restrict user from accesing index which is out of bounds.
You were trying to access ascii[size-1] whose address is ascii[size-2](last element of declared array) + 4 which you shouldn't had but C++ doesn't mind. It wouldn't had worked if some other variable used in your code resided at memory ascii[size-1]. So, everything worked fine in your case but this is only one of the possible effects of unexpected behaviour (UB).

Back to the whole size - 1 thing. Ever since I changed it to size, I've had extra characters appearing at the end of my hashed strings(ex. hello = igopt, the '' appears as an 'ACK' surrounded by black)

Edit: nevermind, I made the mistake of also changing the 'size - 1' in the for loops, which added an extra character.

Edited 3 Years Ago by Sci3nc3F1cti0n

Your for loops should be for (int i = 0; i < size; i++) instead of for (int i = 0; i <= size - 1; i++)

It might be common to find size<0 but syntactically it isn't different.
Edit:*semantically

Edited 3 Years Ago by nchy13

This question has already been answered. Start a new discussion instead.