Encryption issue in c++

Question

I'm currently attempting to implement a substitution cipher that for some reason keeps crashing, the code is fairly straight forward but I keep running into problems I believe originate in the for loop or when I attempt to read in the data from a file.

cout << "Ener a key :";
cin >> key;

cin.ignore();
cout << endl << "Enter input file name: ";
getline(cin,fileIn);

inputfile.open(fileIn.c_str(), ios::in);


cout << endl << "Enter output file name: ";
getline(cin,fileOut);
outfile.open(fileOut.c_str(), ios::app);

cout << endl << "[E]ncryption or [D]ecryption? :";
cin >> EorD;


//Encryption
if (EorD == "E" || "e")
{
    while(!inputfile.eof()) // Reading in file data, while not end of file.
    {
        getline(inputfile,plainText);
    }

        for (int i = 0; i <= plainText.length(); i++)
        {
        char letter = plainText.at(i);
        int val = (int)letter; // getting ascii value of each letter.
        int EnVal = (val - 32) + key;
            if(EnVal > 95)
            {
                EnVal = (EnVal - 95) + 32;

            }
        char EnLetter = static_cast<char>(EnVal);
        outfile <<  EnLetter;

`.eof()` is never right. Any of the hundreds of duplicates on this site will explain why. — Kerrek SB, May 01 '13 at 13:36
@KerrekSB In this case, it's particularly bad, since he throws out all of the good input; in practice, if the file ends with a `'\n'`, the _only_ time he'll process `plainText` is after the `getline` fails, when the contents of `plainText` aren't valid. — James Kanze, May 01 '13 at 13:42
Also, the inner `for` loop is not only wrong, but would be better handled by `std::transform` (which wouldn't get it wrong). — James Kanze, May 01 '13 at 13:44
And you really shouldn't use `std::string::at` if you don't have a `try` block. The only reason to use it is to be able to do something else if you have a bounds error. Using `std::string::operator[]` is usually preferable. — James Kanze, May 01 '13 at 13:45
Would `while(inputfile.good())` make a better substitute for .eof? — , May 01 '13 at 13:58
No, see this question: [Testing stream.good() or !stream.eof() reads last line twice](http://stackoverflow.com/questions/4324441/testing-stream-good-or-stream-eof-reads-last-line-twice) — Blastfurnace, May 01 '13 at 14:02
possible duplicate of [Why doesn't my simple XOR encryption program translate the characters right, and why does it add more characters at the end?](http://stackoverflow.com/questions/5083612/why-doesnt-my-simple-xor-encryption-program-translate-the-characters-right-and) — Jerry Coffin, May 01 '13 at 14:08
What type is `EorD`? If it's not a `std::string` the comparison with `"E"` or `"e"` is unlikely to work. — Peter Wood, May 01 '13 at 14:12

score 2 · Answer 1 · answered May 01 '13 at 13:38

You are looping one index too far in the plainText string. Since it has length() entries and the first one is 0, the last index is length()-1. Try this:

for (int i = 0; i < plainText.length(); i++)

Otherwise plainText.at(i) will crash when i is too big.

4pie0 · Accepted Answer · 2013-05-01T14:58:25.210

2

change

for (int i = 0; i <= plainText.length(); i++)

to

for (int i = 0; i <= plainText.length()-1; i++)

because out of range. Even better use iterator.

also change this:

if (EorD == "E" || "e")

to

if (EorD == "E" || EorD == "e")

because former is always true.

as James Kanze pointed out, don't use std::string::at, you don't need it here, change it to std::string operator[] and my advice: additionally cover your code in a nice try{}catch(...){} block

you might consider something like this:

#include <vector>
#include <iterator>
#include <algorithm>

int key=100;
char op(char c){
    char letter = c;
        int val = (int)letter; // getting ascii value of each letter.
        int EnVal = (val - 32) + key;
            if(EnVal > 95)
            {
                EnVal = (EnVal - 95) + 32;

            }
        char EnLetter = static_cast<char>(EnVal);
        return EnLetter;
}
int main(){
    try{
       std::string s="piotrek";
       std::vector<char> vc_in(s.begin(),s.end());
       std::vector<char> vc_out;
       std::transform (vc_in.begin(), vc_in.end(), 
          std::back_inserter(vc_out), op); //this do the thing
       std::copy(vc_out.begin(), vc_out.end(),
          std::ostream_iterator<char> (std::cout,"_")); // to print
    }catch(std::exception& e){
       cout<<"exception: "<<e.what();
    }
    return OK;
}

edited May 01 '13 at 14:58

answered May 01 '13 at 13:46

4pie0

29,204
9
82
118

Wouldn't it be better to use iterators? They avoid this sort of error. – James Kanze May 01 '13 at 13:47
After making cf16's suggested changes, I still receive the error "This application has requested the run time to terminate in an unusual way". Any ideas? – May 01 '13 at 13:52
maybe uncaught exception causes std::terminate? – 4pie0 May 01 '13 at 13:52
do a try{}catch(...){cout<<"exception";} – 4pie0 May 01 '13 at 13:54
@cf16: Less than length should be fine. He is doing less than or equal though. – Victor Sand May 01 '13 at 13:57
Wouldn't it be better to say `i < plainText.length()`? – Peter Wood May 01 '13 at 14:14
@JohnnyDiamond08 see my edits and please update us if you have still issues – 4pie0 May 01 '13 at 14:59

Encryption issue in c++

2 Answers2